From ee30c94a5814150fb94d0892f5d909d102dc7b55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Bourbonnais?= Date: Thu, 11 Feb 2021 12:48:37 -0500 Subject: [PATCH 1/6] Fix a ref count issue in PythonException --- src/runtime/pythonexception.cs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/runtime/pythonexception.cs b/src/runtime/pythonexception.cs index 648888293..923dc640c 100644 --- a/src/runtime/pythonexception.cs +++ b/src/runtime/pythonexception.cs @@ -166,7 +166,16 @@ public void Normalize() { if (Exceptions.ErrorOccurred()) throw new InvalidOperationException("Cannot normalize when an error is set"); // 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. + IntPtr oldValue = _pyValue; Runtime.PyErr_NormalizeException(ref _pyType, ref _pyValue, ref _pyTB); + + if (_pyValue != oldValue) + { + // we own the reference to _pyValue, so make adjustments if it changed. + // Disown old, own new + Runtime.XDecref(oldValue); + Runtime.XIncref(_pyValue); + } } finally { From 480a5498dd1b518999a183f9ada06e95a6616ec6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Bourbonnais?= Date: Fri, 26 Feb 2021 13:24:23 -0500 Subject: [PATCH 2/6] 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. --- src/domain_tests/test_domain_reload.py | 1 - src/runtime/classbase.cs | 6 +++ src/runtime/clrobject.cs | 5 ++ src/runtime/exceptions.cs | 41 +++++++++++--- src/runtime/importhook.cs | 1 + src/runtime/pythonexception.cs | 9 ---- src/testing/exceptiontest.cs | 33 ++++++++++++ tests/test_exceptions.py | 74 ++++++++++++++++++++++++++ 8 files changed, 153 insertions(+), 17 deletions(-) diff --git a/src/domain_tests/test_domain_reload.py b/src/domain_tests/test_domain_reload.py index a7cd2fa4d..b54f7c495 100644 --- a/src/domain_tests/test_domain_reload.py +++ b/src/domain_tests/test_domain_reload.py @@ -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") diff --git a/src/runtime/classbase.cs b/src/runtime/classbase.cs index 8b96a96da..60a66829a 100644 --- a/src/runtime/classbase.cs +++ b/src/runtime/classbase.cs @@ -517,5 +517,11 @@ public static int mp_ass_subscript(IntPtr ob, IntPtr idx, IntPtr v) return 0; } + + public override string ToString() + { + return $""; + } + } } diff --git a/src/runtime/clrobject.cs b/src/runtime/clrobject.cs index 0a352b381..0c0c3242d 100644 --- a/src/runtime/clrobject.cs +++ b/src/runtime/clrobject.cs @@ -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 $""; + } } } diff --git a/src/runtime/exceptions.cs b/src/runtime/exceptions.cs index da8653853..9a566128e 100644 --- a/src/runtime/exceptions.cs +++ b/src/runtime/exceptions.cs @@ -85,6 +85,25 @@ 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.Py_CLEAR(ref field); + } + + // 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); + } } /// @@ -181,9 +200,19 @@ internal static void SetArgsAndCause(IntPtr ob) if (e.InnerException != null) { - // Note: For an AggregateException, InnerException is only the first of the InnerExceptions. - IntPtr cause = CLRObject.GetInstHandle(e.InnerException); - Marshal.WriteIntPtr(ob, ExceptionOffset.cause, cause); + if(e.InnerException is PythonException) + { + // If the error is a Python exception, write the real one + var pyerr = (e.InnerException as PythonException); + Runtime.XIncref(pyerr.PyValue); + Runtime.PyException_SetCause(ob, pyerr.PyValue); + } + else + { + // Note: For an AggregateException, InnerException is only the first of the InnerExceptions. + IntPtr cause = CLRObject.GetInstHandle(e.InnerException); + Runtime.PyException_SetCause(ob, cause); + } } } @@ -279,10 +308,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; } @@ -399,6 +425,7 @@ internal static IntPtr RaiseTypeError(string message) if (previousException != null) { SetCause(previousException); + previousException.Dispose(); } return IntPtr.Zero; } diff --git a/src/runtime/importhook.cs b/src/runtime/importhook.cs index 13b45df51..2fd34a901 100644 --- a/src/runtime/importhook.cs +++ b/src/runtime/importhook.cs @@ -368,6 +368,7 @@ public static IntPtr __import__(IntPtr self, IntPtr argsRaw, IntPtr kw) } Runtime.XIncref(mod.pyHandle); + originalException.Dispose(); return mod.pyHandle; } } diff --git a/src/runtime/pythonexception.cs b/src/runtime/pythonexception.cs index 923dc640c..648888293 100644 --- a/src/runtime/pythonexception.cs +++ b/src/runtime/pythonexception.cs @@ -166,16 +166,7 @@ public void Normalize() { if (Exceptions.ErrorOccurred()) throw new InvalidOperationException("Cannot normalize when an error is set"); // 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. - IntPtr oldValue = _pyValue; Runtime.PyErr_NormalizeException(ref _pyType, ref _pyValue, ref _pyTB); - - if (_pyValue != oldValue) - { - // we own the reference to _pyValue, so make adjustments if it changed. - // Disown old, own new - Runtime.XDecref(oldValue); - Runtime.XIncref(_pyValue); - } } finally { diff --git a/src/testing/exceptiontest.cs b/src/testing/exceptiontest.cs index 45acaa2cc..ce82f8817 100644 --- a/src/testing/exceptiontest.cs +++ b/src/testing/exceptiontest.cs @@ -1,3 +1,4 @@ +using Python.Runtime; using System; using System.Collections; using System.Collections.Generic; @@ -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(); + } + } + } diff --git a/tests/test_exceptions.py b/tests/test_exceptions.py index 7fafeebcb..6b468d752 100644 --- a/tests/test_exceptions.py +++ b/tests/test_exceptions.py @@ -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') @@ -375,3 +422,30 @@ 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)} + assert not exc - orig_exc + + 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') From 5ea9c3ca63ecbaf8a6c39142a4d0fb73b006918c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Bourbonnais?= Date: Thu, 4 Mar 2021 10:50:01 -0500 Subject: [PATCH 3/6] Code review fixes --- src/runtime/clrobject.cs | 2 +- src/runtime/exceptions.cs | 19 +++++-------------- tests/test_exceptions.py | 3 ++- 3 files changed, 8 insertions(+), 16 deletions(-) diff --git a/src/runtime/clrobject.cs b/src/runtime/clrobject.cs index 0c0c3242d..1a1a6c987 100644 --- a/src/runtime/clrobject.cs +++ b/src/runtime/clrobject.cs @@ -106,7 +106,7 @@ protected override void OnLoad(InterDomainContext context) public override string ToString() { - return $""; + return $""; } } } diff --git a/src/runtime/exceptions.cs b/src/runtime/exceptions.cs index 9a566128e..7519d85b4 100644 --- a/src/runtime/exceptions.cs +++ b/src/runtime/exceptions.cs @@ -89,7 +89,8 @@ internal static Exception ToException(IntPtr ob) static void ClearOffsetHelper(IntPtr ob, int offset) { var field = Marshal.ReadIntPtr(ob, offset); - Runtime.Py_CLEAR(ref field); + Runtime.XDecref(field); + Marshal.WriteIntPtr(ob, offset, IntPtr.Zero); } // As seen in exceptions.c, every derived type must also clean the base. @@ -200,19 +201,9 @@ internal static void SetArgsAndCause(IntPtr ob) if (e.InnerException != null) { - if(e.InnerException is PythonException) - { - // If the error is a Python exception, write the real one - var pyerr = (e.InnerException as PythonException); - Runtime.XIncref(pyerr.PyValue); - Runtime.PyException_SetCause(ob, pyerr.PyValue); - } - else - { - // Note: For an AggregateException, InnerException is only the first of the InnerExceptions. - IntPtr cause = CLRObject.GetInstHandle(e.InnerException); - Runtime.PyException_SetCause(ob, cause); - } + // Note: For an AggregateException, InnerException is only the first of the InnerExceptions. + IntPtr cause = CLRObject.GetInstHandle(e.InnerException); + Runtime.PyException_SetCause(ob, cause); } } diff --git a/tests/test_exceptions.py b/tests/test_exceptions.py index 6b468d752..066facbf8 100644 --- a/tests/test_exceptions.py +++ b/tests/test_exceptions.py @@ -429,7 +429,8 @@ def do_test_leak(): 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)} - assert not exc - orig_exc + possibly_leaked = exc - orig_exc + assert not possibly_leaked return do_test_leak From 891ad8c93afba36499b777b5140a388cb70e9d6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Bourbonnais?= Date: Fri, 5 Mar 2021 13:43:24 -0500 Subject: [PATCH 4/6] code review fixes --- src/runtime/exceptions.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/runtime/exceptions.cs b/src/runtime/exceptions.cs index 7519d85b4..3269b3883 100644 --- a/src/runtime/exceptions.cs +++ b/src/runtime/exceptions.cs @@ -89,8 +89,11 @@ internal static Exception ToException(IntPtr ob) static void ClearOffsetHelper(IntPtr ob, int offset) { var field = Marshal.ReadIntPtr(ob, offset); - Runtime.XDecref(field); - Marshal.WriteIntPtr(ob, offset, IntPtr.Zero); + if (field != IntPtr.Zero) + { + Marshal.WriteIntPtr(ob, offset, IntPtr.Zero); + Runtime.XDecref(field); + } } // As seen in exceptions.c, every derived type must also clean the base. From d180f2a7977f1a9e4edc1a17defdd06bc283c16d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Bourbonnais?= Date: Mon, 8 Mar 2021 11:31:17 -0500 Subject: [PATCH 5/6] kick the build From 47290ee7e3f84f265e7577eda5719e5cd613cc3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Bourbonnais?= Date: Mon, 8 Mar 2021 11:52:07 -0500 Subject: [PATCH 6/6] kick the build, again