Don't leak partially-constructed modules when __init__ raises - #1251
Open
nstarman wants to merge 3 commits into
Open
Don't leak partially-constructed modules when __init__ raises#1251nstarman wants to merge 3 commits into
__init__ raises#1251nstarman wants to merge 3 commits into
Conversation
`Module.__new__` adds the new instance to `_currently_initialising`, and `_ModuleMeta.__call__` was responsible for removing it again. But it did so via a `tryself` variable assigned from the result of `super().__call__(...)`, which is never reached if construction raises. As `_IdSet` deliberately holds a strong reference (to stop `id` being reallocated), every failed construction leaked its instance for the lifetime of the program. There are two routes to this: a user `__init__` raising, and `BetterABCMeta.__call__` raising its `__abstractvars__` check, which happens after the instance has been constructed. We can't just `remove` the offending instance on the error path, as `type.__call__` doesn't hand back the partially-constructed object. Instead, record the size of `_currently_initialising` beforehand and unwind back to it if an exception propagates. Nested modules constructed during our `__init__` remove their own entries, so anything left above the mark was added on our behalf. The success path is unchanged in cost: a `len` and a `remove`, no allocation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Owner
|
Ah no! That's not good. For the proposed fix though: will this be threadsafe? |
Contributor
Author
|
Good catch! No, the proposed fix wasn't thread safe. |
The `mark`/`discard_after` unwinding introduced in the previous commit reads a
count of a set that is shared between threads, so the mark taken by one thread
and the unwind performed by another are coupled:
thread A: mark = 0
thread A: `__new__` registers A (len 1)
thread B: mark = 1
thread B: `__new__` registers B (len 2)
thread A: `__init__` raises -> unwind to 0
The unwind pops past B's entry, so thread B -- which is merely constructing its
own module -- then fails its `self in _currently_initialising` check and raises a
spurious `FrozenInstanceError` on its next field assignment.
The previous `add`/`remove` pairs didn't have this problem, as they are keyed on
`id(self)` and so only ever touch the caller's own instances. Restore that
independence by giving each thread its own set: an instance is only ever
initialised by the thread that constructed it, which also makes the ordering
assumption behind `discard_after` sound.
Note that this means a module whose `__init__` assigns fields to `self` from a
different thread is no longer supported.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Re-adding `__slots__` looks like a free optimization, but under `threading.local` slots are shared across threads rather than per-thread, so a new thread's `__init__` would clobber another thread's in-progress `_dict`. Leave a comment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
Claude tests are telling me that "losing slots plus the threading.local attribute lookup adds roughly 10% to Module() construction in a microbenchmark (~4360 → ~4806 ns for a 2-field module)". So this isn't optimal, but I haven't figured out a way to avoid this and solve the issue. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Module.__new__adds the new instance to_currently_initialising, and_ModuleMeta.__call__is responsible for removing it again. It did so via atryselfvariable assigned from the result ofsuper().__call__(...):If construction raises then the assignment is never reached, so
tryselfstaysNoneand nothing is removed. As_IdSetdeliberately holds a strong reference (to stopidbeing reallocated), every failed construction leaks its instance for the lifetime of the program.There are two routes in: a user
__init__raising, andBetterABCMeta.__call__raising its__abstractvars__check — the latter happening after the instance has been constructed.Fix
We can't simply
removethe offending instance on the error path, sincetype.__call__doesn't hand back the partially-constructed object. Splitting__new__/__init__explicitly in_ModuleMeta.__call__would give us the instance, but it would bypassBetterABCMeta.__call__and silently drop both of its abstract-class checks.So instead: record the size of
_currently_initialisingbeforehand, and unwind back to that mark if an exception propagates._IdSetis backed by an insertion-ordereddict, and nested modules constructed during our__init__remove their own entries, so anything left above the mark was added on our behalf.The success path is unchanged in cost — a
lenand aremove, no allocation — which seemed worth preserving given the recent module-init perf work. (A weakref-based_IdSetwould also fix this, but at the cost of a weakref allocation per instantiation.)Tests
Three regression tests in
tests/test_module.py, covering__init__raising, a nested failure inside an outer__init__, and theAbstractVarroute. Full suite passes locally (474 passed, 4 skipped).🤖 Generated with Claude Code