Repository navigation
Align weak-reference collection with Python 3.15 finalization - #9168
Conversation
Trace and retain owned callback edges independently of weak referent links. Clear callback-bearing weakrefs before finalizers, suppress callbacks from unreachable weakrefs, and close remaining weakref ingress after resurrection checks while the world is stopped. Publish and unlink intrusive nodes safely. Refresh weakref and canonical weakref/GC/finalization tests through update_lib from pinned CPython 3.15.0. Retire four obsolete guards only after fresh body passes, retaining applicable RustPython limitations. Assisted-by: OpenAI Codex:model version unavailable
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: :4: SyntaxWarning: invalid decimal literal dependencies:
dependent tests: (227 tests)
[ ] lib: cpython/Lib/io.py dependencies:
dependent tests: (108 tests)
[x] test: cpython/Lib/test/test_finalization.py dependencies: dependent tests: (no tests depend on finalization) [x] test: cpython/Lib/test/test_gc.py (TODO: 2) dependencies: dependent tests: (188 tests)
Legend:
|
Migrate the complete CPython 3.15.0 test_io package at d220409 with the fail-closed legacy-map updater, preserving all ten existing guards. Retire the seven consolidated standalone suites and update io/file dependency paths with mapping regression tests. The canonical GC tests ignore ResourceWarning without retaining its source, so callback-free weakrefs correctly reflect surviving owners. This closes the stale test dependency exposed by the lifecycle change. Assisted-by: OpenAI Codex:model version unavailable
Merging this PR will not alter performance
Comparing Footnotes
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
| wr_object: Radium::new(obj as *const PyObject as *mut PyObject), | ||
| callback: UnsafeCell::new(callback), | ||
| wr_object: Radium::new(ptr::null_mut()), | ||
| callback: PyRwLock::new(callback), |
There was a problem hiding this comment.
this lock is looking suspicious
There was a problem hiding this comment.
Reworked in f8a8d64 to follow CPython free-threading: the weakref retains the original referent stripe key after wr_object is cleared. All callback access uses that existing stripe; the separate callback RwLock is removed. Drop/tp_clear unlink and steal the callback while locked, then release callback ownership after unlocking. The ordinary wr() upgrade path is unchanged.
RustPython’s running-world GC phases and gc.get_referents still need stripe synchronization, rather than CPython’s lockless stopped-world operations. Fresh canonical weakref/finalization/weakset, selected GC, six IO GC regressions, workspace/C-API native and doc tests, both Clippy gates, and nonthreading compilation passed. Existing guards and the previously documented local GC exclusion are unchanged. Full hosted checks for the new head are pending.
Follow CPython's free-threading weakref locking model instead of keeping an independent callback RwLock. Retain the original referent stripe key so callback access stays synchronized after GC clears the referent. Unlink and steal callbacks under that stripe, then release ownership outside the lock. Preserve synchronization for running-world GC paths and gc.get_referents while keeping ordinary weakref upgrades unchanged. Assisted-by: OpenAI Codex:model version unavailable
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8a8d643a7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let weak_payload = PyWeak { | ||
| pointers: Pointers::new(), | ||
| wr_object: Radium::new(obj as *const PyObject as *mut PyObject), | ||
| wr_object: Radium::new(ptr::null_mut()), |
There was a problem hiding this comment.
Track weakrefs only after publishing their referent
Because this change makes PyWeak traversable, PyRef::new_ref auto-tracks the object before returning, while wr_object remains null until the later list insertion. In a threaded build, a concurrent gc.get_objects() can therefore retain this half-built weakref; in a non-threaded build, an inline auto-collection can expose it to a finalizer the same way. The retained object is observable as dead and then later becomes live (or remains as a never-live duplicate if reentrant creation wins), which is not a valid weakref state transition; allocate it untracked and track it only after linking, storing the referent, and releasing the stripe lock.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 51e0ffe. PyWeak now opts out of automatic allocation-time GC tracking. WeakRefList::add tracks it only after linking, publishing wr_object, and releasing the stripe; generic reuse losers stay untracked. The constructor's initial strong reference prevented reclamation but did not prevent the premature visibility described here.
Added an allocator-boundary regression using actual GC-list membership and header state, covering refs, both proxies and a subclass, plus tracking of completed constructors. It does not embed Python behavior tests. The old allocation path's failure is source-established, not claimed as an executed baseline repro. Fresh workspace/C-API native and doc tests, both Clippy gates, nonthread compilation, canonical lifecycle suites and six IO GC regressions pass. Hosted checks for the new head are pending.
Keep freshly allocated weakrefs out of GC's object lists until they are linked and their referent is published. Release the referent stripe before tracking, since tracking can collect and invoke finalizers. Generic reuse losers remain untracked. Exercise the allocator boundary directly for reference, proxy, callable proxy and subclass payloads, and ensure completed constructors track their objects. This prevents observers from retaining a half-built dead weakref that later becomes live. Assisted-by: OpenAI Codex:model version unavailable
| pointers: Pointers::new(), | ||
| wr_object: Radium::new(obj as *const PyObject as *mut PyObject), | ||
| wr_object: Radium::new(ptr::null_mut()), | ||
| referent_addr: obj as *const PyObject as usize, |
There was a problem hiding this comment.
why not simply obj as *const PyObject but as usize?
There was a problem hiding this comment.
I used usize because referent_addr is only the immutable stripe-selection key: weakref_lock::lock hashes its bits, and the value is never converted back to a pointer or dereferenced. The key must survive clearing wr_object and freeing the referent so callback access still uses the original stripe. A typed raw pointer would also work; there is no Send/Sync requirement forcing usize.
There was a problem hiding this comment.
tell me how about CPython do as reference
There was a problem hiding this comment.
CPython 3.15 stores a pointer to the selected lock, rather than another referent pointer: PyMutex *weakrefs_lock, initialized from &WEAKREF_LIST_LOCK(ob). Clearing uses that saved lock after wr_object becomes Py_None. My integer key selects the same stripe again, but does not match that representation. I will align this with CPython by retaining a safe reference to the existing static stripe, with a no-op handle in nonthreaded builds. This needs no extra lock or unsafe pointer.
There was a problem hiding this comment.
Aligned in 5cbebf4. Like CPython’s weakrefs_lock, PyWeak now retains the selected existing stripe directly: a safe static-lock reference under threading and a zero-sized no-op handle otherwise. No saved referent address, extra lock, or new unsafe code. Stripe selection, memory ordering, fork reset, GC publication and callback release outside the lock are unchanged.
Fresh canonical lifecycle/selected-GC and six IO-GC regressions, workspace/C-API native and doc tests, both Clippy gates, nonthreaded compilation and normal hooks passed. Existing guards and the documented local GC exclusion are unchanged. Hosted verification for this head is now complete: all 25 jobs in CI run 39037908394 passed, including full Linux/macOS/Windows Python suites. All additional checks are successful or intentionally skipped.
Store a safe handle to the selected static weakref stripe instead of retaining and rehashing the referent address. Keep the same lock after referent clearing, matching CPython free-threaded weakrefs_lock. The nonthreaded handle is zero-sized and introduces no locking. Preserve lock selection, acquire/release ordering, fork reset, deferred GC publication, and callback-owner release outside the stripe. No new locks or unsafe code are needed. Assisted-by: OpenAI Codex:model version unavailable
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cbebf40c0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| self.assertIs(wr(), None) | ||
| # This used to be None because weakrefs were cleared before | ||
| # calling finalizers. Now they are cleared after. | ||
| self.assertIsNotNone(wr()) |
There was a problem hiding this comment.
Revert the direct canonical-test logic changes
This reverses the existing assertion so that wr() must now be non-None and adds companion callback assertions, while the same patch also rewrites the GC, weakref, and IO canonical suites. Repository policy permits test-file edits only for adding or removing expected-failure markers, so these behavioral test changes cannot land in this form; keep the production/native Rust fix but revert the direct Lib/test logic and assertion changes.
AGENTS.md reference: AGENTS.md:L278-L282
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This assertion and its callback controls are the canonical CPython 3.15 expectations, imported through update_lib in 1df14b3 as part of the explicitly requested Python 3.15 migration. They were not hand-edited to accommodate this implementation.
I checked the complete test_finalization.py at this head against pinned CPython d220409a13b77de7a6c30abe16ea90db01528edc: both have the identical Git blob 9dd68cf8d574139d990659827cd7f210f953bcc9. The exact upstream resurrection assertions are here: https://github.com/python/cpython/blob/d220409a13b77de7a6c30abe16ea90db01528edc/Lib/test/test_finalization.py#L273-L296 . The related weakref/GC and complete IO-package updates likewise belong to the documented update_lib migration, rather than selected assertion rewrites.
The latest 5cbebf4 commit changes only Rust core.rs to retain the original stripe lock. Reverting this canonical expectation would restore the older Python behavior the requested migration is intended to replace; I am keeping the imported coverage intact.
Align weak-reference collection with Python 3.15 finalization
Summary
Extract the weakref/finalizer lifecycle work from #8954 onto main's existing collector.
The corresponding canonical sequence is in Python/gc.c and Objects/weakrefobject.c. Review core.rs callback ownership/list operations together with gc_state.rs finalization and resurrection ordering.
Scope and related work
Base: main 9a16d28. The source batch contains two native lifecycle files, the four related weakref/GC/finalization library files, the complete canonical IO test-package migration, and updater dependency-path metadata/tests. No dependency, heap architecture, embedding API, interpreter isolation, workflow or unrelated teardown changes.
Open #8966 also reports the missing callback traversal edge and includes a simpler stripe-protected traversal with unrelated teardown changes. Open #8939 addresses related lifetime issues as part of its much larger collector/embedding redesign; #8944 contains separate interpreter-local visibility work. This extraction uses the existing main architecture and leaves those authors' branches unchanged. The merged #9094 stop-the-world transaction prerequisite is already on the base.
Canonical IO test closure
Initial hosted Ubuntu and macOS runs exposed a stale
PyIOTest.test_garbage_collection: its warning recorder retainedWarningMessage.source, keeping the file alive during finalization. The corrected callback-free weakref now properly observes that surviving owner. CPython 3.15's canonical test ignoresResourceWarningwithout recording it and keeps the collection/content assertions intact.Migrate all twelve
Lib/test/test_io/files from the same pinned CPython source withscripts/update_lib migrate --legacy-map, replacing the seven legacy standalone suites. All ten existing guards are preserved, including five Windows GC skips and inherited expected-failure wrappers. No selected-method backport, assertion edits, forwarding shims, new guards or duplicated tests are introduced._pyio.pyalready matches the pinned source. The updater'sioandfilealiases now point to the complete package, with regression tests for both mappings.Validation for earlier head
429c7718test_weakref: 138 cases, 13 existing skips, one retained expected failure. Wholetest_finalization: 18 cases, five existing skips. Wholetest_weakset: 46 cases, all passed.test_gc: 60 cases, 16 existing skips, one retained expected failure; the exact local exclusion is below. All four module runs returned success, without unexpected successes, timeouts or altered-environment results.crates/capinative tests: 123 passed, four ignored; its doctest gate passed with zero cases.-D warnings. The nonthreading VM compile check passed with defaults disabled and onlycompilerenabled.Local test_gc coverage excludes exactly PythonFinalizationTests.test_warnings_fini using the standard test runner. That test creates an asyncio AF_UNIX socketpair, which this execution environment previously denied. Its canonical source is unchanged. A selected local run is not full-module coverage. The exact-head hosted runs below supply full-module GC coverage with the existing guards. Existing crash/race, CPU-resource, CPython-only and missing-C-API guards remain in effect unless fresh evidence explicitly supports a permitted retirement.
Local IO verification uses the exact canonical general, bufferedio, textio, memoryio, file and univnewlines submodules. General excludes
*IOTest.test_large_file_ops(the >2 GiB sparse-file cases) via the standard runner. Direct submodule selection avoids package-wide discovery.test_fileiois not imported locally because it performs an import-time strace capability probe;test_largefileandtest_signalsare also left to hosted CI because of their socket, large-file and signal effects. The exact-head hosted runs below supply full-packagetest_iocoverage with the existing guards. Canonical source and guards are unchanged by these local selections.The IO closure changes only tests and updater metadata, so its runtime gates reuse the already sealed default release, which does not freeze the standard library. A separate manifest verifies every native/production/build input is unchanged and binds the repaired test tree and exact executable/ELF hashes without replacing the original build provenance.
CPython free-threading correction
CPython's free-threading design uses the referent stripe for mutable weakref state and retains its lock pointer after the referent is cleared. This correction follows that design:
PyWeakkeeps the original address solely as a stripe key, never dereferences it, and uses the same stripe for callback access. The separate callbackPyRwLockis removed. Ordinarywr()upgrade is unchanged.RustPython collects callbacks and clears payloads after resuming threads, and
gc.get_referents()traverses outside stop-the-world. Those paths therefore retain stripe synchronization rather than copying CPython's lockless stopped-world GC operations. Stripe critical sections neither run Python, release Python references nor suspend; callback drops remain outside the lock. No canonical tests, guards or snippets change in this correction.Revision
f8a8d643a7passed a fresh default release build, both workspace/C-API Clippy gates, 1,359 workspace native tests, 123 C-API tests (four ignored), 24 workspace doctests (18 ignored), C-API doctests and the nonthreading VM compile check. The unchanged canonical suites passed again: weakref 138, finalization 18, weakset 46 and selected GC 60 with the exact pre-existing local exclusion documented above. The six canonical IO garbage-collection cases also passed. Normal commit hooks passed, and source/executable/dependency hashes were reverified. No new guards or test changes.These local gates tested the existing branch based on
9a16d284, not a new merge with current main. All eight required checks and all 25 jobs in CI run 39008837863 passed for headf8a8d643a7682d613d5fa9d804d71581191263a6. The older hosted results below apply only to head429c7718.The completed Ubuntu, macOS and Windows logs explicitly confirm full
test_io,test_gc,test_weakref,test_finalizationandtest_weaksetpasses. All three checked out merge2ed8eaf03a87a01be8456c47281ba929cf671236of this head into mainfc9f4a95416bf7ffcc43747c256c2c4a35f996d9. They used-u all, emptySKIPSand no IO/GC selectors; Windows retains its unrelated launcher-version exclusion. Existing platform/resource/CPython guards still apply.Hosted results for earlier head
429c7718All eight required checks passed for head
429c7718e703b139fadc1b12f417c6bb9d76f351in CI run 38971792428: lint, three native-test platforms, three snippets/CPython platforms and WASI. The workflow completed successfully.The completed Ubuntu, macOS and Windows logs explicitly show full
test_io,test_gc,test_weakref,test_finalizationandtest_weaksetpassing. Each checked out merge7a2bdb7528da48d3bfbfbc1c741561344a3074f1of the exact head into base9a16d284. The runs used-u all, emptySKIPSand no IO/GC selectors; Windows retains its unrelated launcher-version exclusion. Existing platform/resource/CPython guards still apply.AI assistance
OpenAI Codex extracted and reviewed the source changes, prepared the canonical migration, and performed the listed checks. The exact model version is unavailable. No human review or manual-use verification is claimed here; this is a draft for maintainer review.
Summary by CodeRabbit
Weakref publication ordering
Addressed the follow-up review about GC exposing a half-built weakref. PyWeak now opts out of automatic allocation-time tracking; WeakRefList::add explicitly tracks it only after list insertion, referent publication, and stripe release. Generic reuse losers remain untracked. The constructor’s strong root prevents collection, but the old ordering still allowed gc.get_objects to observe a dead-then-live transition.
A native allocator-boundary regression checks real allocation headers across ref/proxy/callable-proxy/subclass payloads and verifies completed constructors are tracked. It runs no Python program. Baseline failure is established from the previous allocator/track_new_object source path, not claimed as an executed baseline repro. Canonical GC-during-ref/proxy-creation tests remain unchanged.
CPython free-threaded gc.get_objects stops the world; RustPython’s enumeration does not. Deferred tracking follows RustPython’s existing allocation pattern while preserving the publication invariant, without adding another lock.
Implemented in 51e0ffe. Fresh final-source local validation passed: workspace native 1,360; C-API 123 (4 existing ignores); workspace doctests 24 (18 existing ignores); C-API doctests; both Clippy gates with warnings denied; nonthreading VM compilation; canonical test_weakref (138), test_finalization (18), test_weakset (46), selected test_gc (60), and all six existing IO garbage-collection regressions. Existing skips/expected failures and the documented local test_warnings_fini exclusion are unchanged. The new allocator regression also checks GC-list membership directly. One initial test-fixture layout assertion was corrected before these final gates.
Hosted verification for 51e0ffe is complete: CI run 39014010518 passed all 25 jobs. The full Ubuntu, macOS and Windows Python jobs each passed test_gc, test_io, test_weakref, test_finalization and test_weakset with all resources and empty SKIPS. Windows retains only its unrelated existing launcher-version exclusion. All additional checks are successful or intentionally skipped.
Local tests retain the existing 9a16d28 base. Hosted CI tested merge a3b5ff1, whose parents are current main fc9f4a9 and head 51e0ffe. The PR is mergeable and awaiting review. Earlier hosted results above belong to f8a8d64.
CPython lock-handle alignment
Revision
5cbebf40c018a8ff8d694f44df124eea83f4b28ffollows CPython free-threading more directly:PyWeak.weakrefs_lockretains a safe reference to the selected existing static stripe, rather than saving and rehashing the referent address. CPython likewise storesPyMutex *weakrefs_lock, initializes it from the referent's stripe, and uses it after referent clearing.The nonthreaded handle is zero-sized. Stripe selection, acquire/release ordering, fork reset, deferred GC publication and callback-owner release outside the lock remain unchanged. This adds no new locks or unsafe code. The earlier address-key description above records the previous revision.
Fresh final-source validation passed: 1,360 workspace native tests, 123 C-API tests (4 existing ignores), 24 workspace doctests (18 existing ignores), C-API doctests, both Clippy gates with warnings denied, nonthreading VM compilation, canonical weakref (138), finalization (18), weakset (46), selected GC (60), and six canonical IO-GC regressions. Existing guards and the documented local
test_warnings_finiexclusion remain unchanged. Normal commit hooks passed. Hosted verification is complete for this head: CI run 39037908394 passed all 25 jobs. Full Ubuntu, macOS and Windows Python logs each confirm test_gc, test_io, test_weakref, test_finalization and test_weakset passed. All 30 reported checks are successful, with one intentional pyperformance skip. Prior hosted results above apply to older revisions.