Reinit IO buffer locks after fork to prevent deadlocks#7339
Conversation
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin io-fork |
Sorry, something went wrong.
![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".
Sorry, something went wrong.
![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
Sorry, something went wrong.
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
youknowone
merged commit c55a9ff
into
RustPython:main
Mar 4, 2026
* Reinit IO buffer locks after fork to prevent deadlocks 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(). * Address review: unsafe fn + decoder lock 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 * Auto-format: cargo fmt --all --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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