fix finalizing and atexit timing - #6626
Conversation
Sorry, something went wrong.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command. You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file. WalkthroughThe interpreter finalization sequence is restructured to dynamically import the threading module and call threading._shutdown() during shutdown. Additionally, unraisable exception handling is suppressed when the VM is finalizing by checking the finalizing flag. 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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
|
You may want to take a look in test_bdb for 31 tests that can be un-skipped. |
Sorry, something went wrong.
|
let me check this again when #6718 is done |
Sorry, something went wrong.
|
@fanninpm they are downgraded to expectedFailure (@terryluan12 thanks!) |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agentsIn `@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.
Sorry, something went wrong.
| 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.
⚠️ Potential issue | 🟠 Major
🧩 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 AgentsIn `@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.
Sorry, something went wrong.
That's better than nothing! |
Sorry, something went wrong.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.