diff --git a/AUTHORS.md b/AUTHORS.md index e37ec6436..3c39794e4 100644 --- a/AUTHORS.md +++ b/AUTHORS.md @@ -42,6 +42,7 @@ - Ville M. Vainio ([@vivainio](https://github.com/vivainio)) - Virgil Dupras ([@hsoft](https://github.com/hsoft)) - Wenguang Yang ([@yagweb](https://github.com/yagweb)) +- William Sardar ([@williamsardar])(https://github.com/williamsardar) - Xavier Dupré ([@sdpython](https://github.com/sdpython)) - Zane Purvis ([@zanedp](https://github.com/zanedp)) - ([@bltribble](https://github.com/bltribble)) diff --git a/CHANGELOG.md b/CHANGELOG.md index a19f4a10c..3dc668b0c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,7 @@ This document follows the conventions laid out in [Keep a CHANGELOG][]. - Fixed `LockRecursionException` when loading assemblies ([#627][i627]) - Fixed errors breaking .NET Remoting on method invoke ([#276][i276]) - Fixed PyObject.GetHashCode ([#676][i676]) +- Fix memory leaks due to spurious handle incrementation ([#691][i691]) ## [2.3.0][] - 2017-03-11 diff --git a/requirements.txt b/requirements.txt index 81adc3672..29c2e4566 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,6 +1,7 @@ # Requirements for both Travis and AppVeyor pytest==3.2.5 coverage +psutil # Coverage upload codecov diff --git a/src/runtime/constructorbinding.cs b/src/runtime/constructorbinding.cs index 4839f9913..3908628b9 100644 --- a/src/runtime/constructorbinding.cs +++ b/src/runtime/constructorbinding.cs @@ -75,7 +75,7 @@ public static IntPtr tp_descr_get(IntPtr op, IntPtr instance, IntPtr owner) return Exceptions.RaiseTypeError("How in the world could that happen!"); } }*/ - Runtime.XIncref(self.pyHandle); // Decref'd by the interpreter. + Runtime.XIncref(self.pyHandle); return self.pyHandle; } @@ -105,8 +105,6 @@ public static IntPtr mp_subscript(IntPtr op, IntPtr key) } var boundCtor = new BoundContructor(self.type, self.pyTypeHndl, self.ctorBinder, ci); - /* Since nothing is cached, do we need the increment??? - Runtime.XIncref(boundCtor.pyHandle); // Decref'd by the interpreter??? */ return boundCtor.pyHandle; } diff --git a/src/runtime/methodbinding.cs b/src/runtime/methodbinding.cs index 07090a92c..3985ed2cb 100644 --- a/src/runtime/methodbinding.cs +++ b/src/runtime/methodbinding.cs @@ -56,7 +56,6 @@ public static IntPtr mp_subscript(IntPtr tp, IntPtr idx) } var mb = new MethodBinding(self.m, self.target) { info = mi }; - Runtime.XIncref(mb.pyHandle); return mb.pyHandle; } @@ -85,7 +84,6 @@ public static IntPtr tp_getattro(IntPtr ob, IntPtr key) case "__overloads__": case "Overloads": var om = new OverloadMapper(self.m, self.target); - Runtime.XIncref(om.pyHandle); return om.pyHandle; default: return Runtime.PyObject_GenericGetAttr(ob, key); diff --git a/src/runtime/overload.cs b/src/runtime/overload.cs index 6b48299e8..67868a1b1 100644 --- a/src/runtime/overload.cs +++ b/src/runtime/overload.cs @@ -44,7 +44,6 @@ public static IntPtr mp_subscript(IntPtr tp, IntPtr idx) } var mb = new MethodBinding(self.m, self.target) { info = mi }; - Runtime.XIncref(mb.pyHandle); return mb.pyHandle; } diff --git a/src/testing/methodtest.cs b/src/testing/methodtest.cs index 83dc907c0..cf653f9f9 100644 --- a/src/testing/methodtest.cs +++ b/src/testing/methodtest.cs @@ -666,3 +666,23 @@ public string PublicMethod(string echo) } } } + +namespace PlainOldNamespace +{ + public class PlainOldClass + { + public PlainOldClass() { } + + public PlainOldClass(int param) { } + + private readonly byte[] payload = new byte[(int)Math.Pow(2, 20)]; //1 MB + + public void NonGenericMethod() { } + + public void GenericMethod() { } + + public void OverloadedMethod() { } + + public void OverloadedMethod(int param) { } + } +} diff --git a/src/tests/test_method.py b/src/tests/test_method.py index ad182678d..ad678611b 100644 --- a/src/tests/test_method.py +++ b/src/tests/test_method.py @@ -832,3 +832,137 @@ def test_case_sensitive(): with pytest.raises(AttributeError): MethodTest.casesensitive() + +def test_getting_generic_method_binding_does_not_leak_ref_count(): + """Test that managed object is freed after calling generic method. Issue #691""" + + from PlainOldNamespace import PlainOldClass + + import sys + + refCount = sys.getrefcount(PlainOldClass().GenericMethod[str]) + assert refCount == 1 + +def test_getting_generic_method_binding_does_not_leak_memory(): + """Test that managed object is freed after calling generic method. Issue #691""" + + from PlainOldNamespace import PlainOldClass + + import psutil, os, gc, clr + + process = psutil.Process(os.getpid()) + processBytesBeforeCall = process.memory_info().rss + print("\n\nMemory consumption (bytes) at start of test: " + str(processBytesBeforeCall)) + + iterations = 500 + for i in range(iterations): + PlainOldClass().GenericMethod[str] + + gc.collect() + clr.System.GC.Collect() + + processBytesAfterCall = process.memory_info().rss + print("Memory consumption (bytes) at end of test: " + str(processBytesAfterCall)) + processBytesDelta = processBytesAfterCall - processBytesBeforeCall + print("Memory delta: " + str(processBytesDelta)) + + bytesAllocatedPerIteration = pow(2, 20) # 1MB + bytesLeakedPerIteration = processBytesDelta / iterations + + # Allow 50% threshold - this shows the original issue is fixed, which leaks the full allocated bytes per iteration + failThresholdBytesLeakedPerIteration = bytesAllocatedPerIteration / 2 + + assert bytesLeakedPerIteration < failThresholdBytesLeakedPerIteration + +def test_getting_overloaded_method_binding_does_not_leak_ref_count(): + """Test that managed object is freed after calling overloaded method. Issue #691""" + + from PlainOldNamespace import PlainOldClass + + import sys + + refCount = sys.getrefcount(PlainOldClass().OverloadedMethod.Overloads[int]) + assert refCount == 1 + +def test_getting_overloaded_method_binding_does_not_leak_memory(): + """Test that managed object is freed after calling overloaded method. Issue #691""" + + from PlainOldNamespace import PlainOldClass + + import psutil, os, gc, clr + + process = psutil.Process(os.getpid()) + processBytesBeforeCall = process.memory_info().rss + print("\n\nMemory consumption (bytes) at start of test: " + str(processBytesBeforeCall)) + + iterations = 500 + for i in range(iterations): + PlainOldClass().OverloadedMethod.Overloads[int] + + gc.collect() + clr.System.GC.Collect() + + processBytesAfterCall = process.memory_info().rss + print("Memory consumption (bytes) at end of test: " + str(processBytesAfterCall)) + processBytesDelta = processBytesAfterCall - processBytesBeforeCall + print("Memory delta: " + str(processBytesDelta)) + + bytesAllocatedPerIteration = pow(2, 20) # 1MB + bytesLeakedPerIteration = processBytesDelta / iterations + + # Allow 50% threshold - this shows the original issue is fixed, which leaks the full allocated bytes per iteration + failThresholdBytesLeakedPerIteration = bytesAllocatedPerIteration / 2 + + assert bytesLeakedPerIteration < failThresholdBytesLeakedPerIteration + +def test_getting_method_overloads_binding_does_not_leak_ref_count(): + """Test that managed object is freed after calling overloaded method. Issue #691""" + + from PlainOldNamespace import PlainOldClass + + import sys + + refCount = sys.getrefcount(PlainOldClass().OverloadedMethod.Overloads) + assert refCount == 1 + +def test_getting_method_overloads_binding_does_not_leak_memory(): + """Test that managed object is freed after calling overloaded method. Issue #691""" + + from PlainOldNamespace import PlainOldClass + + import psutil, os, gc, clr + + process = psutil.Process(os.getpid()) + processBytesBeforeCall = process.memory_info().rss + print("\n\nMemory consumption (bytes) at start of test: " + str(processBytesBeforeCall)) + + iterations = 500 + for i in range(iterations): + PlainOldClass().OverloadedMethod.Overloads + + gc.collect() + clr.System.GC.Collect() + + processBytesAfterCall = process.memory_info().rss + print("Memory consumption (bytes) at end of test: " + str(processBytesAfterCall)) + processBytesDelta = processBytesAfterCall - processBytesBeforeCall + print("Memory delta: " + str(processBytesDelta)) + + bytesAllocatedPerIteration = pow(2, 20) # 1MB + bytesLeakedPerIteration = processBytesDelta / iterations + + # Allow 50% threshold - this shows the original issue is fixed, which leaks the full allocated bytes per iteration + failThresholdBytesLeakedPerIteration = bytesAllocatedPerIteration / 2 + + assert bytesLeakedPerIteration < failThresholdBytesLeakedPerIteration + +def test_getting_overloaded_constructor_binding_does_not_leak_ref_count(): + """Test that managed object is freed after calling overloaded constructor, constructorbinding.cs mp_subscript. Issue #691""" + + from PlainOldNamespace import PlainOldClass + + import sys + + # simple test + refCount = sys.getrefcount(PlainOldClass.Overloads[int]) + assert refCount == 1