pivotal-jbarrett commented on a change in pull request #786:
URL: https://github.com/apache/geode-native/pull/786#discussion_r617104208
##########
File path: clicache/src/Cache.cpp
##########
@@ -57,6 +56,21 @@ namespace Apache
m_typeRegistry = gcnew Apache::Geode::Client::TypeRegistry(this);
}
+ Cache::~Cache() { //Destructor - deterministic
+ if (!_disposed) {
+ //Clean-up managed resources
+ this->!Cache();
+ _disposed = true;
Review comment:
Member variables should be consistently formatted.
Should be `m_disposed` to be consistent with old convention or `disposed_`
for current convention.
##########
File path: clicache/integration-test2/GarbageCollectCache.cs
##########
@@ -0,0 +1,177 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+using System;
+using System.Collections;
+using System.Collections.Generic;
+using System.Diagnostics;
+using System.IO;
+using System.Linq;
+using Xunit;
+using Xunit.Abstractions;
+
+namespace Apache.Geode.Client.IntegrationTests
+{
+ [Trait("Category", "Integration")]
+ public class GarbageCollectCache : TestBase
+ {
+ public GarbageCollectCache(ITestOutputHelper testOutputHelper) :
base(testOutputHelper)
+ {
+ }
+
+ [Fact]
+ public void VerifyNoLeakedThreads()
+ {
+ using (var cluster = new Cluster(output,
CreateTestCaseDirectoryName(), 1, 1))
+ {
+ Assert.True(cluster.Start());
+ Assert.Equal(0, cluster.Gfsh
+ .create()
+ .region()
+ .withName("testRegion")
+ .withType("PARTITION")
+ .execute());
+
+ for (int i=0; i<25; i++)
+ {
+ var ncThreadsBefore =
Process.GetCurrentProcess().Threads.Count;
+
+ using (var cache = new CacheFactory()
+ .Set("log-level", "none")
+ .Create())
+ {
+
+
cluster.ApplyLocators(cache.GetPoolFactory()).Create("default");
+
+ var regionFactory =
cache.CreateRegionFactory(RegionShortcut.PROXY)
+ .SetPoolName("default");
+
+ var region = regionFactory.Create<string,
string>("testRegion");
+
+ const string key = "hello";
+ const string expectedResult = "dave";
+
+ region.Put(key, expectedResult);
+ var actualResult = region.Get(key);
+ Assert.Equal(expectedResult, actualResult);
+
+ cache.Close();
+ }
+
+ var ncThreadsAfter =
Process.GetCurrentProcess().Threads.Count;
+ var ratio = ncThreadsBefore / (double)ncThreadsAfter;
+
+ // Because the number of threads in the process depends on
+ // the number of cores, debug vs release, whether the GC
thread
+ // is running, etc., a robust test is to check whether the
ratio
+ // of before and after threads is close to one. Also
skipping the
+ // first couple of iterations avoids threads related to
test
+ // environment startup.
+ if (i > 5)
+ {
+ //Assert.True(.8 < ratio && ratio < 1.3);
+ string error = "ncThreadsBefore = " +
ncThreadsBefore.ToString() +
+ ", ncThreadsAfter = " + ncThreadsAfter.ToString();
+ Assert.False(!(.8 < ratio && ratio < 1.3), error);
+ }
+ }
+ }
+ }
+
+ [Fact(Skip = "Need a heuristic to filter out GC effect.")]
Review comment:
Added a skipped test? What does this do for us?
##########
File path: clicache/src/native_shared_ptr.hpp
##########
@@ -35,7 +35,8 @@ namespace Apache
std::shared_ptr<_T>* ptr;
public:
- native_shared_ptr(const std::shared_ptr<_T>& ptr) : ptr(new
std::shared_ptr<_T>(ptr)) {}
+ native_shared_ptr(const std::shared_ptr<_T>& ptr) : ptr(new
std::shared_ptr<_T>(ptr)) {
Review comment:
Intentional formatting change? Almost all our empty methods are `{}` on
the same line.
##########
File path: clicache/src/Cache.cpp
##########
@@ -57,6 +56,21 @@ namespace Apache
m_typeRegistry = gcnew Apache::Geode::Client::TypeRegistry(this);
}
+ Cache::~Cache() { //Destructor - deterministic
Review comment:
This sure looks like we copied comments from an example somewhere. I
also don't think they provide meaningful content. The method name makes it
clear it is a destructor.
##########
File path: clicache/src/Cache.cpp
##########
@@ -57,6 +56,21 @@ namespace Apache
m_typeRegistry = gcnew Apache::Geode::Client::TypeRegistry(this);
}
+ Cache::~Cache() { //Destructor - deterministic
+ if (!_disposed) {
+ //Clean-up managed resources
+ this->!Cache();
+ _disposed = true;
+ //GC.SuppressFinalize(this) is automatically added here
+ //Base destructor is automatically called too if needed
+ }
+ }
+
+ Cache::!Cache() { //Finalizer - non-deterministic when called by GC
+ //Clean-up unmanaged resources
+ delete m_nativeptr;
Review comment:
Technically this value is a managed resource. If you are following the
example from
https://manski.net/2012/01/idisposable-finalizer-and-suppressfinalize/ then it
should be deleted in the destructor.
##########
File path: cppcache/include/geode/Cache.hpp
##########
@@ -167,17 +167,17 @@ class APACHE_GEODE_EXPORT Cache : public GeodeCache {
virtual void readyForEvents();
/**
- * Creates an authenticated cache using the given user security properties.
- * Multiple instances with different user properties can be created with a
- * single client cache.
+ * Creates an authenticated cache using the given user security
+ * properties. Multiple instances with different user properties can be
Review comment:
Why is this suddenly changing format? Will clang-format choke on this?
##########
File path: clicache/integration-test2/GarbageCollectCache.cs
##########
@@ -0,0 +1,177 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+using System;
+using System.Collections;
+using System.Collections.Generic;
+using System.Diagnostics;
+using System.IO;
+using System.Linq;
+using Xunit;
+using Xunit.Abstractions;
+
+namespace Apache.Geode.Client.IntegrationTests
+{
+ [Trait("Category", "Integration")]
+ public class GarbageCollectCache : TestBase
+ {
+ public GarbageCollectCache(ITestOutputHelper testOutputHelper) :
base(testOutputHelper)
+ {
+ }
+
+ [Fact]
+ public void VerifyNoLeakedThreads()
+ {
+ using (var cluster = new Cluster(output,
CreateTestCaseDirectoryName(), 1, 1))
+ {
+ Assert.True(cluster.Start());
+ Assert.Equal(0, cluster.Gfsh
+ .create()
+ .region()
+ .withName("testRegion")
+ .withType("PARTITION")
+ .execute());
+
+ for (int i=0; i<25; i++)
+ {
+ var ncThreadsBefore =
Process.GetCurrentProcess().Threads.Count;
+
+ using (var cache = new CacheFactory()
+ .Set("log-level", "none")
+ .Create())
+ {
+
+
cluster.ApplyLocators(cache.GetPoolFactory()).Create("default");
+
+ var regionFactory =
cache.CreateRegionFactory(RegionShortcut.PROXY)
+ .SetPoolName("default");
+
+ var region = regionFactory.Create<string,
string>("testRegion");
+
+ const string key = "hello";
+ const string expectedResult = "dave";
+
+ region.Put(key, expectedResult);
+ var actualResult = region.Get(key);
+ Assert.Equal(expectedResult, actualResult);
+
+ cache.Close();
+ }
+
+ var ncThreadsAfter =
Process.GetCurrentProcess().Threads.Count;
+ var ratio = ncThreadsBefore / (double)ncThreadsAfter;
+
+ // Because the number of threads in the process depends on
+ // the number of cores, debug vs release, whether the GC
thread
+ // is running, etc., a robust test is to check whether the
ratio
+ // of before and after threads is close to one. Also
skipping the
+ // first couple of iterations avoids threads related to
test
+ // environment startup.
+ if (i > 5)
+ {
+ //Assert.True(.8 < ratio && ratio < 1.3);
+ string error = "ncThreadsBefore = " +
ncThreadsBefore.ToString() +
+ ", ncThreadsAfter = " + ncThreadsAfter.ToString();
+ Assert.False(!(.8 < ratio && ratio < 1.3), error);
+ }
+ }
+ }
+ }
+
+ [Fact(Skip = "Need a heuristic to filter out GC effect.")]
+ public void VerifyNoLeakedMemoryPutAllGetAll()
+ {
+ using (var cluster = new Cluster(output,
CreateTestCaseDirectoryName(), 1, 1))
+ {
+ Assert.True(cluster.Start());
+ Assert.Equal(0, cluster.Gfsh
+ .create()
+ .region()
+ .withName("testRegion")
+ .withType("PARTITION")
+ .execute());
+
+ const int numEntries = 1000;
+ const int numIterations = 100;
+ const int stringLength = 1000;
+
+
+ Int64[] workingSetBefore = new Int64[numIterations];
+ Int64[] workingSetAfter = new Int64[numIterations];
+ string value = new string('*', stringLength);
+
+ for (int i = 0; i < numIterations; i++)
+ {
+ workingSetBefore[i] =
(Int64)Process.GetCurrentProcess().WorkingSet64;
+
+ using (var cache = new CacheFactory()
+ .Set("log-level", "none")
+ .Create())
+ {
+
+
cluster.ApplyLocators(cache.GetPoolFactory()).Create("default");
+
+ var regionFactory =
cache.CreateRegionFactory(RegionShortcut.PROXY)
+ .SetPoolName("default");
+
+ var region = regionFactory.Create<object,
object>("testRegion");
+
+ IDictionary<object, object> collection = new
Dictionary<object, object>();
+
+ for (int n = 0; n < numEntries; n++)
+ {
+ collection.Add(n, value);
+ }
+
+ region.PutAll(collection);
+
+ List<Object> keys = new List<Object>();
+ for (int item = 0; item < numEntries; item++)
+ {
+ Object K = item;
+ keys.Add(K);
+ }
+ Dictionary<Object, Object> values = new
Dictionary<Object, Object>();
+ region.GetAll(keys.ToArray(), values, null, true);
+
+ cache.Close();
+ }
+
+ // Because the are many small leaks in the native client,
the
+ // heuristic below is based on empirical data on across
many iterations
+ // to ensure there are no huge mmemory leaks. After a
warmup,
+ // measurements are taken across several iterations
+ // to see the trend across many garbage collect cycles.
+
+ //if (i > 10)
+ // Assert.True(.9 < ratio && ratio < 1.1);
+
+ workingSetAfter[i] =
(Int64)Process.GetCurrentProcess().WorkingSet64;
+
+ }
+
+ Int64[] diff = new Int64[numIterations];
+ for (int i=0; i<numIterations; i++)
+ {
+ diff[i] = workingSetAfter[i] - workingSetBefore[i];
+ }
+
+ Console.WriteLine("diff[99] = {0}", diff[numIterations-1]);
Review comment:
Tests should not be writing to the console.
##########
File path: clicache/integration-test2/GarbageCollectCache.cs
##########
@@ -0,0 +1,177 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+using System;
+using System.Collections;
+using System.Collections.Generic;
+using System.Diagnostics;
+using System.IO;
+using System.Linq;
+using Xunit;
+using Xunit.Abstractions;
+
+namespace Apache.Geode.Client.IntegrationTests
+{
+ [Trait("Category", "Integration")]
+ public class GarbageCollectCache : TestBase
+ {
+ public GarbageCollectCache(ITestOutputHelper testOutputHelper) :
base(testOutputHelper)
+ {
+ }
+
+ [Fact]
+ public void VerifyNoLeakedThreads()
+ {
+ using (var cluster = new Cluster(output,
CreateTestCaseDirectoryName(), 1, 1))
+ {
+ Assert.True(cluster.Start());
+ Assert.Equal(0, cluster.Gfsh
+ .create()
+ .region()
+ .withName("testRegion")
+ .withType("PARTITION")
+ .execute());
+
+ for (int i=0; i<25; i++)
+ {
+ var ncThreadsBefore =
Process.GetCurrentProcess().Threads.Count;
+
+ using (var cache = new CacheFactory()
+ .Set("log-level", "none")
+ .Create())
+ {
+
+
cluster.ApplyLocators(cache.GetPoolFactory()).Create("default");
+
+ var regionFactory =
cache.CreateRegionFactory(RegionShortcut.PROXY)
+ .SetPoolName("default");
+
+ var region = regionFactory.Create<string,
string>("testRegion");
+
+ const string key = "hello";
+ const string expectedResult = "dave";
+
+ region.Put(key, expectedResult);
+ var actualResult = region.Get(key);
+ Assert.Equal(expectedResult, actualResult);
+
+ cache.Close();
+ }
+
+ var ncThreadsAfter =
Process.GetCurrentProcess().Threads.Count;
+ var ratio = ncThreadsBefore / (double)ncThreadsAfter;
+
+ // Because the number of threads in the process depends on
+ // the number of cores, debug vs release, whether the GC
thread
+ // is running, etc., a robust test is to check whether the
ratio
+ // of before and after threads is close to one. Also
skipping the
+ // first couple of iterations avoids threads related to
test
+ // environment startup.
+ if (i > 5)
+ {
+ //Assert.True(.8 < ratio && ratio < 1.3);
+ string error = "ncThreadsBefore = " +
ncThreadsBefore.ToString() +
+ ", ncThreadsAfter = " + ncThreadsAfter.ToString();
+ Assert.False(!(.8 < ratio && ratio < 1.3), error);
+ }
+ }
+ }
+ }
+
+ [Fact(Skip = "Need a heuristic to filter out GC effect.")]
+ public void VerifyNoLeakedMemoryPutAllGetAll()
+ {
+ using (var cluster = new Cluster(output,
CreateTestCaseDirectoryName(), 1, 1))
+ {
+ Assert.True(cluster.Start());
+ Assert.Equal(0, cluster.Gfsh
+ .create()
+ .region()
+ .withName("testRegion")
+ .withType("PARTITION")
+ .execute());
+
+ const int numEntries = 1000;
+ const int numIterations = 100;
+ const int stringLength = 1000;
+
+
+ Int64[] workingSetBefore = new Int64[numIterations];
+ Int64[] workingSetAfter = new Int64[numIterations];
+ string value = new string('*', stringLength);
+
+ for (int i = 0; i < numIterations; i++)
+ {
+ workingSetBefore[i] =
(Int64)Process.GetCurrentProcess().WorkingSet64;
+
+ using (var cache = new CacheFactory()
+ .Set("log-level", "none")
+ .Create())
+ {
+
+
cluster.ApplyLocators(cache.GetPoolFactory()).Create("default");
+
+ var regionFactory =
cache.CreateRegionFactory(RegionShortcut.PROXY)
+ .SetPoolName("default");
+
+ var region = regionFactory.Create<object,
object>("testRegion");
+
+ IDictionary<object, object> collection = new
Dictionary<object, object>();
+
+ for (int n = 0; n < numEntries; n++)
+ {
+ collection.Add(n, value);
+ }
+
+ region.PutAll(collection);
+
+ List<Object> keys = new List<Object>();
+ for (int item = 0; item < numEntries; item++)
+ {
+ Object K = item;
+ keys.Add(K);
+ }
+ Dictionary<Object, Object> values = new
Dictionary<Object, Object>();
+ region.GetAll(keys.ToArray(), values, null, true);
+
+ cache.Close();
+ }
+
+ // Because the are many small leaks in the native client,
the
+ // heuristic below is based on empirical data on across
many iterations
+ // to ensure there are no huge mmemory leaks. After a
warmup,
+ // measurements are taken across several iterations
+ // to see the trend across many garbage collect cycles.
+
+ //if (i > 10)
+ // Assert.True(.9 < ratio && ratio < 1.1);
+
+ workingSetAfter[i] =
(Int64)Process.GetCurrentProcess().WorkingSet64;
+
+ }
+
+ Int64[] diff = new Int64[numIterations];
+ for (int i=0; i<numIterations; i++)
+ {
+ diff[i] = workingSetAfter[i] - workingSetBefore[i];
+ }
+
+ Console.WriteLine("diff[99] = {0}", diff[numIterations-1]);
+ }
+ }
+ }
+}
Review comment:
Missing blank line at EOF.
##########
File path: clicache/src/Cache.cpp
##########
@@ -57,6 +56,21 @@ namespace Apache
m_typeRegistry = gcnew Apache::Geode::Client::TypeRegistry(this);
}
+ Cache::~Cache() { //Destructor - deterministic
+ if (!_disposed) {
+ //Clean-up managed resources
+ this->!Cache();
+ _disposed = true;
Review comment:
Not thread safe. If two threads both invoke dispose, boom! Given the
C++/CLI auto generates the `Dispose` method around the destructor we really
aren't given a chance to make that discoverable in the documentation.
Safest to use an atomic check and set operation in the if check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
