Skip to content

Commit 7fd6fc4

Browse files
author
dse
committed
Finalizer implementation improvement. PythonException memory leak fix. Review fixes.
1 parent 37c2449 commit 7fd6fc4

File tree

9 files changed

+165
-72
lines changed

9 files changed

+165
-72
lines changed

‎src/embed_tests/TestFinalizer.cs‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,5 +64,67 @@ public void TestClrObjectFullRelease()
6464

6565
Assert.IsFalse(weakRef.IsAlive, "Clr object should be collected.");
6666
}
67+
68+
[Test]
69+
[Ignore("For debug only")]
70+
public void TestExceptionMemoryLeak()
71+
{
72+
dynamic pymodule;
73+
PyObject gc;
74+
dynamic testmethod;
75+
76+
var ts = PythonEngine.BeginAllowThreads();
77+
IntPtr pylock = PythonEngine.AcquireLock();
78+
79+
#if NETCOREAPP
80+
const string s = "../../fixtures";
81+
#else
82+
const string s = "../fixtures";
83+
#endif
84+
string testPath = Path.Combine(TestContext.CurrentContext.TestDirectory, s);
85+
86+
IntPtr str = Runtime.Runtime.PyString_FromString(testPath);
87+
IntPtr path = Runtime.Runtime.PySys_GetObject("path");
88+
Runtime.Runtime.PyList_Append(path, str);
89+
90+
{
91+
PyObject sys = PythonEngine.ImportModule("sys");
92+
gc = PythonEngine.ImportModule("gc");
93+
94+
pymodule = PythonEngine.ImportModule("MemoryLeakTest.pyraise");
95+
testmethod = pymodule.test_raise_exception;
96+
}
97+
98+
PythonEngine.ReleaseLock(pylock);
99+
100+
double floatarg1 = 5.1f;
101+
dynamic res = null;
102+
{
103+
for (int i = 1; i <= 10000000; i++)
104+
{
105+
using (Py.GIL())
106+
{
107+
try
108+
{
109+
res = testmethod(Py.kw("number", floatarg1), Py.kw("astring", "bbc"));
110+
}
111+
catch (Exception e)
112+
{
113+
if (i % 10000 == 0)
114+
{
115+
TestContext.Progress.WriteLine(e.Message);
116+
}
117+
}
118+
}
119+
120+
if (i % 10000 == 0)
121+
{
122+
GC.Collect();
123+
}
124+
}
125+
}
126+
127+
PythonEngine.EndAllowThreads(ts);
128+
}
67129
}
68130
}

‎src/embed_tests/fixtures/MemoryLeakTest/pyraise.py‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
def test_raise_exception(number=3, astring='abc'):
2+
raise ValueError("testing for memory leak")
3+
return astring * int(number)
4+
5+
def test_raise_exception2(number, astring):
6+
#raise ValueError("testing for memory leak")
7+
#astring * int(number)
8+
return "test"

‎src/runtime/debughelper.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -116,10 +116,10 @@ internal static void debug(string msg)
116116
Console.WriteLine(" {0}", msg);
117117
}
118118

119-
/// <summary>
119+
/// <summary>
120120
/// Helper function to inspect/compare managed to native conversions.
121-
/// Especially useful when debugging CustomMarshaler.
122-
/// </summary>
121+
/// Especially useful when debugging CustomMarshaler.
122+
/// </summary>
123123
/// <param name="bytes"></param>
124124
[Conditional("DEBUG")]
125125
public static void PrintHexBytes(byte[] bytes)

‎src/runtime/pyiter.cs‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -40,14 +40,20 @@ public PyIter(PyObject iterable)
4040
}
4141
}
4242

43-
protected override void Dispose(bool disposing)
43+
protected internal override void Dispose(bool disposing)
4444
{
45-
if (null != _current)
45+
try
4646
{
47-
_current.Dispose();
48-
_current = null;
47+
if (null != _current)
48+
{
49+
_current.Dispose(disposing);
50+
_current = null;
51+
}
52+
}
53+
finally
54+
{
55+
base.Dispose(disposing);
4956
}
50-
base.Dispose(disposing);
5157
}
5258

5359
public bool MoveNext()

‎src/runtime/pyobject.cs‎

