-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix finalizing and atexit timing #6626
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe interpreter finalization sequence is restructured to dynamically import the threading module and call Changes
Sequence DiagramsequenceDiagram
participant VM as Virtual Machine
participant Threading as Threading Module
participant Atexit as Atexit System
VM->>Threading: Attempt dynamic import
alt Import Successful
VM->>Threading: Call threading._shutdown()
Threading-->>VM: Shutdown complete
else Import Failed
VM-->>VM: Continue
end
VM->>VM: Set finalizing flag = true
VM->>Atexit: Run atexit functions
Atexit-->>VM: Cleanup complete
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
You may want to take a look in |
|
let me check this again when #6718 is done |
|
@fanninpm they are downgraded to expectedFailure (@terryluan12 thanks!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/vm/src/vm/mod.rs`:
- Around line 552-562: The finalizing flag is set before calling
atexit::_run_exitfuncs, causing run_unraisable to early-return and suppress
atexit handler exceptions; either move the store to state.finalizing to after
atexit::_run_exitfuncs(vm) so finalization suppression happens post-atexit, or
add a small atexit bypass: introduce an atexit-running marker on VM state (e.g.,
vm.state.atexit_running) set around atexit::_run_exitfuncs and change
run_unraisable to allow reporting when that marker is true (while still
suppressing in other finalization contexts). Ensure references are to
run_unraisable, atexit::_run_exitfuncs, and state.finalizing (or the new
state.atexit_running) so reviewers can find and apply the change.
| pub fn run_unraisable(&self, e: PyBaseExceptionRef, msg: Option<String>, object: PyObjectRef) { | ||
| // Suppress unraisable exceptions during interpreter finalization. | ||
| // when daemon thread exceptions and | ||
| // __del__ errors are silently ignored during shutdown. | ||
| if self | ||
| .state | ||
| .finalizing | ||
| .load(std::sync::atomic::Ordering::Acquire) | ||
| { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the context of this code
# Find where the finalizing flag is set and where atexit runs
cd crates/vm/src/vm
grep -n "finalizing" mod.rs | head -20Repository: RustPython/RustPython
Length of output: 184
🏁 Script executed:
# Look for atexit-related code in the vm
grep -r "atexit" crates/vm/src --include="*.rs" | head -20Repository: RustPython/RustPython
Length of output: 1449
🏁 Script executed:
# Find where finalizing is set to understand the order of operations
grep -r "finalizing.*=" crates/ --include="*.rs" -B2 -A2 | head -40Repository: RustPython/RustPython
Length of output: 1454
🏁 Script executed:
# Look at the vm initialization and shutdown sequence
grep -n "fn run_unraisable\|fn finalize\|fn exit" crates/vm/src/vm/mod.rs | head -20Repository: RustPython/RustPython
Length of output: 168
🏁 Script executed:
# Look at the finalization sequence in interpreter.rs
grep -n "finalizing\|atexit" crates/vm/src/vm/interpreter.rs | head -30Repository: RustPython/RustPython
Length of output: 435
🏁 Script executed:
# Get more context around the finalization and atexit calls
sed -n '1,300p' crates/vm/src/vm/interpreter.rs | tail -100Repository: RustPython/RustPython
Length of output: 3393
🏁 Script executed:
# Look at the full shutdown/exit handling sequence
rg "fn.*exit|fn.*finali" crates/vm/src/vm/interpreter.rs -A 10 | head -50Repository: RustPython/RustPython
Length of output: 389
🌐 Web query:
CPython Py_FinalizeEx atexit exceptions handling unraisable hook
💡 Result:
Short summary — how CPython handles exceptions during Py_FinalizeEx / atexit and the unraisable hook
-
Py_FinalizeEx runs Python-level shutdown which includes running atexit handlers. Exceptions raised in atexit handlers produce a printed traceback (to sys.stderr) and the last non-SystemExit exception raised is re-raised after all handlers have run. [1][2]
-
C-level exit handlers (Py_AtExit) are separate: they are called by Py_FinalizeEx but run after Python internal finalization; they must not call Python APIs. Py_AtExit handlers are for C code, not Python-level cleanup. [3]
-
sys.unraisablehook (added in Python 3.8) is the configurable hook used for "unraisable exceptions" (e.g., exceptions in del, weakref callbacks, GC callbacks). By default Python logs these to sys.stderr; you can override sys.unraisablehook to customize handling. [4]
-
Important caveat: some "unraisable" errors occur very late in interpreter finalization (after core modules like sys and stderr have been cleared). In that case the custom sys.unraisablehook cannot be called and the error may go unreported (PyErr_WriteUnraisable may be a no‑op if stderr/sys are gone). This limitation is documented and tracked in the issue thread that introduced sys.unraisablehook. [4][5]
Practical implications
- Do not call Py_FinalizeEx recursively or from code that may run during shutdown (atexit handlers, del, etc.). The docs explicitly warn against that. [3]
- If you need reliable reporting of errors during shutdown, avoid relying on very-late finalizers; prefer earlier, explicit cleanup or handle errors before finalization. [3][1][4]
Sources
[1] atexit — Exit handlers (Python docs).
[2] atexit behavior / examples (Python docs, PyMotW).
[3] Initialization, Finalization, and Threads / Py_AtExit (CPython C-API docs).
[4] Issue and PR discussion and docs for sys.unraisablehook (bpo-36829 / sys docs / Victor Stinner blog).
[5] Python tracker discussion about very-late unraisable exceptions during finalization.
Atexit exceptions are being suppressed during finalization, which differs from CPython's behavior.
RustPython sets the finalizing flag at line 143, before calling atexit::_run_exitfuncs(vm) at line 146. This causes the run_unraisable call in atexit handlers to return early and suppress exception reporting. CPython still prints atexit handler exceptions to stderr during finalization, making them visible for debugging. Consider either moving the finalizing flag to after atexit completion, or adding a special case in run_unraisable to allow atexit exceptions to bypass the finalization suppression.
🤖 Prompt for AI Agents
In `@crates/vm/src/vm/mod.rs` around lines 552 - 562, The finalizing flag is set
before calling atexit::_run_exitfuncs, causing run_unraisable to early-return
and suppress atexit handler exceptions; either move the store to
state.finalizing to after atexit::_run_exitfuncs(vm) so finalization suppression
happens post-atexit, or add a small atexit bypass: introduce an atexit-running
marker on VM state (e.g., vm.state.atexit_running) set around
atexit::_run_exitfuncs and change run_unraisable to allow reporting when that
marker is true (while still suppressing in other finalization contexts). Ensure
references are to run_unraisable, atexit::_run_exitfuncs, and state.finalizing
(or the new state.atexit_running) so reviewers can find and apply the change.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.