Repository navigation
Run a .NET constructor once when it is instantiated from Python - #166
Conversation
Constructing a reflected class from Python ran a constructor twice on the same object whenever the type had a constructor callable with no arguments: tp_new invoked the zero-argument one, then __init__ bound and invoked the matching constructor again. Field initializers allocated twice and constructor side effects (event subscriptions, registrations, counters) happened twice. - tp_new no longer runs a constructor when the type being constructed is the reflected class itself: MetaType.tp_call always follows with __init__, which runs the matching one. - A Python subclass still gets its base constructor in tp_new, before its own __init__ runs, so an __init__ that never calls the base __init__ keeps working. When that __init__ then calls the base __init__() with no arguments, the binder resolves the constructor tp_new already ran, so it is not run a second time. The mark tp_new leaves on the object only lasts for that construction. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Review
CI is green: Build and Test on Python 3.8 to 3.14, plus the Lean Python unit and regression workflows. The root cause is right, and the approach fixes every case in the description's table. I built the head commit and master and ran the same probe scripts on both (Python 3.11, .NET 10):
| Python code | master | this PR |
|---|---|---|
Counted(v=4) (kwargs) |
2 | 1 |
Python subclass of a Derived : Base type, super().__init__() |
Derived 2, Base 2 |
1, 1 |
two-level Python subclass, each calling super().__init__() |
2 | 1 |
subclass calling Counted.__init__(self) explicitly |
2 | 1 |
subclass of an (int value = 0) type, super().__init__() |
(2, 0) |
(1, 0) |
subclass of an (int value = 0) type, super().__init__(5) |
(2, 5) |
(2, 5) |
__namespace__ subclass, no __init__ |
2 | 1 |
__namespace__ subclass, super().__init__() |
2 | 2 |
System.Exception('m').Message, List[int]() |
ok | ok |
Counted.__new__(Counted) |
constructed, Calls == 1 |
uninitialized: field initializers not run, Calls == 0 |
The last row matches stock pythonnet. copy.copy, copy.deepcopy and pickle of a .NET object raise cannot pickle on both builds, so they don't reach the uninitialized path.
I checked the reason for keeping the base constructor before __init__, and it holds. Lean's PythonIndicator() is : base(""), and CustomSimpleMovingAverage.__init__ in CustomIndicatorWithExtensionAlgorithm.py sets self.name without calling super(). If the base constructor ran after __init__, it would reset that name.
The new checks on the construction path cost a ConstructorInfo type test in MethodBinder.Invoke and a GetManagedObject in ClassBase.Init. Exact-type construction also stops building a MethodBinder in tp_new. Net, construction does less work than before.
Findings
- Needs a decision: the remaining double run for
super().__init__(args). A Python subclass whose__init__calls the base__init__with arguments still runs()+(int, string). Stock pythonnet runs only(int, string), and that is the result this fix is meant to reach. The description's question is the right one: intp_newthere's no way to know which constructor__init__will call. Removing the extra run means moving the base constructor after__init__, which breaks the pattern above. A QuantConnect maintainer needs to accept this remaining case on the PR, or pick the trade-off. Once that's settled, this finding is resolved either way. __namespace__subclasses callingsuper().__init__()still run the constructor twice (inline onMethodBinder.cs). Either cover the case or list it in the description as unchanged.- Test gap (inline on
QCTest.cs): nothing covers the skip for a Python subclass of a type whose only constructor has all-optional parameters.
Risk
I set risk:high. The change affects how every .NET object is constructed from Python: every Lean Python algorithm, custom data type and indicator goes through it. It also changes what T.__new__(T) returns. That blast radius is the high-risk trait, and it stays even with the green Lean workflows.
Other repositories
No other repository needs a code change. Lean picks this up with the next pythonnet package version.
| co.ranZeroArgCtorInNew = false; | ||
| // A zero-argument base __init__() call binds the constructor tp_new already ran on this | ||
| // Python subclass instance (same type's constructors, same zero arguments): don't run it twice. | ||
| if (binding.info.DeclaringType == co.inst.GetType() |
There was a problem hiding this comment.
For a subclass that declares __namespace__, co.inst.GetType() is the generated .NET type. Its super().__init__() binds a constructor declared on the reflected base type, so this comparison fails, the mark is cleared, and the base constructor runs a second time. I measured 2 runs on both master and this branch for:
class A(Counted):
__namespace__ = 'probe.ns'
def __init__(self):
super().__init__()The same subclass without __init__ now runs once. This isn't a regression, but it's the same double run the PR fixes. Please check whether the generated zero-argument constructor already chains to the base one that super().__init__() binds. If it does, comparing against the base type for an IPythonDerivedType instance would cover this case; add a QCTest for it. If not, list it in the description as unchanged.
There was a problem hiding this comment.
Covered in 5757cec. The generated type mirrors each base constructor and its body calls the base one (_Base__cinit__ → call ctor), so the parameterless constructor tp_new runs on the generated type has already run the base parameterless one. The skip now also accepts the base type's parameterless constructor when the instance is an IPythonDerivedType. It requires binding.args.Length == 0 because mirrors don't keep default values, so an all-optional base constructor never gets the mark from tp_new. New Namespaced_DefaultConstructor (1 run; fails without the condition) and Namespaced_NoInit.
A __namespace__ subclass's generated .NET type mirrors each base constructor and chains to it, so the parameterless constructor tp_new runs on it has already run the base type's parameterless one. A zero-argument super().__init__() binds that base constructor, which was then run a second time. Also pin the all-optional-parameter constructor cases for Python subclasses in QCTest. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressed the review in 5757cec:
CI suites on Python 3.11: embed tests passed 1046, skipped 8, failed 0; |
There was a problem hiding this comment.
Review of 5757cec
CI is green on this commit: Build and Test on Python 3.8 to 3.14, plus the Lean Python unit and regression workflows.
What changed since my last review
__namespace__subclasses callingsuper().__init__(): resolved. I checked the claim that the generated type's constructors chain to the base ones.ClassDerived.AddConstructor(src/runtime/Types/ClassDerived.cs:307-359) emits each mirrored constructor as a call toInvokeCtor, which calls_<Base>__cinit__, and that methodcalls the base constructor. So the parameterless constructortp_newruns on the generated type has already run the base one, and skipping the base()inMethodBinder.Invokeis correct. Thebinding.args.Length == 0guard is also correct: mirrored constructors don't keep default values, so an all-optional base constructor never gets the mark.- Optional-argument subclasses: resolved.
OptionalArgs_DefaultConstructorandOptionalArgs_ValueConstructorpin both cases.
I ran the 20 QCTests on the head commit locally, and all pass. I also probed the new __namespace__ path for cases the tests don't cover (Python 3.11, .NET 10):
| Python code | constructor runs |
|---|---|
__namespace__ subclass of (int value = 0), super().__init__() |
1, Value == 0 |
same, super().__init__(5) |
1, Value == 5 |
__namespace__ subclass, then explicit obj.__init__() |
1, then 2 (the re-init still runs, as on master) |
plain Python subclass of a __namespace__ class, with and without __init__ |
1 |
__namespace__ subclass calling Base.__init__(self) explicitly |
1 |
__namespace__ subclass constructing another instance in __init__ before super().__init__() |
1 and 1 |
type with both () and (int a = 7): direct, Python subclass and __namespace__ subclass, each calling () |
() once, (int) never |
None of these regress.
Findings
- Still open, needs a decision: the remaining double run for
super().__init__(args). A Python subclass whose__init__calls the base__init__with arguments still runs()+(int, string). Stock pythonnet runs only(int, string), and that's the result the original report asks for. The only way to get there is to defer the base constructor until after__init__. That breaks subclasses that set managed state in__init__without callingsuper(), which I verified last time against a real indicator pattern. A QuantConnect maintainer needs to accept this remaining case or pick the trade-off on this PR. Once they do, this finding is resolved either way, andOptionalArgs_ValueConstructoralready pins whichever way it goes.
Risk
risk:high stays. The change affects how every .NET object is constructed from Python, including every Python algorithm, custom data type and indicator, and it changes what T.__new__(T) returns. Green Lean workflows and the probes above don't shrink that blast radius.
Other repositories
No other repository needs a code change. Downstream picks this up with the next pythonnet package version.
|
@Martin-Molinero, one decision is needed before this can go further. The rest of the review is resolved, and there are no code changes in this round. A Python subclass whose
|
|
About the decision, does stock Pythonnet has the same limitation as option B? |
|
No. Stock pythonnet has a different limitation: it never runs a base constructor unless I measured it with a .NET class
So the three behave differently:
No code changes in this round. |
|
Go with Option A, the PR as it is with this one only limitation so we do not change current behaviour in existing Lean algorithms |
|
Go for Option A, the PR as it is. |
|
Thanks, going with option A: the PR stays as it is. I replaced the open question in the description with the decision, as a "Known limitation (accepted)" note. It says a subclass calling |
There was a problem hiding this comment.
Review of 6089cc0
CI is green on this commit: Build and Test on Python 3.8 to 3.14, plus the Lean Python unit and regression workflows. The CI logs show that test_failed_construction_does_not_finalize_uninitialized_object runs on every Python version and isn't skipped.
What changed since the last reviews
Both regressions from the second review on 5757cec are fixed.
-
Failed construction and the finalizer: resolved. I built the head and ran probes with a finalizable class that has a field initializer and a finalizer that uses it, each scenario 100 times (Python 3.11, .NET 10). Each probe ran
gc.collect(), thenGC.Collect()andGC.WaitForPendingFinalizers()twice:Python code constructors run finalizers run exit Fin()100 100 0 Fin(i)100 100 0 FinNoDefault(i)(no parameterless constructor)100 100 0 Fin("x"), constructor throws100 100 0 Fin(1.5, 2), no matching overload,TypeErrorcaught0 0 0 Fin.__new__(Fin), theno.__init__(3)100 100 0 Fin.__new__(Fin)only0 0 0 Python subclass calling super().__init__(2)200 (accepted limitation) 100 0 A successful construction is finalized exactly once, not twice. After
SuppressFinalize,ReRegisterForFinalizeonly clears the suppressed bit and doesn't queue a second entry. A constructor that throws is still finalized, as with a .NETnew. -
Explicitly selected overload: resolved. The skip now compares the constructor
tp_newran (binding.info == ranInNew). For a__namespace__subclass it accepts only the base parameterless constructor, and only when the generated constructortp_newran is the parameterless one.SelectedOverload_OptionalArgsConstructorpins it.__init__overloads go throughMethodBinder.Invoketoo, so no other constructor path skips the re-register.
The extra cost per construction is a GetManagedObject and the SuppressFinalize/ReRegisterForFinalize pair. Both calls only flip a bit in the object header, so this isn't a hot-path concern.
Findings
- No test covers the half of the new finalization mechanism that keeps today's behaviour (inline on
MethodBinder.cs). Every object of the reflected type now starts with finalization suppressed.GC.ReRegisterForFinalizein the binder is the only thing that gives it back. If that line is dropped or moved behind a branch that doesn't run, no finalizable .NET object constructed from Python is ever finalized again, and no test fails. The new subprocess test only asserts the failure path: that the finalizer doesn't run. The only fixture with a finalizer isFinalizedConstructorTest, and nothing intests/orsrc/embed_tests/checks that a .NET object constructed from Python still gets finalized. TheTestFinalizertests coverPyObjectwrappers. Please extend the subprocess test, for example with a static counter that the fixture's finalizer increments. It should assert that a successfulFinalizedConstructorTest()is finalized once, and that an instance whose constructor throws is finalized, as the description claims.
Risk
risk:high stays. Every .NET object constructed from Python goes through this path, including every Python algorithm, custom data type and indicator. This commit adds a new way for that path to go wrong: finalization is now suppressed and re-enabled on every construction. The probes above show it behaves correctly today, but finding 1 is what would keep it that way.
Other repositories
No other repository needs a code change. Downstream gets the fix with the next pythonnet package version.
| // 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); |
There was a problem hiding this comment.
This line is now the only thing that makes a .NET object constructed from Python finalizable, because tp_new suppresses finalization for every reflected-type instance. Nothing tests it. If it's removed, every test still passes, including the new subprocess test, which only asserts that the finalizer does not run after a failed bind. I checked the head with a finalizable probe class: Fin() and a throwing Fin("x") are each finalized once, so the code is right. Please pin it: in the subprocess script, construct FinalizedConstructorTest() successfully, and also call a constructor that throws, then assert both were finalized, for example through a static counter the finalizer increments.
There was a problem hiding this comment.
Pinned in 2692e63. FinalizedConstructorTest now has a static Finalized counter that its finalizer increments, and a constructor that throws a given exception. The subprocess test, renamed test_construction_finalization, runs three constructions: a failed bind (TypeError caught), a successful FinalizedConstructorTest(), and the throwing constructor (exception caught). It then forces finalization and asserts the counter is exactly 2. With this GC.ReRegisterForFinalize line commented out, the test fails with 0 != 2.
tp_new suppresses finalization of the reflected type's object until a constructor runs, and the binder re-registers it, so the subprocess test now also asserts that a successfully constructed instance, and one whose constructor throws, are each finalized once. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressed the review of 6089cc0 in 2692e63 (tests only, no runtime change). The subprocess test, now CI suites on Python 3.11 at 2692e63:
The description is updated. |
There was a problem hiding this comment.
Review of 2692e63
CI is green on this commit: Build and Test on Python 3.8 to 3.14, plus the Lean Python unit and regression workflows. The CI logs show test_construction_finalization runs on every Python version. The one skipped test in test_constructors.py is the existing test_constructor_leak.
What changed since my last review
Only tests changed: src/testing/constructortests.cs and tests/test_constructors.py. The runtime is the same as in 6089cc0, which I reviewed last time.
- The finalization re-register now has a test: resolved.
FinalizedConstructorTestcounts its finalizer runs with a static counter and has a constructor that throws. The subprocess script runs three constructions: a failed bind, which must not be finalized; a successfulFinalizedConstructorTest(); and the throwing constructor. It then asserts that exactly 2 finalizers ran. I built the head and checked it locally (Python 3.11, .NET 10):- The test passed 30 out of 30 runs, so the GC-dependent count isn't flaky here.
- With
GC.ReRegisterForFinalizeinMethodBinder.Invokecommented out, it fails with['caught', 'thrown', '0'] != ['caught', 'thrown', '2']. So the test catches the regression I asked about.
Findings
None. Earlier rounds resolved every finding: the __namespace__ double run, the optional-argument and selected-overload cases, and finalization of a failed construction. The double run for super().__init__(args) is a known limitation, which the maintainers accepted as option A.
Risk
The risk:high label stays. Every .NET object constructed from Python goes through this path, including every Python algorithm, custom data type and indicator. Finalization is now suppressed and re-enabled on each construction, and T.__new__(T) returns an object whose constructor hasn't run, as in stock pythonnet. The tests, probes and green Lean workflows support approving the change, but they don't reduce how much it can affect. A person should merge it.
Other repositories
No other repository needs a code change. Downstream gets the fix with the next pythonnet package version.
There was a problem hiding this comment.
Review of 2692e63
Both regressions from my previous review are resolved:
- Failed binding and finalization: reflected allocations stay non-finalizable until a constructor is about to execute. The subprocess test now also verifies that successful and throwing constructions remain finalizable, rather than testing only the abandoned allocation.
- Explicit optional overload: deduplication compares the constructor that actually ran, so
super().__init__.__overloads__[System.String]()still executes the selected overload.
The maintainers' option A is preserved, including the accepted extra constructor call when a Python subclass passes arguments to super().__init__. I found no new blocking regression in the changes since my previous review.
Verification
- CI is green on this head: Python 3.8–3.14 and both Lean Python workflows.
dotnet test --runtime any-x64 src/embed_tests/ --filter "FullyQualifiedName!~SetPythonPath": 1047 passed, 8 skipped; the isolatedSetPythonPathtest passed 1/1. All 21 QCTests passed.pytest --runtime netcore tests: 486 passed, 23 skipped.dotnet test --runtime any-x64 src/python_tests_runner/: 2 passed. Initial local runs encountered installed-package import collisions; isolating Python dependencies and giving repository imports precedence eliminated those failures.- The finalization script passed 20/20 fresh subprocesses, each catching both expected exceptions and counting exactly two finalizations. In an isolated copy with
GC.ReRegisterForFinalizeremoved, the regression test fails with 0 instead of 2, confirming it protects re-registration. - End-to-end functional assertions passed with the reviewed runtime: direct and subclass constructor counts, namespaced subclass construction, explicit overload selection, managed-state preservation without
super(), explicit reinitialization, failed-binding survival, and finalization counts. These were not clean end-to-end passes: the embedding host subsequently aborted during shutdown inPy.GILState.Finalize. I reproduced the identical shutdown failure with an unchangedmasterruntime, so it is not a new regression from this PR.
Risk and consumers
I retained risk:high: this changes the common CLR construction path and introduces finalization suppression/re-registration, so the blast radius remains broad despite the passing evidence.
No consumer API or algorithm edits are required. Lean adopts the fix through its next pythonnet package update.
What changed and why
Constructing a .NET class from Python ran a constructor twice on the same object whenever the type had a public constructor callable with no arguments (parameterless, or all parameters optional). Stock pythonnet runs it once; the extra run comes from #67. It was also reported upstream as pythonnet#2555.
MetaType.tp_callcallstp_new, thenInit.ClassObject.tp_new_implbinds zero arguments against the type's constructors and invokes the match on the new object. Bug constructor default call #67 added this so that a Python subclass whose__init__never callssuper().__init__()still gets its managed base constructor.Initcallsobj.__init__(*args, **kw), which binds and invokes the matching constructor on the same object again.Field initializers allocated twice, and constructor side effects (event subscriptions, registrations, counters) happened twice.
The fix:
ClassObject.tp_new_impl: when the type being constructed is the reflected class itself, no constructor runs intp_new, becausetp_callalways follows with__init__. Subclasses that declare__namespace__keep the current path: their generated .NET type is registered as its own reflected type, but they're Python subclasses. Until a constructor runs on that object, none of its field initializers have run. Sotp_newsuppresses its finalization (GC.SuppressFinalize, marked byCLRObject.awaitingCtor), andMethodBinder.Invokeregisters it again (GC.ReRegisterForFinalize) right before the first constructor runs on it. A failedT(bad args)therefore no longer leaves the uninitialized object to its finalizer. A constructor that throws still gets its object finalized, as with a .NETnew.tp_new, before their own__init__runs. That keeps Bug constructor default call #67's guarantee, and the base constructor still runs before the subclass's__init__. Running it after__init__instead would reset managed state that__init__sets without callingsuper(), such as a property assigned in__init__.tp_newnow records the constructor it ran on the object (CLRObject.ctorRanInNew), andMethodBinder.Invokechecks it. It skips the call when a zero-argument base__init__()binds that same constructor. An explicitly selected overload such assuper().__init__.__overloads__[System.String]()binds a different constructor, so it still runs. For a__namespace__subclass it also skips the base type's parameterless constructor: the generated type mirrors each base constructor and chains to it, so the parameterless constructortp_newran has already run the base one. Any constructor call through the binder clears the mark, and so doesClassBase.Initonce that construction's__init__returns. So a later explicitobj.__init__()still runs the constructor, as it does today.Results, with each constructor recording its calls (Python 3.11, .NET 10):
SomeClass()()+()()SomeClass(1, "str")()+(int, string)(int, string)__init__()+()()super().__init__()()+()()super().__init__(1, "s")()+(int, string)()+(int, string)(unchanged)__init__not callingsuper()()()__namespace__subclass, no__init__()+()()__namespace__subclass callingsuper().__init__()()+()()OnlyDefault()(only a parameterless ctor)()+()()OnlyArgs(1, "s")(only an(int, string)ctor)(int, string)(int, string)OptionalArgs(5)(only an(int a = 0)ctor)(0)+(5)(5)OptionalArgs()(0)+(0)(0)Known limitation (accepted): one double run remains, for a Python subclass whose
__init__callssuper().__init__(args)with arguments:()intp_new, then the constructor with arguments. Intp_newthere's no way to know whether__init__will call a base constructor at all, so the parameterless one runs first. Deferring it until after__init__would reset managed state that subclasses set in__init__without callingsuper(), which changes how existing Lean algorithms behave. Maintainers decided to keep this behaviour (option A), andOptionalArgs_ValueConstructorpins it.Tests
QCTest(src/embed_tests/QCTest.cs): the expected counts now match a single run.PureCSharpConstructionexpectsCalledDefault == 0, andNoArgs_DefaultConstructor,NoArgs_NoConstructorandWithArgs_DefaultConstructorexpectCalledDefault == 1(it was 2). New tests:PureCSharpDefaultConstruction,PureCSharpOptionalArgsConstructionandPureCSharpOptionalArgsDefaultConstructionconstruct the type directly.NoArgs_DefaultConstructorAfterOtherConstructionconstructs another object inside the subclass's__init__before callingsuper().__init__().ReinitAfterConstructionRunsConstructorAgaincalls__init__()explicitly after construction.OptionalArgs_DefaultConstructorandOptionalArgs_ValueConstructorcover Python subclasses of a type whose only constructor has all-optional parameters, callingsuper().__init__()(1 run) andsuper().__init__(5)(2 runs, the accepted limitation below).Namespaced_NoInitandNamespaced_DefaultConstructorcover__namespace__subclasses.SelectedOverload_OptionalArgsConstructor: a subclass callingsuper().__init__.__overloads__[System.String]()on a type with()and(string value = "optional")ends with the string constructor's state.New
test_construction_finalization(tests/test_constructors.py, fixturePython.Test.FinalizedConstructorTest). The fixture has a field initializer, a finalizer that uses the field and counts its runs, and a constructor that throws. In a subprocess, the test callsFinalizedConstructorTest("unsupported")(catches theTypeError),FinalizedConstructorTest(), and the throwing constructor (catches its exception). It then forces finalization and asserts a normal exit with exactly 2 finalizer runs: the constructed instance and the one whose constructor threw, but not the never-constructed one. With theGC.ReRegisterForFinalizecall removed, it counts 0.Without the runtime change, 9 of the original 16
QCTestsfail. Without the__namespace__condition,Namespaced_DefaultConstructorfails. On 5757cec, the failed construction aborts the subprocess withNullReferenceExceptioninFinalizedConstructorTest.Finalize, andSelectedOverload_OptionalArgsConstructorgets"default". With the full change, all 21QCTestsand the subprocess test pass.Ran CI's suites on Python 3.11, on this branch and on
master:dotnet test --runtime any-x64 src/embed_tests/ --filter "FullyQualifiedName!~SetPythonPath": branch Passed 1047, Skipped 8, Failed 0 (master: Passed 1037, Skipped 8; the difference is the 10 new tests).dotnet test --runtime any-x64 src/embed_tests/ --filter "FullyQualifiedName~SetPythonPath": Passed 1.pytest --runtime netcore tests: 486 passed, 23 skipped (master: 485; +1 new test).dotnet test --runtime any-x64 src/python_tests_runner/: Passed 2.The Lean Python unit and regression test workflows run in this PR's CI.