10000 Fixes a leak in ExceptionClassObject · Unity-Technologies/pythonnet@480a549 · GitHub
[go: up one dir, main page]
Skip to content

Commit 480a549

Browse files
BadSingletonBadSingleton
committed
Fixes a leak in ExceptionClassObject
As seen in exceptions.c (from cpython), derived exception classes must also clean the ExceptionBase members. If we don't, we leak the Python and C# objects set as the __cause__/Inner (amongst others) of thrown C# exceptions. This does not address the serializability of the PythonException class. Also fix various PythonException (ab)uses.
1 parent ee30c94 commit 480a549

File tree

8 files changed

+153
-17
lines changed

8 files changed

+153
-17
lines changed

‎src/domain_tests/test_domain_reload.py

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,6 @@ def test_property_visibility_change():
5656
def test_class_visibility_change():
5757
_run_test("class_visibility_change")
5858

59-
@pytest.mark.skip(reason='FIXME: Domain reload fails when Python points to a .NET object which points back to Python objects')
6059
def test_method_parameters_change():
6160
_run_test("method_parameters_change")
6261

‎src/runtime/classbase.cs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -517,5 +517,11 @@ public static int mp_ass_subscript(IntPtr ob, IntPtr idx, IntPtr v)
517517

518518
return 0;
519519
}
520+
521+
public override string ToString()
522+
{
523+
return $"<ClassBase of {type}>";
524+
}
525+
520526
}
521527
}

‎src/runtime/clrobject.cs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,5 +103,10 @@ protected override void OnLoad(InterDomainContext context)
103103
GCHandle gc = AllocGCHandle(TrackTypes.Wrapper);
104104
Marshal.WriteIntPtr(pyHandle, ObjectOffset.magic(tpHandle), (IntPtr)gc);
105105
}
106+
107+
public override string ToString()
108+
{
109+
return $"<CLRObject wrapping {inst}>";
110+
}
106111
}
107112
}

‎src/runtime/exceptions.cs

Lines changed: 34 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,25 @@ internal static Exception ToException(IntPtr ob)
8585
}
8686
return Runtime.PyUnicode_FromString(message);
8787
}
88+
89+
static void ClearOffsetHelper(IntPtr ob, int offset)
90+
{
91+
var field = Marshal.ReadIntPtr(ob, offset);
92+
Runtime.Py_CLEAR(ref field);
93+
}
94+
95+
// As seen in exceptions.c, every derived type must also clean the base.
96+
// TODO: once the call tp_traverse and tp_clear on the instance and not
97+
// the type, implement those.
98+
public new static void tp_dealloc(IntPtr self)
99+
{
100+
ClearOffsetHelper(self, ExceptionOffset.dict);
101+
ClearOffsetHelper(self, ExceptionOffset.args);
102+
ClearOffsetHelper(self, ExceptionOffset.traceback);
103+
ClearOffsetHelper(self, ExceptionOffset.context);
104+
ClearOffsetHelper(self, ExceptionOffset.cause);
105+
ClassBase.tp_dealloc(self);
106+
}
88107
}
89108

90109
/// <summary>
@@ -181,9 +200,19 @@ internal static void SetArgsAndCause(IntPtr ob)
181200

182201
if (e.InnerException != null)
183202
{
184-
// Note: For an AggregateException, InnerException is only the first of the InnerExceptions.
185-
IntPtr cause = CLRObject.GetInstHandle(e.InnerException);
186-
Marshal.WriteIntPtr(ob, ExceptionOffset.cause, cause);
203+
if(e.InnerException is PythonException)
204+
{
205+
// If the error is a Python exception, write the real one
206+
var pyerr = (e.InnerException as PythonException);
207+
Runtime.XIncref(pyerr.PyValue);
208+
Runtime.PyException_SetCause(ob, pyerr.PyValue);
209+
}
210+
else
211+
{
212+
// Note: For an AggregateException, InnerException is only the first of the InnerExceptions.
213+
IntPtr cause = CLRObject.GetInstHandle(e.InnerException);
214+
Runtime.PyException_SetCause(ob, cause);
215+
}
187216
}
188217
}
189218

@@ -279,10 +308,7 @@ public static void SetError(Exception e)
279308
var pe = e as PythonException;
280309
if (pe != null)
281310
{
282-
Runtime.XIncref(pe.PyType);
283-
Runtime.XIncref(pe.PyValue);
284-
Runtime.XIncref(pe.PyTB);
285-
Runtime.PyErr_Restore(pe.PyType, pe.PyValue, pe.PyTB);
311+
pe.Restore();
286312
return;
287313
}
288314

@@ -399,6 +425,7 @@ internal static IntPtr RaiseTypeError(string message)
399425
if (previousException != null)
400426
{
401427
SetCause(previousException);
428+
previousException.Dispose();
402429
}
403430
return IntPtr.Zero;
404431
}

‎src/runtime/importhook.cs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -368,6 +368,7 @@ public static IntPtr __import__(IntPtr self, IntPtr argsRaw, IntPtr kw)
368368
}
369369

370370
Runtime.XIncref(mod.pyHandle);
371+
originalException.Dispose();
371372
return < F438 span class=pl-s1>mod.pyHandle;
372373
}
373374
}