Lines changed: 12 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -132,39 +132,32 @@ public T As<T>()
132132
/// to Python objects will not be released until a managed garbage
133133
/// collection occurs.
134134
/// </remarks>
135-
protected virtual void Dispose(bool disposing)
135+
protected internal virtual void Dispose(bool disposing)
136136
{
137137
if (!disposed)
138138
{
139139
disposed = true;
140+
IntPtr objToDispose = obj;
141+
obj = IntPtr.Zero;
140142

141143
if (disposing)
142144
{
143-
try
145+
if (Runtime.Py_IsInitialized() > 0 && !Runtime.IsFinalizing)
144146
{
145-
if (Runtime.Py_IsInitialized() > 0 && !Runtime.IsFinalizing)
147+
IntPtr gs = PythonEngine.AcquireLock();
148+
try
146149
{
147-
IntPtr gs = PythonEngine.AcquireLock();
148-
try
149-
{
150-
Runtime.XDecref(obj);
151-
obj = IntPtr.Zero;
152-
}
153-
finally
154-
{
155-
PythonEngine.ReleaseLock(gs);
156-
}
150+
Runtime.XDecref(objToDispose);
151+
}
152+
finally
153+
{
154+
PythonEngine.ReleaseLock(gs);
157155
}
158-
}
159-
catch
160-
{
161-
// Do nothing.
162156
}
163157
}
164158
else
165159
{
166-
_referenceDecrementer?.ScheduleDecRef(obj);
167-
obj = IntPtr.Zero;
160+
_referenceDecrementer?.ScheduleDecRef(objToDispose);
168161
}
169162
}
170163
}

‎src/runtime/pyreferencedecrementer.cs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,14 @@
77

