Skip to content

Fix exception refcnt #1402

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/domain_tests/test_domain_reload.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ def test_property_visibility_change():
def test_class_visibility_change():
_run_test("class_visibility_change")

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

Expand Down
6 changes: 6 additions & 0 deletions src/runtime/classbase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -517,5 +517,11 @@ public static int mp_ass_subscript(IntPtr ob, IntPtr idx, IntPtr v)

return 0;
}

public override string ToString()
{
return $"<ClassBase of {type}>";
}

}
}
5 changes: 5 additions & 0 deletions src/runtime/clrobject.cs
Original file line number Diff line number Diff line change
Expand Up @@ -103,5 +103,10 @@ protected override void OnLoad(InterDomainContext context)
GCHandle gc = AllocGCHandle(TrackTypes.Wrapper);
Marshal.WriteIntPtr(pyHandle, ObjectOffset.magic(tpHandle), (IntPtr)gc);
}

public override string ToString()
{
return $"<CLRObject: {inst}>";
}
}
}
28 changes: 23 additions & 5 deletions src/runtime/exceptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,26 @@ internal static Exception ToException(IntPtr ob)
}
return Runtime.PyUnicode_FromString(message);
}

static void ClearOffsetHelper(IntPtr ob, int offset)
{
var field = Marshal.ReadIntPtr(ob, offset);
Runtime.XDecref(field);
Marshal.WriteIntPtr(ob, offset, IntPtr.Zero);
}

// As seen in exceptions.c, every derived type must also clean the base.
// TODO: once the call tp_traverse and tp_clear on the instance and not
// the type, implement those.
public new static void tp_dealloc(IntPtr self)
{
ClearOffsetHelper(self, ExceptionOffset.dict);
ClearOffsetHelper(self, ExceptionOffset.args);
ClearOffsetHelper(self, ExceptionOffset.traceback);
ClearOffsetHelper(self, ExceptionOffset.context);
ClearOffsetHelper(self, ExceptionOffset.cause);
ClassBase.tp_dealloc(self);
}
}

/// <summary>
Expand Down Expand Up @@ -183,7 +203,7 @@ internal static void SetArgsAndCause(IntPtr ob)
{
// Note: For an AggregateException, InnerException is only the first of the InnerExceptions.
IntPtr cause = CLRObject.GetInstHandle(e.InnerException);
Marshal.WriteIntPtr(ob, ExceptionOffset.cause, cause);
Runtime.PyException_SetCause(ob, cause);
}
}

Expand Down Expand Up @@ -279,10 +299,7 @@ public static void SetError(Exception e)
var pe = e as PythonException;
if (pe != null)
{
Runtime.XIncref(pe.PyType);
Runtime.XIncref(pe.PyValue);
Runtime.XIncref(pe.PyTB);
Runtime.PyErr_Restore(pe.PyType, pe.PyValue, pe.PyTB);
pe.Restore();
return;
}

Expand Down Expand Up @@ -399,6 +416,7 @@ internal static IntPtr RaiseTypeError(string message)
if (previousException != null)
{
SetCause(previousException);
previousException.Dispose();
}
return IntPtr.Zero;
}
Expand Down
1 change: 1 addition & 0 deletions src/runtime/importhook.cs
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,7 @@ public static IntPtr __import__(IntPtr self, IntPtr argsRaw, IntPtr kw)
}

Runtime.XIncref(mod.pyHandle);
originalException.Dispose();
return mod.pyHandle;
}
}
Expand Down
33 changes: 33 additions & 0 deletions src/testing/exceptiontest.cs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using Python.Runtime;
using System;
using System.Collections;
using System.Collections.Generic;
Expand Down Expand Up @@ -81,6 +82,38 @@ public static void ThrowChainedExceptions()
throw new Exception("Outer exception", exc2);
}
}
public static IntPtr DoThrowSimple()
{
using (Py.GIL())
{
// Set a python TypeError.
Exceptions.SetError(Exceptions.TypeError, "type error, The first.");
var pyerr = new PythonException();
// PyErr_Fetch() it and set it as the cause of an ArgumentException (and raise).
throw new ArgumentException("Bogus bad parameter", pyerr);
}
}

public static void DoThrowWithInner()
{
using(Py.GIL())
{
// Set a python TypeError.
Exceptions.SetError(Exceptions.TypeError, "type error, The first.");
var pyerr = new PythonException();
// PyErr_Fetch() it and set it as the cause of an ArgumentException (and raise).
Exceptions.SetError(new ArgumentException("Bogus bad parameter", pyerr));
// But we want Python error.. raise a TypeError and set the cause to the
// previous ArgumentError.
PythonException previousException = new PythonException();
Exceptions.SetError(Exceptions.TypeError, "type error, The second.");
Exceptions.SetCause(previousException);
previousException.Dispose();
// Then throw-raise the TypeError.
throw new PythonException();
}
}

}


Expand Down
75 changes: 75 additions & 0 deletions tests/test_exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,53 @@
import pickle


# begin code from https://utcc.utoronto.ca/~cks/space/blog/python/GetAllObjects
import gc
# Recursively expand slist's objects
# into olist, using seen to track
# already processed objects.

def _getr(slist, olist, seen):
for e in slist:
if id(e) in seen:
continue
seen[id(e)] = None
olist.append(e)
tl = gc.get_referents(e)
if tl:
_getr(tl, olist, seen)

# The public function.
def get_all_objects():
gcl = gc.get_objects()
olist = []
seen = {}
# Just in case:
seen[id(gcl)] = None
seen[id(olist)] = None
seen[id(seen)] = None
# _getr does the real work.
_getr(gcl, olist, seen)
return olist
# end code from https://utcc.utoronto.ca/~cks/space/blog/python/GetAllObjects

def leak_check(func):
def do_leak_check():
func()
gc.collect()
exc = {x for x in get_all_objects() if isinstance(x, Exception) and not isinstance(x, pytest.PytestDeprecationWarning)}
print(len(exc))
if len(exc):
for x in exc:
print('-------')
print(repr(x))
print(gc.get_referrers(x))
print(len(gc.get_referrers(x)))
assert False
gc.collect()
return do_leak_check


def test_unified_exception_semantics():
"""Test unified exception semantics."""
e = System.Exception('Something bad happened')
Expand Down Expand Up @@ -375,3 +422,31 @@ def test_iteration_innerexception():
# after exception is thrown iterator is no longer valid
with pytest.raises(StopIteration):
next(val)

def leak_test(func):
def do_test_leak():
# PyTest leaks things, gather the current state
orig_exc = {x for x in get_all_objects() if isinstance(x, Exception)}
func()
exc = {x for x in get_all_objects() if isinstance(x, Exception)}
possibly_leaked = exc - orig_exc
assert not possibly_leaked

return do_test_leak

@leak_test
def test_dont_leak_exceptions_simple():
from Python.Test import ExceptionTest

try:
ExceptionTest.DoThrowSimple()
except System.ArgumentException:
print('type error, as expected')

@leak_test
def test_dont_leak_exceptions_inner():
from Python.Test import ExceptionTest
try:
ExceptionTest.DoThrowWithInner()
except TypeError:
print('type error, as expected')