‎src/runtime/pythonexception.cs

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -166,16 +166,7 @@ public void Normalize()
166166
{
167167
if (Exceptions.ErrorOccurred()) throw new InvalidOperationException("Cannot normalize when an error is set");
168168
// If an error is set and this PythonException is unnormalized, the error will be cleared and the PythonException will be replaced by a different error.
169-
IntPtr oldValue = _pyValue;
170169
Runtime.PyErr_NormalizeException(ref _pyType, ref _pyValue, ref _pyTB);
171-
172-
if (_pyValue != oldValue)
173-
{
174-
// we own the reference to _pyValue, so make adjustments if it changed.
175-
// Disown old, own new
176-
Runtime.XDecref(oldValue);
177-
Runtime.XIncref(_pyValue);
178-
}
179170
}
180171
finally
181172
{

‎src/testing/exceptiontest.cs

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
using Python.Runtime;
12
using System;
23
using System.Collections;
34
using System.Collections.Generic;
@@ -81,6 +82,38 @@ public static void ThrowChainedExceptions()
8182
throw new Exception("Outer exception", exc2);
8283
}
8384
}
85+
public static IntPtr DoThrowSimple()
86+
{
87+
using (Py.GIL())
88+
{
89+
// Set a python TypeError.
90+
Exceptions.SetError(Exceptions.TypeError, "type error, The first.");
91+
var pyerr = new PythonException();
92+
// PyErr_Fetch() it and set it as the cause of an ArgumentException (and raise).
93+
throw new ArgumentException("Bogus bad parameter", pyerr);
94+
}
95+
}
96+
97+
public static void DoThrowWithInner()
98+
{
99+
using(Py.GIL())
100+
{
101+
// Set a python TypeError.
102+
Exceptions.SetError(Exceptions.TypeError, "type error, The first.");
103+
var pyerr = new PythonException();
104+
// PyErr_Fetch() it and set it as the cause of an ArgumentException (and raise).
105+
Exceptions.SetError(new ArgumentException("Bogus bad parameter", pyerr));
106+
// But we want Python error.. raise a TypeError and set the cause to the
107+
// previous ArgumentError.
108+
PythonException previousException = new PythonException();
109+
Exceptions.SetError(Exceptions.TypeError, "type error, The second.");
110+
Exceptions.SetCause(previousException);
111+
previousException.Dispose();
112+
// Then throw-raise the TypeError.
113+
throw new PythonException();
114+
}
115+
}
116+
84117
}
85118

86119

‎tests/test_exceptions.py

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,53 @@
99
import pickle
1010

1111

12+
# begin code from https://utcc.utoronto.ca/~cks/space/blog/python/GetAllObjects
13+
import gc
14+
# Recursively expand slist's objects
15+
# into olist, using seen to track
16+
# already processed objects.
17+
18+
def _getr(slist, olist, seen):
19+
for e in slist:
20+
if id(e) in seen:
21+
continue
22+
seen[id(e)] = None
23+
olist.append(e)
24+
tl = gc.get_referents(e)
25+
if tl:
26+
_getr(tl, olist, seen)
27+
28+
# The public function.
29+
def get_all_objects():
30+
gcl = gc.get_objects()
31+
olist = []
32+
seen = {}
33+
# Just in case:
34+
seen[id(gcl)] = None
35+
seen[id(olist)] = None
36+
seen[id(seen)] = None
37+
# _getr does the real work.
38+
_getr(gcl, olist, seen)
39+
return olist
40+
# end code from https://utcc.utoronto.ca/~cks/space/blog/python/GetAllObjects
41+
42+
def leak_check(func):
43+
def do_leak_check():
44+
func()
45+
gc.collect()
46+
exc = {x for x in get_all_objects() if isinstance(x, Exception) and not isinstance(x, pytest.PytestDeprecationWarning)}
47+
print(len(exc))
48+
if len(exc):
49+
for x in exc:
50+
print('-------')
51+
print(repr(x))
52+
print(gc.get_referrers(x))
53+
print(len(gc.get_referrers(x)))
54+
assert False
55+
gc.collect()
56+
return do_leak_check
57+
58+
1259
def test_unified_exception_semantics():
1360
"""Test unified exception semantics."""
1461
e = System.Exception('Something bad happened')
@@ -375,3 +422,30 @@ def test_iteration_innerexception():
375422
# after exception is thrown iterator is no longer valid
376423
with pytest.raises(StopIteration):
377424
next(val)
425+
426+
def leak_test(func):
427+
def do_test_leak():
428+
# PyTest leaks things, gather the current state
429+
orig_exc = {x for x in get_all_objects() if isinstance(x, Exception)}
430+
func()
431+
exc = {x for x in get_all_objects() if isinstance(x, Exception)}
432+
assert not exc - orig_exc
433+
434+
return do_test_leak
435+
436+
@leak_test
437+
def test_dont_leak_exceptions_simple():
438+
from Python.Test import ExceptionTest
439+
440+
try:
441+
ExceptionTest.DoThrowSimple()
442+
except System.ArgumentException:
443+
print('type error, as expected')
444+
445+
@leak_test
446+
def test_dont_leak_exceptions_inner():
447+
from Python.Test import ExceptionTest
448+
try:
449+
ExceptionTest.DoThrowWithInner()
450+
except TypeError:
451+
print('type error, as expected')

0 commit comments

Comments
 (0)
0