diff --git a/src/embed_tests/QCTest.cs b/src/embed_tests/QCTest.cs index ea90f96ab..b2d22b8dc 100644 --- a/src/embed_tests/QCTest.cs +++ b/src/embed_tests/QCTest.cs @@ -17,6 +17,16 @@ class QCTests private static dynamic withArgs_PythonSuperInitInt; private static dynamic pureCSharpConstruction; + private static dynamic pureCSharpDefaultConstruction; + private static dynamic pureCSharpOptionalArgsConstruction; + private static dynamic pureCSharpOptionalArgsDefaultConstruction; + private static dynamic pythonSuperInitDefaultAfterOtherConstruction; + private static dynamic reinitAfterConstruction; + private static dynamic pythonOptionalArgsSuperInitDefault; + private static dynamic pythonOptionalArgsSuperInitValue; + private static dynamic namespacedSuperInitNone; + private static dynamic namespacedSuperInitDefault; + private static dynamic pythonSelectedOverloadInit; private static dynamic containsTest; private static dynamic module; @@ -25,6 +35,7 @@ from clr import AddReference AddReference(""System"") AddReference(""Python.EmbeddingTest"") from Python.EmbeddingTest import * +import System class PythonModule(Algo): def TestA(self): try: @@ -68,6 +79,45 @@ def jose(self): def PureCSharpConstruction(): return SuperInit(1) + +def PureCSharpDefaultConstruction(): + return SuperInit() + +def PureCSharpOptionalArgsConstruction(): + return OptionalArgsInit(5) + +def PureCSharpOptionalArgsDefaultConstruction(): + return OptionalArgsInit() + +class PythonSuperInitDefaultAfterOtherConstruction(SuperInit): + def __init__(self): + self.other = SuperInit() + super().__init__() + +class PythonOptionalArgsSuperInitDefault(OptionalArgsInit): + def __init__(self): + super().__init__() + +class PythonOptionalArgsSuperInitValue(OptionalArgsInit): + def __init__(self): + super().__init__(5) + +class PythonSelectedOverloadInit(SelectedOverloadInit): + def __init__(self): + super().__init__.__overloads__[System.String]() + +class NamespacedSuperInitNone(SuperInit): + __namespace__ = 'Python.EmbeddingTest.QCTests' + +class NamespacedSuperInitDefault(SuperInit): + __namespace__ = 'Python.EmbeddingTest.QCTests' + def __init__(self): + super().__init__() + +def ReinitAfterConstruction(): + instance = PythonSuperInitNone() + instance.__init__() + return instance "; [OneTimeSetUp] @@ -88,6 +138,16 @@ public void Setup() withArgs_PythonSuperInitInt = pyModule.GetAttr("WithArgs_PythonSuperInitInt"); pureCSharpConstruction = pyModule.GetAttr("PureCSharpConstruction"); + pureCSharpDefaultConstruction = pyModule.GetAttr("PureCSharpDefaultConstruction"); + pureCSharpOptionalArgsConstruction = pyModule.GetAttr("PureCSharpOptionalArgsConstruction"); + pureCSharpOptionalArgsDefaultConstruction = pyModule.GetAttr("PureCSharpOptionalArgsDefaultConstruction"); + pythonSuperInitDefaultAfterOtherConstruction = pyModule.GetAttr("PythonSuperInitDefaultAfterOtherConstruction"); + reinitAfterConstruction = pyModule.GetAttr("ReinitAfterConstruction"); + pythonOptionalArgsSuperInitDefault = pyModule.GetAttr("PythonOptionalArgsSuperInitDefault"); + pythonOptionalArgsSuperInitValue = pyModule.GetAttr("PythonOptionalArgsSuperInitValue"); + namespacedSuperInitNone = pyModule.GetAttr("NamespacedSuperInitNone"); + namespacedSuperInitDefault = pyModule.GetAttr("NamespacedSuperInitDefault"); + pythonSelectedOverloadInit = pyModule.GetAttr("PythonSelectedOverloadInit"); } [OneTimeTearDown] @@ -127,7 +187,119 @@ public void PureCSharpConstruction() { var instance = pureCSharpConstruction(); Assert.AreEqual(1, (int)instance.CalledInt); + Assert.AreEqual(0, (int)instance.CalledDefault); + } + } + + [Test] + public void PureCSharpDefaultConstruction() + { + using (Py.GIL()) + { + var instance = pureCSharpDefaultConstruction(); + Assert.AreEqual(0, (int)instance.CalledInt); + Assert.AreEqual(1, (int)instance.CalledDefault); + } + } + + [Test] + public void PureCSharpOptionalArgsConstruction() + { + using (Py.GIL()) + { + var instance = pureCSharpOptionalArgsConstruction(); + Assert.AreEqual(1, (int)instance.Calls); + Assert.AreEqual(5, (int)instance.Value); + } + } + + [Test] + public void PureCSharpOptionalArgsDefaultConstruction() + { + using (Py.GIL()) + { + var instance = pureCSharpOptionalArgsDefaultConstruction(); + Assert.AreEqual(1, (int)instance.Calls); + Assert.AreEqual(0, (int)instance.Value); + } + } + + [Test] + public void NoArgs_DefaultConstructorAfterOtherConstruction() + { + using (Py.GIL()) + { + var instance = pythonSuperInitDefaultAfterOtherConstruction(); + Assert.AreEqual(0, (int)instance.CalledInt); Assert.AreEqual(1, (int)instance.CalledDefault); + Assert.AreEqual(1, (int)instance.other.CalledDefault); + } + } + + [Test] + public void OptionalArgs_DefaultConstructor() + { + using (Py.GIL()) + { + var instance = pythonOptionalArgsSuperInitDefault(); + Assert.AreEqual(1, (int)instance.Calls); + Assert.AreEqual(0, (int)instance.Value); + } + } + + [Test] + public void OptionalArgs_ValueConstructor() + { + using (Py.GIL()) + { + var instance = pythonOptionalArgsSuperInitValue(); + // the base constructor runs in tp_new before __init__, then again with the given value + Assert.AreEqual(2, (int)instance.Calls); + Assert.AreEqual(5, (int)instance.Value); + } + } + + [Test] + public void SelectedOverload_OptionalArgsConstructor() + { + using (Py.GIL()) + { + var instance = pythonSelectedOverloadInit(); + // tp_new ran the parameterless constructor; the explicitly selected one must still run + Assert.AreEqual("optional", (string)instance.Selected); + } + } + + [Test] + public void Namespaced_NoInit() + { + using (Py.GIL()) + { + var instance = namespacedSuperInitNone(); + Assert.AreEqual(0, (int)instance.CalledInt); + Assert.AreEqual(1, (int)instance.CalledDefault); + } + } + + [Test] + public void Namespaced_DefaultConstructor() + { + using (Py.GIL()) + { + var instance = namespacedSuperInitDefault(); + Assert.AreEqual(0, (int)instance.CalledInt); + Assert.AreEqual(1, (int)instance.CalledDefault); + } + } + + [Test] + public void ReinitAfterConstructionRunsConstructorAgain() + { + using (Py.GIL()) + { + var instance = reinitAfterConstruction(); + Assert.AreEqual(0, (int)instance.CalledInt); + Assert.AreEqual(2, (int)instance.CalledDefault); } } @@ -161,7 +333,7 @@ public void WithArgs_DefaultConstructor() { var instance = withArgs_PythonSuperInitDefault(1); Assert.AreEqual(0, (int)instance.CalledInt); - Assert.AreEqual(2, (int)instance.CalledDefault); + Assert.AreEqual(1, (int)instance.CalledDefault); } } @@ -196,7 +368,7 @@ public void NoArgs_DefaultConstructor() { var instance = pythonSuperInitNone(); Assert.AreEqual(0, (int)instance.CalledInt); - Assert.AreEqual(2, (int)instance.CalledDefault); + Assert.AreEqual(1, (int)instance.CalledDefault); } } @@ -208,7 +380,7 @@ public void NoArgs_NoConstructor() var instance = pythonSuperInitDefault.Invoke(); Assert.AreEqual(0, (int)instance.CalledInt); - Assert.AreEqual(2, (int)instance.CalledDefault); + Assert.AreEqual(1, (int)instance.CalledDefault); } } } @@ -246,6 +418,30 @@ public SuperInit() } } + public class OptionalArgsInit + { + public int Calls { get; private set; } + public int Value { get; private set; } + public OptionalArgsInit(int value = 0) + { + Calls++; + Value = value; + } + } + + public class SelectedOverloadInit + { + public string Selected { get; private set; } + public SelectedOverloadInit() + { + Selected = "default"; + } + public SelectedOverloadInit(string value = "optional") + { + Selected = value; + } + } + public class Insight { public string info; diff --git a/src/runtime/MethodBinder.cs b/src/runtime/MethodBinder.cs index 5ec3edc66..dc6a366d4 100644 --- a/src/runtime/MethodBinder.cs +++ b/src/runtime/MethodBinder.cs @@ -1107,6 +1107,37 @@ internal virtual NewReference Invoke(BorrowedReference inst, BorrowedReference a return default; } + if (binding.info is ConstructorInfo && ManagedType.GetManagedObject(inst) is CLRObject co) + { + if (co.awaitingCtor) + { + // tp_new suppressed finalization until a constructor runs; register it before the + // constructor body, as for a .NET 'new', so the object is finalized even if it throws. + co.awaitingCtor = false; + GC.ReRegisterForFinalize(co.inst); + } + else if (co.ctorRanInNew != null) + { + var ranInNew = co.ctorRanInNew; + co.ctorRanInNew = null; + // A zero-argument base __init__() call that binds the constructor tp_new already ran on this + // Python subclass instance must not run it twice. A __namespace__ subclass's generated type + // mirrors each base constructor and chains to it, so its parameterless constructor has + // already run the base type's parameterless one. + var ranInNewAlready = binding.info == ranInNew + || co.inst is IPythonDerivedType + && binding.info.DeclaringType == ranInNew.DeclaringType.BaseType + && binding.args.Length == 0 + && ranInNew.GetParameters().Length == 0; + if (ranInNewAlready + && Runtime.PyTuple_Size(args) == 0 + && (kw == null || Runtime.PyDict_Size(kw) == 0)) + { + return new NewReference(Runtime.PyNone); + } + } + } + if (allow_threads) { ts = PythonEngine.BeginAllowThreads(); diff --git a/src/runtime/Types/ClassBase.cs b/src/runtime/Types/ClassBase.cs index 3d08f8ca6..19d8abf32 100644 --- a/src/runtime/Types/ClassBase.cs +++ b/src/runtime/Types/ClassBase.cs @@ -711,7 +711,14 @@ public override bool Init(BorrowedReference obj, BorrowedReference args, Borrowe // initialization must be done in tp_new return true; - return base.Init(obj, args, kw); + var initialized = base.Init(obj, args, kw); + // tp_new's marks only apply to this construction's __init__ + if (GetManagedObject(obj) is CLRObject co) + { + co.ctorRanInNew = null; + co.awaitingCtor = false; + } + return initialized; } protected virtual void OnDeserialization(object sender) diff --git a/src/runtime/Types/ClassObject.cs b/src/runtime/Types/ClassObject.cs index b57378a32..2aa8ad7aa 100644 --- a/src/runtime/Types/ClassObject.cs +++ b/src/runtime/Types/ClassObject.cs @@ -113,6 +113,21 @@ static NewReference tp_new_impl(BorrowedReference tp, BorrowedReference args, Bo object obj = FormatterServices.GetUninitializedObject(type); var pythonObj = self.NewObjectToPython(obj, tp); + // Constructing the reflected type itself always continues with __init__ (MetaType.tp_call), + // which binds and runs the matching constructor: running one here too would run a constructor + // twice on the same object. A Python subclass's __init__ may never call the base __init__, so + // it still gets the base constructor here, before its own __init__ runs; that includes a subclass + // declaring __namespace__, whose generated .NET type is registered as its own reflected type. + if (tp == ClassManager.GetClass(type).Reference && self is not ClassDerivedObject) + { + GC.SuppressFinalize(obj); + if (GetManagedObject(pythonObj.Borrow()) is CLRObject newObject) + { + newObject.awaitingCtor = true; + } + return pythonObj; + } + try { var binder = new MethodBinder(); @@ -126,6 +141,10 @@ static NewReference tp_new_impl(BorrowedReference tp, BorrowedReference args, Bo if (binding != null) { binding.info.Invoke(obj, BindingFlags.Default, null, binding.args, null); + if (GetManagedObject(pythonObj.Borrow()) is CLRObject co) + { + co.ctorRanInNew = binding.info; + } } } catch (Exception) diff --git a/src/runtime/Types/ClrObject.cs b/src/runtime/Types/ClrObject.cs index a45080bf7..98ceba8f4 100644 --- a/src/runtime/Types/ClrObject.cs +++ b/src/runtime/Types/ClrObject.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Diagnostics; +using System.Reflection; using System.Runtime.InteropServices; namespace Python.Runtime @@ -10,6 +11,11 @@ namespace Python.Runtime internal sealed class CLRObject : ManagedType { internal readonly object inst; + // The zero-argument constructor tp_new ran on this object before __init__, if any. + internal MethodBase ctorRanInNew; + // Set when tp_new returned this object without running a constructor: its finalization stays + // suppressed until one runs, so a failed __init__ doesn't leave it to its finalizer uninitialized. + internal bool awaitingCtor; static NewReference Create(object ob, BorrowedReference tp) { diff --git a/src/testing/constructortests.cs b/src/testing/constructortests.cs index 732692b3a..5912853b0 100644 --- a/src/testing/constructortests.cs +++ b/src/testing/constructortests.cs @@ -1,5 +1,7 @@ using System; +using System.Collections.Generic; using System.IO; +using System.Threading; namespace Python.Test { @@ -83,4 +85,27 @@ public class DefaultConstructorMatching public DefaultConstructorMatching() { a = 1; } public DefaultConstructorMatching(double a) { this.a = a; } } + + public class FinalizedConstructorTest + { + public static int Finalized; + + private readonly List items = new List(); + + public FinalizedConstructorTest() + { + } + + public FinalizedConstructorTest(Exception toThrow) + { + throw toThrow; + } + + ~FinalizedConstructorTest() + { + // throws, terminating the process, if no constructor ever ran on this instance + items.Clear(); + Interlocked.Increment(ref Finalized); + } + } } diff --git a/tests/test_constructors.py b/tests/test_constructors.py index 51822d36a..ba9e7cc0d 100644 --- a/tests/test_constructors.py +++ b/tests/test_constructors.py @@ -2,6 +2,8 @@ """Test CLR class constructor support.""" +import os +import subprocess import pytest import sys @@ -104,3 +106,51 @@ def test_string_constructor(): with pytest.raises(TypeError): String(arr, 5, 4) + + +_FAILED_CONSTRUCTION_SCRIPT = """ +import os, sys +bin_path = sys.argv[1] +sys.path.append(bin_path) +from clr_loader import get_coreclr +from pythonnet import set_runtime +set_runtime(get_coreclr(runtime_config=os.path.join(bin_path, "Python.Test.runtimeconfig.json"))) +import clr +clr.AddReference("Python.Test") +import gc +from Python.Test import FinalizedConstructorTest +from System import GC, InvalidOperationException + +def construct(): + try: + FinalizedConstructorTest("unsupported") + except TypeError: + print("caught") + FinalizedConstructorTest() + try: + FinalizedConstructorTest(InvalidOperationException("thrown")) + except InvalidOperationException: + print("thrown") + +construct() +gc.collect() +for _ in range(2): + GC.Collect() + GC.WaitForPendingFinalizers() +print(FinalizedConstructorTest.Finalized) +""" + + +@pytest.mark.skipif(os.environ.get("PYTHONNET_RUNTIME") != "netcore", reason="loads the netcore runtime") +def test_construction_finalization(): + """A failed T(...) must not leave an object whose constructor never ran + to its finalizer, which would terminate the process, while a constructed + object, or one whose constructor threw, is finalized once, as with new""" + from Python.Test import FinalizedConstructorTest + + bin_path = os.path.dirname(FinalizedConstructorTest().GetType().Assembly.Location) + result = subprocess.run([sys.executable, "-c", _FAILED_CONSTRUCTION_SCRIPT, bin_path], + capture_output=True, text=True, timeout=300) + + assert result.returncode == 0, result.stderr + assert result.stdout.splitlines()[-3:] == ["caught", "thrown", "2"]