From b2e1bb2e188e5616690944f3f7c0da36c2efa3e3 Mon Sep 17 00:00:00 2001 From: William Sardar Date: Fri, 20 Jul 2018 16:25:51 +0100 Subject: [PATCH 1/6] Fix memory leaks and add unit tests --- src/runtime/constructorbinding.cs | 1 - src/runtime/methodbinding.cs | 2 - src/runtime/overload.cs | 1 - src/testing/methodtest.cs | 20 +++++ src/tests/test_method.py | 134 ++++++++++++++++++++++++++++++ 5 files changed, 154 insertions(+), 4 deletions(-) diff --git a/src/runtime/constructorbinding.cs b/src/runtime/constructorbinding.cs index 4839f9913..c76ded89e 100644 --- a/src/runtime/constructorbinding.cs +++ b/src/runtime/constructorbinding.cs @@ -75,7 +75,6 @@ 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. return self.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..1be9fea6e 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().private + 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().private + 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().private + 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().private + 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().private + 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().private + 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 From aa222b3d18c09a5e81e379e33a2d764a680a66af Mon Sep 17 00:00:00 2001 From: William Sardar Date: Fri, 20 Jul 2018 16:35:43 +0100 Subject: [PATCH 2/6] Add changelog and authors entries --- AUTHORS.md | 1 + CHANGELOG.md | 1 + 2 files changed, 2 insertions(+) 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 From b6c024cea5b7488983546aff90201c98f532161e Mon Sep 17 00:00:00 2001 From: William Sardar Date: Fri, 20 Jul 2018 17:11:18 +0100 Subject: [PATCH 3/6] Add new requirement for memory leak testing --- requirements.txt | 1 + 1 file changed, 1 insertion(+) 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 From bcf98413665fa7789f2d378bc4803b7a2ba5fb43 Mon Sep 17 00:00:00 2001 From: William Sardar Date: Fri, 20 Jul 2018 17:21:00 +0100 Subject: [PATCH 4/6] Update memory monitoring call to use a platform-independent field --- src/tests/test_method.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/tests/test_method.py b/src/tests/test_method.py index 1be9fea6e..ad678611b 100644 --- a/src/tests/test_method.py +++ b/src/tests/test_method.py @@ -851,7 +851,7 @@ def test_getting_generic_method_binding_does_not_leak_memory(): import psutil, os, gc, clr process = psutil.Process(os.getpid()) - processBytesBeforeCall = process.memory_info().private + processBytesBeforeCall = process.memory_info().rss print("\n\nMemory consumption (bytes) at start of test: " + str(processBytesBeforeCall)) iterations = 500 @@ -861,7 +861,7 @@ def test_getting_generic_method_binding_does_not_leak_memory(): gc.collect() clr.System.GC.Collect() - processBytesAfterCall = process.memory_info().private + processBytesAfterCall = process.memory_info().rss print("Memory consumption (bytes) at end of test: " + str(processBytesAfterCall)) processBytesDelta = processBytesAfterCall - processBytesBeforeCall print("Memory delta: " + str(processBytesDelta)) @@ -892,7 +892,7 @@ def test_getting_overloaded_method_binding_does_not_leak_memory(): import psutil, os, gc, clr process = psutil.Process(os.getpid()) - processBytesBeforeCall = process.memory_info().private + processBytesBeforeCall = process.memory_info().rss print("\n\nMemory consumption (bytes) at start of test: " + str(processBytesBeforeCall)) iterations = 500 @@ -902,7 +902,7 @@ def test_getting_overloaded_method_binding_does_not_leak_memory(): gc.collect() clr.System.GC.Collect() - processBytesAfterCall = process.memory_info().private + processBytesAfterCall = process.memory_info().rss print("Memory consumption (bytes) at end of test: " + str(processBytesAfterCall)) processBytesDelta = processBytesAfterCall - processBytesBeforeCall print("Memory delta: " + str(processBytesDelta)) @@ -933,7 +933,7 @@ def test_getting_method_overloads_binding_does_not_leak_memory(): import psutil, os, gc, clr process = psutil.Process(os.getpid()) - processBytesBeforeCall = process.memory_info().private + processBytesBeforeCall = process.memory_info().rss print("\n\nMemory consumption (bytes) at start of test: " + str(processBytesBeforeCall)) iterations = 500 @@ -943,7 +943,7 @@ def test_getting_method_overloads_binding_does_not_leak_memory(): gc.collect() clr.System.GC.Collect() - processBytesAfterCall = process.memory_info().private + processBytesAfterCall = process.memory_info().rss print("Memory consumption (bytes) at end of test: " + str(processBytesAfterCall)) processBytesDelta = processBytesAfterCall - processBytesBeforeCall print("Memory delta: " + str(processBytesDelta)) From 66d31bb5200e1e7d667d1ec0e36c7b3b6f0496cc Mon Sep 17 00:00:00 2001 From: William Sardar Date: Fri, 20 Jul 2018 17:30:16 +0100 Subject: [PATCH 5/6] Delete commented code --- src/runtime/constructorbinding.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/runtime/constructorbinding.cs b/src/runtime/constructorbinding.cs index c76ded89e..06f6362f2 100644 --- a/src/runtime/constructorbinding.cs +++ b/src/runtime/constructorbinding.cs @@ -104,8 +104,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; } From a97a7768e0289e1b53ffc1ab67d89343d15bca44 Mon Sep 17 00:00:00 2001 From: William Sardar Date: Fri, 20 Jul 2018 17:37:45 +0100 Subject: [PATCH 6/6] Back out overzealous line deletion! --- src/runtime/constructorbinding.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/runtime/constructorbinding.cs b/src/runtime/constructorbinding.cs index 06f6362f2..3908628b9 100644 --- a/src/runtime/constructorbinding.cs +++ b/src/runtime/constructorbinding.cs @@ -75,6 +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); return self.pyHandle; }