Skip to content

Commit fb84cd2

Browse files
filmorfilmor
authored
Merge pull request #704 from williamsardar/master
Fix memory leaks due to spurious handle increment and add unit tests
2 parents 611814c + a97a776 commit fb84cd2

File tree

8 files changed

+158
-6
lines changed

8 files changed

+158
-6
lines changed

‎AUTHORS.md

+1
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
- Ville M. Vainio ([@vivainio](https://github.com/vivainio))
4343
- Virgil Dupras ([@hsoft](https://github.com/hsoft))
4444
- Wenguang Yang ([@yagweb](https://github.com/yagweb))
45+
- William Sardar ([@williamsardar])(https://github.com/williamsardar)
4546
- Xavier Dupré ([@sdpython](https://github.com/sdpython))
4647
- Zane Purvis ([@zanedp](https://github.com/zanedp))
4748
- ([@bltribble](https://github.com/bltribble))

‎CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ This document follows the conventions laid out in [Keep a CHANGELOG][].
3535
- Fixed `LockRecursionException` when loading assemblies ([#627][i627])
3636
- Fixed errors breaking .NET Remoting on method invoke ([#276][i276])
3737
- Fixed PyObject.GetHashCode ([#676][i676])
38+
- Fix memory leaks due to spurious handle incrementation ([#691][i691])
3839

3940

4041
## [2.3.0][] - 2017-03-11

‎requirements.txt

+1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
# Requirements for both Travis and AppVeyor
22
pytest==3.2.5
33
coverage
4+
psutil
45

56
# Coverage upload
67
codecov

‎src/runtime/constructorbinding.cs

+1-3
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ public static IntPtr tp_descr_get(IntPtr op, IntPtr instance, IntPtr owner)
7575
return Exceptions.RaiseTypeError("How in the world could that happen!");
7676
}
7777
}*/
78-
Runtime.XIncref(self.pyHandle); // Decref'd by the interpreter.
78+
Runtime.XIncref(self.pyHandle);
7979
return self.pyHandle;
8080
}
8181

@@ -105,8 +105,6 @@ public static IntPtr mp_subscript(IntPtr op, IntPtr key)
105105
}
106106
var boundCtor = new BoundContructor(self.type, self.pyTypeHndl, self.ctorBinder, ci);
107107

108-
/* Since nothing is cached, do we need the increment???
109-
Runtime.XIncref(boundCtor.pyHandle); // Decref'd by the interpreter??? */
110108
return boundCtor.pyHandle;
111109
}
112110

‎src/runtime/methodbinding.cs

-2
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,6 @@ public static IntPtr mp_subscript(IntPtr tp, IntPtr idx)
5656
}
5757

5858
var mb = new MethodBinding(self.m, self.target) { info = mi };
59-
Runtime.XIncref(mb.pyHandle);
6059
return mb.pyHandle;
6160
}
6261

@@ -85,7 +84,6 @@ public static IntPtr tp_getattro(IntPtr ob, IntPtr key)
8584
case "__overloads__":
8685
case "Overloads":
8786
var om = new OverloadMapper(self.m, self.target);
88-
Runtime.XIncref(om.pyHandle);
8987
return om.pyHandle;
9088
default:
9189
return Runtime.PyObject_GenericGetAttr(ob, key);

‎src/runtime/overload.cs

-1
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,6 @@ public static IntPtr mp_subscript(IntPtr tp, IntPtr idx)
4444
}
4545

4646
var mb = new MethodBinding(self.m, self.target) { info = mi };
47-
Runtime.XIncref(mb.pyHandle);
4847
return mb.pyHandle;
4948
}
5049

‎src/testing/methodtest.cs

+20
Original file line numberDiff line numberDiff line change
@@ -666,3 +666,23 @@ public string PublicMethod(string echo)
666666
}
667667
}
668668
}
669+
670+
namespace PlainOldNamespace
671+
{
672+
public class PlainOldClass
673+
{
674+
public PlainOldClass() { }
675+
676+
public PlainOldClass(int param) { }
677+
678+
private readonly byte[] payload = new byte[(int)Math.Pow(2, 20)]; //1 MB
679+
680+
public void NonGenericMethod() { }
681+
682+
public void GenericMethod<T>() { }
683+
684+
public void OverloadedMethod() { }
685+
686+
public void OverloadedMethod(int param) { }
687+
}
688+
}

‎src/tests/test_method.py

+134
Original file line numberDiff line numberDiff line change
@@ -832,3 +832,137 @@ def test_case_sensitive():
832832

833833
with pytest.raises(AttributeError):
834834
MethodTest.casesensitive()
835+
836+
def test_getting_generic_method_binding_does_not_leak_ref_count():
837+
"""Test that managed object is freed after calling generic method. Issue #691"""
838+
839+
from PlainOldNamespace import PlainOldClass
840+
841+
import sys
842+
843+
refCount = sys.getrefcount(PlainOldClass().GenericMethod[str])
844+
assert refCount == 1
845+
846+
def test_getting_generic_method_binding_does_not_leak_memory():
847+
"""Test that managed object is freed after calling generic method. Issue #691"""
848+
849+
from PlainOldNamespace import PlainOldClass
850+
851+
import psutil, os, gc, clr
852+
853+
process = psutil.Process(os.getpid())
854+
processBytesBeforeCall = process.memory_info().rss
855+
print("\n\nMemory consumption (bytes) at start of test: " + str(processBytesBeforeCall))
856+
857+
iterations = 500
858+
for i in range(iterations):
859+
PlainOldClass().GenericMethod[str]
860+
861+
gc.collect()
862+
clr.System.GC.Collect()
863+
864+
processBytesAfterCall = process.memory_info().rss
865+
print("Memory consumption (bytes) at end of test: " + str(processBytesAfterCall))
866+
processBytesDelta = processBytesAfterCall - processBytesBeforeCall
867+
print("Memory delta: " + str(processBytesDelta))
868+
869+
bytesAllocatedPerIteration = pow(2, 20) # 1MB
870+
bytesLeakedPerIteration = processBytesDelta / iterations
871+
872+
# Allow 50% threshold - this shows the original issue is fixed, which leaks the full allocated bytes per iteration
873+
failThresholdBytesLeakedPerIteration = bytesAllocatedPerIteration / 2
874+
875+
assert bytesLeakedPerIteration < failThresholdBytesLeakedPerIteration
876+
877+
def test_getting_overloaded_method_binding_does_not_leak_ref_count():
878+
"""Test that managed object is freed after calling overloaded method. Issue #691"""
879+
880+
from PlainOldNamespace import PlainOldClass
881+
882+
import sys
883+
884+
refCount = sys.getrefcount(PlainOldClass().OverloadedMethod.Overloads[int])
885+
assert refCount == 1
886+
887+
def test_getting_overloaded_method_binding_does_not_leak_memory():
888+
"""Test that managed object is freed after calling overloaded method. Issue #691"""
889+
890+
from PlainOldNamespace import PlainOldClass
891+
892+
import psutil, os, gc, clr
893+
894+
process = psutil.Process(os.getpid())
895+
processBytesBeforeCall = process.memory_info().rss
896+
print("\n\nMemory consumption (bytes) at start of test: " + str(processBytesBeforeCall))
897+
898+
iterations = 500
899+
for i in range(iterations):
900+
PlainOldClass().OverloadedMethod.Overloads[int]
901+
902+
gc.collect()
903+
clr.System.GC.Collect()
904+
905+
processBytesAfterCall = process.memory_info().rss
906+
print("Memory consumption (bytes) at end of test: " + str(processBytesAfterCall))
907+
processBytesDelta = processBytesAfterCall - processBytesBeforeCall
908+
print("Memory delta: " + str(processBytesDelta))
909+
910+
bytesAllocatedPerIteration = pow(2, 20) # 1MB
911+
bytesLeakedPerIteration = processBytesDelta / iterations
912+
913+
# Allow 50% threshold - this shows the original issue is fixed, which leaks the full allocated bytes per iteration
914+
failThresholdBytesLeakedPerIteration = bytesAllocatedPerIteration / 2
915+
916+
assert bytesLeakedPerIteration < failThresholdBytesLeakedPerIteration
917+
918+
def test_getting_method_overloads_binding_does_not_leak_ref_count():
919+
"""Test that managed object is freed after calling overloaded method. Issue #691"""
920+
921+
from PlainOldNamespace import PlainOldClass
922+
923+
import sys
924+
925+
refCount = sys.getrefcount(PlainOldClass().OverloadedMethod.Overloads)
926+
assert refCount == 1
927+
928+
def test_getting_method_overloads_binding_does_not_leak_memory():
929+
"""Test that managed object is freed after calling overloaded method. Issue #691"""
930+
931+
from PlainOldNamespace import PlainOldClass
932+
933+
import psutil, os, gc, clr
934+
935+
process = psutil.Process(os.getpid())
936+
processBytesBeforeCall = process.memory_info().rss
937+
print("\n\nMemory consumption (bytes) at start of test: " + str(processBytesBeforeCall))
938+
939+
iterations = 500
940+
for i in range(iterations):
941+
PlainOldClass().OverloadedMethod.Overloads
942+
943+
gc.collect()
944+
clr.System.GC.Collect()
945+
946+
processBytesAfterCall = process.memory_info().rss
947+
print("Memory consumption (bytes) at end of test: " + str(processBytesAfterCall))
948+
processBytesDelta = processBytesAfterCall - processBytesBeforeCall
949+
print("Memory delta: " + str(processBytesDelta))
950+
951+
bytesAllocatedPerIteration = pow(2, 20) # 1MB
952+
bytesLeakedPerIteration = processBytesDelta / iterations
953+
954+
# Allow 50% threshold - this shows the original issue is fixed, which leaks the full allocated bytes per iteration
955+
failThresholdBytesLeakedPerIteration = bytesAllocatedPerIteration / 2
956+
957+
assert bytesLeakedPerIteration < failThresholdBytesLeakedPerIteration
958+
959+
def test_getting_overloaded_constructor_binding_does_not_leak_ref_count():
960+
"""Test that managed object is freed after calling overloaded constructor, constructorbinding.cs mp_subscript. Issue #691"""
961+
962+
from PlainOldNamespace import PlainOldClass
963+
964+
import sys
965+
966+
# simple test
967+
refCount = sys.getrefcount(PlainOldClass.Overloads[int])
968+
assert refCount == 1

0 commit comments

Comments
 (0)