Reinit IO buffer locks after fork to prevent deadlocks#7339
Reinit IO buffer locks after fork to prevent deadlocks#7339youknowone merged 3 commits intoRustPython:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Unix-only post-fork reinitialization for per-object PyThreadMutex state and for stdio buffer locks. Introduces RawThreadMutex::reinit_after_fork and ThreadMutex::raw(), a helper reinit_thread_mutex_after_fork, and a VM hook to reset stdin/stdout/stderr mutexes in the single-threaded child after fork. Changes
Sequence DiagramsequenceDiagram
participant Child as Forked Child
participant PostFork as py_os_after_fork_child
participant VM as VirtualMachine
participant IOReInit as reinit_std_streams_after_fork
participant IOWalker as reinit_io_locks
participant RawReset as reinit_thread_mutex_after_fork
Child->>PostFork: start (single-threaded)
PostFork->>IOReInit: reinit_std_streams_after_fork(&VM)
IOReInit->>VM: retrieve stdin/stdout/stderr
IOReInit->>IOWalker: reinit_io_locks(stdin)
IOWalker->>IOWalker: inspect wrapper types / nested buffers
IOWalker->>RawReset: call reinit_thread_mutex_after_fork on internal mutex
RawReset->>RawReset: reset owner=0 and zero inner mutex state
IOWalker->>IOWalker: recurse for stdout and stderr
IOReInit-->>PostFork: streams reinitialized
PostFork->>Child: child ready
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin io-fork |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d3fce68b5
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| use crate::common::lock::reinit_thread_mutex_after_fork; | ||
|
|
||
| if let Some(tio) = obj.downcast_ref::<TextIOWrapper>() { | ||
| unsafe { reinit_thread_mutex_after_fork(&tio.data) }; |
There was a problem hiding this comment.
This unconditionally resets TextIOWrapper's PyThreadMutex in the child, but the forking thread can still have a live guard on that same lock when fork() is called from code executed under the lock (for example, TextIOWrapper::write calls encoder.encode while holding self.lock(vm), and a custom encoder can call os.fork()). After this reset, that pre-fork guard will later drop and call unlock() on a mutex that was forcibly reinitialized, which can corrupt lock state or panic in parking_lot. Please guard this path so locks currently owned by the calling thread are not blindly reinitialized.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm/src/stdlib/io.rs`:
- Around line 5005-5031: The TextIOWrapper branch currently reinitializes
wrapper/buffer locks but misses reinitializing the decoder's thread mutex;
update the TextIOWrapper handling in the reinitialization traversal to also
check tio.decoder (and any inner decoder like IncrementalNewlineDecoder) and
call unsafe { reinit_thread_mutex_after_fork(&decoder.some_mutex_field) } (or
the decoder root object) and reinit any decoder-side locks analogous to
reinit_io_locks so the decoder's PyThreadMutex cannot remain owned by a dead
parent thread.
- Around line 4992-4999: The function reinit_std_streams_after_fork should be
marked unsafe to reflect its fork-only precondition: change its signature to pub
unsafe fn reinit_std_streams_after_fork(vm: &VirtualMachine) and update all call
sites to invoke it within an unsafe block (e.g., where it's currently used from
posix.rs). This documents and enforces that it may call unsafe reinit_io_locks,
which in turn calls unsafe reinit_thread_mutex_after_fork, preventing accidental
safe usage elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 6ead2f11-5edf-4b09-9b26-423e27798d64
📒 Files selected for processing (4)
crates/common/src/lock.rscrates/common/src/lock/thread_mutex.rscrates/vm/src/stdlib/io.rscrates/vm/src/stdlib/posix.rs
crates/vm/src/stdlib/io.rs
Outdated
| pub fn reinit_std_streams_after_fork(vm: &VirtualMachine) { | ||
| for name in ["stdin", "stdout", "stderr"] { | ||
| let Ok(stream) = vm.sys_module.get_attr(name, vm) else { | ||
| continue; | ||
| }; | ||
| reinit_io_locks(&stream); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the function in question
cd crates/vm/src/stdlib && wc -l io.rsRepository: RustPython/RustPython
Length of output: 75
🏁 Script executed:
# Read the specific function and surrounding context
sed -n '4985,5040p' crates/vm/src/stdlib/io.rsRepository: RustPython/RustPython
Length of output: 2025
🏁 Script executed:
# Find all callsites of reinit_std_streams_after_fork
rg -n 'reinit_std_streams_after_fork' --type=rust -C2Repository: RustPython/RustPython
Length of output: 1242
🏁 Script executed:
# Examine the reinit_io_locks function to understand safety preconditions
rg -n 'fn reinit_io_locks' --type=rust -A 30Repository: RustPython/RustPython
Length of output: 2238
🏁 Script executed:
# Check if the function is cfg-gated and understand its context
rg -n 'reinit_std_streams_after_fork\|fork' crates/vm/src/stdlib/io.rs | head -20Repository: RustPython/RustPython
Length of output: 47
Mark reinit_std_streams_after_fork as unsafe to encode its fork-only precondition.
This function calls unsafe { reinit_thread_mutex_after_fork(...) } via reinit_io_locks, which is only valid immediately after fork(). The current safe pub fn signature does not reflect this precondition, allowing potential misuse within the crate. Although the function is currently crate-private and only called correctly after fork (in posix.rs), the type signature should enforce the safety invariant.
Suggested API boundary fix
#[cfg(all(unix, feature = "threading"))]
-pub fn reinit_std_streams_after_fork(vm: &VirtualMachine) {
+pub unsafe fn reinit_std_streams_after_fork(vm: &VirtualMachine) {
for name in ["stdin", "stdout", "stderr"] {
let Ok(stream) = vm.sys_module.get_attr(name, vm) else {
continue;
};
reinit_io_locks(&stream);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/vm/src/stdlib/io.rs` around lines 4992 - 4999, The function
reinit_std_streams_after_fork should be marked unsafe to reflect its fork-only
precondition: change its signature to pub unsafe fn
reinit_std_streams_after_fork(vm: &VirtualMachine) and update all call sites to
invoke it within an unsafe block (e.g., where it's currently used from
posix.rs). This documents and enforces that it may call unsafe reinit_io_locks,
which in turn calls unsafe reinit_thread_mutex_after_fork, preventing accidental
safe usage elsewhere.
BufferedReader/Writer/TextIOWrapper use PyThreadMutex internally. If a parent thread held one of these locks during fork(), the child would deadlock on any IO operation. Add reinit_after_fork() to RawThreadMutex and call it on sys.stdin/ stdout/stderr in the child process fork handler, analogous to CPython's _PyIO_Reinit().
- Mark reinit_std_streams_after_fork as unsafe fn to encode fork-only precondition, update call site in posix.rs - Reinit IncrementalNewlineDecoder's PyThreadMutex via TextIOWrapper's decoder field to prevent child deadlocks
1 participant
BufferedReader/Writer/TextIOWrapper use PyThreadMutex internally. If a parent thread held one of these locks during fork(), the child would deadlock on any IO operation.
Add reinit_after_fork() to RawThreadMutex and call it on sys.stdin/ stdout/stderr in the child process fork handler, analogous to CPython's _PyIO_Reinit().
Summary by CodeRabbit