88
namespace Python.Runtime
99
{
10+
/// <summary>
11+
/// Background python objects reference decrementer.
12+
/// !!! By some unknown reasons python references decrements have been performed in the GC thread - affects GC. !!!
13+
/// This component contains background thread with the queue of IntPtr python object references.
14+
/// Finalizers should schedule reference decrement operation in this component to avoid problem with GC.
15+
/// Each PythonEngine Init/Shutdown have it's own PyReferenceDecrementer, so PyObject should also have a field with the
16+
/// reference to the correct PyReferenceDecrementer.
17+
/// </summary>
1018
internal class PyReferenceDecrementer : IDisposable
1119
{
1220
private static readonly DedicatedThreadTaskScheduler DedicatedThreadTaskScheduler = new DedicatedThreadTaskScheduler();

‎src/runtime/pyscope.cs‎

Lines changed: 22 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ public class PyScope : DynamicObject, IDisposable
3131
/// <summary>
3232
/// the python Module object the scope associated with.
3333
/// </summary>
34-
internal readonly IntPtr obj;
34+
internal IntPtr obj;
3535

3636
/// <summary>
3737
/// the variable dict of the scope.
@@ -522,34 +522,37 @@ protected virtual void Dispose(bool disposing)
522522
{
523523
if (!_isDisposed)
524524
{
525-
_isDisposed = true;
526-
525+
IntPtr objToDispose = obj;
527526
if (disposing)
528527
{
529528
try
530529
{
531-
if (Runtime.Py_IsInitialized() > 0 && !Runtime.IsFinalizing)
532-
{
533-
IntPtr gs = PythonEngine.AcquireLock();
534-
try
535-
{
536-
Runtime.XDecref(obj);
537-
}
538-
finally
539-
{
540-
PythonEngine.ReleaseLock(gs);
541-
}
542-
}
530+
OnDispose?.Invoke(this);
531+
}
532+
finally
533+
{
534+
_isDisposed = true;
535+
obj = IntPtr.Zero;
543536
}
544-
catch
537+
538+
if (Runtime.Py_IsInitialized() > 0 && !Runtime.IsFinalizing)
545539
{
546-
// Do nothing.
540+
IntPtr gs = PythonEngine.AcquireLock();
541+
try
542+
{
543+
Runtime.XDecref(objToDispose);
544+
}
545+
finally
546+
{
547+
PythonEngine.ReleaseLock(gs);
548+
}
547549
}
548-
OnDispose?.Invoke(this);
549550
}
550551
else
551552
{
552-
_referenceDecrementer?.ScheduleDecRef(obj);
553+
_isDisposed = true;
554+
obj = IntPtr.Zero;
555+
_referenceDecrementer?.ScheduleDecRef(objToDispose);
553556
}
554557
}
555558
}

‎src/runtime/pythonengine.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -604,7 +604,7 @@ internal GILState()
604604

605605
protected virtual void Dispose(bool disposing)
606606
{
607-
//ReleeaseLock is thread bound and if it's called in finalizer thread it can wrongly release lock.
607+
//ReleaseLock is thread bound and if it's called in finalizer thread it can wrongly release lock.
608608
if (disposing)
609609
{
610610
PythonEngine.ReleaseLock(state);

‎src/runtime/pythonexception.cs‎

Lines changed: 38 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,11 @@ public PythonException()
2121
_referenceDecrementer = PythonEngine.CurrentRefDecrementer;
2222
IntPtr gs = PythonEngine.AcquireLock();
2323
Runtime.PyErr_Fetch(ref _pyType, ref _pyValue, ref _pyTB);
24-
Runtime.XIncref(_pyType);
25-
Runtime.XIncref(_pyValue);
26-
Runtime.XIncref(_pyTB);
24+
25+
// Those references already owned by the caller
26+
////Runtime.XIncref(_pyType);
27+
////Runtime.XIncref(_pyValue);
28+
////Runtime.XIncref(_pyTB);
2729
if (_pyType != IntPtr.Zero && _pyValue != IntPtr.Zero)
2830
{
2931
string type;
@@ -44,7 +46,13 @@ public PythonException()
4446
}
4547
if (_pyTB != IntPtr.Zero)
4648
{
47-
PyObject tb_module = PythonEngine.ImportModule("traceback");
49+
50+
if (_tbModule == null)
51+
{
52+
_tbModule = PythonEngine.ImportModule("traceback");
53+
}
54+
PyObject tb_module = _tbModule;
55+
4856
Runtime.XIncref(_pyTB);
4957
using (var pyTB = new PyObject(_pyTB))
5058
{
@@ -54,11 +62,14 @@ public PythonException()
5462
PythonEngine.ReleaseLock(gs);
5563
}
5664

65+
private static PyObject _tbModule;
66+
5767
// Ensure that encapsulated Python objects are decref'ed appropriately
5868
// when the managed exception wrapper is garbage-collected.
5969

6070
~PythonException()
6171
{
72+
6273
Dispose(false);
6374
}
6475

@@ -152,49 +163,51 @@ protected void Dispose(bool disposing)
152163
{
153164
disposed = true;
154165

166+
IntPtr pyTypeToDispose = _pyType;
167+
_pyType = IntPtr.Zero;
168+
169+
IntPtr pyValueToDispose = _pyValue;
170+
_pyValue = IntPtr.Zero;
171+
172+
IntPtr pyTBToDispose = _pyTB;
173+
_pyTB = IntPtr.Zero;
174+
155175
if (disposing)
156176
{
157177
if (Runtime.Py_IsInitialized() > 0 && !Runtime.IsFinalizing)
158178
{
179+
IntPtr gs = PythonEngine.AcquireLock();
159180
try
160181
{
161-
IntPtr gs = PythonEngine.AcquireLock();
162-
try
163-
{
164-
Runtime.XDecref(_pyType);
165-
Runtime.XDecref(_pyValue);
166-
// XXX Do we ever get TraceBack? //
167-
if (_pyTB != IntPtr.Zero)
168-
{
169-
Runtime.XDecref(_pyTB);
170-
}
171-
}
172-
finally
182+
Runtime.XDecref(pyTypeToDispose);
183+
Runtime.XDecref(pyValueToDispose);
184+
// XXX Do we ever get TraceBack? //
185+
if (pyTBToDispose != IntPtr.Zero)
173186
{
174-
PythonEngine.ReleaseLock(gs);
187+
Runtime.XDecref(pyTBToDispose);
175188
}
176189
}
177-
catch
190+
finally
178191
{
179-
// Do nothing.
192+
PythonEngine.ReleaseLock(gs);
180193
}
181194
}
182195
}
183196
else
184197
{
185-
if (_pyType != IntPtr.Zero)
198+
if (pyTypeToDispose != IntPtr.Zero)
186199
{
187-
_referenceDecrementer?.ScheduleDecRef(_pyType);
200+
_referenceDecrementer?.ScheduleDecRef(pyTypeToDispose);
188201
}
189202

190-
if (_pyValue != IntPtr.Zero)
203+
if (pyValueToDispose != IntPtr.Zero)
191204
{
192-
_referenceDecrementer?.ScheduleDecRef(_pyValue);
205+
_referenceDecrementer?.ScheduleDecRef(pyValueToDispose);
193206
}
194207

195-
if (_pyTB != IntPtr.Zero)
208+
if (pyTBToDispose != IntPtr.Zero)
196209
{
197-
_referenceDecrementer?.ScheduleDecRef(_pyTB);
210+
_referenceDecrementer?.ScheduleDecRef(pyTBToDispose);
198211
}
199212
}
200213
}

0 commit comments

Comments
 (0)