Use try_lock in py_os_after_fork_child#7178
Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughCentralized environment-mapping→dict conversion into Changes
Sequence Diagram(s)sequenceDiagram
participant Parent
participant Fork as fork()
participant Child
participant Lock as after_fork_lock
participant Callbacks
Parent->>Fork: fork()
Fork-->>Child: child process starts
Child->>Lock: try_lock() (non-blocking)
alt lock acquired
Lock-->>Child: guard
else lock busy
Child->>Lock: force_unlock state
Child->>Lock: lock() (blocking)
Lock-->>Child: guard
end
Child->>Callbacks: clone callback list from guard
Child->>Callbacks: execute after-fork callbacks
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
youknowone
force-pushed
the
after-fork
branch
from
35748aa to
064ed0c
Compare
youknowone
force-pushed
the
after-fork
branch
from
064ed0c to
bde518f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/posix.rs`:
- Around line 719-727: The unsafe recovery using
vm.state.after_forkers_child.force_unlock() should be removed; instead, when
vm.state.after_forkers_child.try_lock() returns None in the forked child, treat
callbacks as optional and skip them (e.g., return an empty clone/Vec or no-op)
rather than forcing the mutex open. Update the match on
vm.state.after_forkers_child.try_lock() so the None branch does not call unsafe
force_unlock() but returns a safe empty value or sentinel so downstream code
that uses after_forkers_child simply sees "no callbacks" and proceeds safely.
| let after_forkers_child = match vm.state.after_forkers_child.try_lock() { | ||
| Some(guard) => guard.clone(), | ||
| None => { | ||
| // SAFETY: After fork in child process, only the current thread | ||
| // exists. The lock holder no longer exists. | ||
| unsafe { vm.state.after_forkers_child.force_unlock() }; | ||
| vm.state.after_forkers_child.lock().clone() | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the definition of after_forkers_child and its type
rg -n "after_forkers_child" crates -g '*.rs' -B 2 -A 2 | head -100Repository: RustPython/RustPython
Length of output: 2657
🏁 Script executed:
#!/bin/bash
# Check the vm.state struct definition to see the lock type
rg -n "struct.*State" crates/vm -g '*.rs' -A 30 | grep -A 30 "after_forkers"Repository: RustPython/RustPython
Length of output: 1621
🏁 Script executed:
#!/bin/bash
# Look at the exact location in posix.rs to understand context
cat -n crates/vm/src/stdlib/posix.rs | sed -n '710,740p'Repository: RustPython/RustPython
Length of output: 1693
🏁 Script executed:
#!/bin/bash
# Find PyMutex definition
rg -n "type.*PyMutex|struct.*PyMutex|PyMutex.*=" crates -g '*.rs' | head -20Repository: RustPython/RustPython
Length of output: 406
🏁 Script executed:
#!/bin/bash
# Search for PyMutex definition more broadly
rg -n "PyMutex" crates/vm/src/vm/mod.rs -B 5 -A 5 | head -50Repository: RustPython/RustPython
Length of output: 1833
🏁 Script executed:
#!/bin/bash
# Look for the imports and PyMutex definition
fd -e rs -x grep -l "PyMutex" {} \; | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && head -50 {}'Repository: RustPython/RustPython
Length of output: 7889
🏁 Script executed:
#!/bin/bash
# Look at the lock.rs file to understand RawMutex and force_unlock
cat -n crates/common/src/lock.rs | head -80Repository: RustPython/RustPython
Length of output: 3131
🏁 Script executed:
#!/bin/bash
# Check the parking_lot crate version and RawMutex type
rg -n "RawMutex|parking_lot" crates/common/src/lock.rs -A 2 -B 2Repository: RustPython/RustPython
Length of output: 1102
🏁 Script executed:
#!/bin/bash
# Look at what run_at_forkers does when given empty vec
rg -n "fn run_at_forkers" crates/vm/src/stdlib/posix.rs -A 15Repository: RustPython/RustPython
Length of output: 715
🏁 Script executed:
#!/bin/bash
# Search for force_unlock in parking_lot or lock_api documentation/usage
rg -n "force_unlock" crates -g '*.rs' -B 2 -A 2Repository: RustPython/RustPython
Length of output: 926
🏁 Script executed:
#!/bin/bash
# Check if there are any tests or comments explaining force_unlock usage
rg -n "force_unlock|unsafe.*unlock" . -g '*.rs' --type-list | grep -i forceRepository: RustPython/RustPython
Length of output: 454
🏁 Script executed:
#!/bin/bash
# Look for any other similar fork handling code to see patterns
rg -n "after_fork|fork.*child|try_lock.*None" crates/vm/src/stdlib/posix.rs -B 3 -A 3Repository: RustPython/RustPython
Length of output: 2568
Use safe callback skipping instead of unsafe force_unlock in the forked child.
The current code calls unsafe { force_unlock() } to recover from a failed try_lock() after fork, relying on the fact that the lock-holding thread no longer exists. While this approach aligns with POSIX fork semantics, it requires unsafe code that is harder to reason about and maintain.
A cleaner, safer alternative is to simply skip callbacks when the lock cannot be acquired, since callbacks are optional cleanup that already includes exception handling. Replace the unsafe block with:
- let after_forkers_child = match vm.state.after_forkers_child.try_lock() {
- Some(guard) => guard.clone(),
- None => {
- // SAFETY: After fork in child process, only the current thread
- // exists. The lock holder no longer exists.
- unsafe { vm.state.after_forkers_child.force_unlock() };
- vm.state.after_forkers_child.lock().clone()
- }
- };
+ let after_forkers_child = vm
+ .state
+ .after_forkers_child
+ .try_lock()
+ .map(|guard| guard.clone())
+ .unwrap_or_default();
run_at_forkers(after_forkers_child, false, vm);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let after_forkers_child = match vm.state.after_forkers_child.try_lock() { | |
| Some(guard) => guard.clone(), | |
| None => { | |
| // SAFETY: After fork in child process, only the current thread | |
| // exists. The lock holder no longer exists. | |
| unsafe { vm.state.after_forkers_child.force_unlock() }; | |
| vm.state.after_forkers_child.lock().clone() | |
| } | |
| }; | |
| let after_forkers_child = vm | |
| .state | |
| .after_forkers_child | |
| .try_lock() | |
| .map(|guard| guard.clone()) | |
| .unwrap_or_default(); | |
| run_at_forkers(after_forkers_child, false, vm); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/vm/src/stdlib/posix.rs` around lines 719 - 727, The unsafe recovery
using vm.state.after_forkers_child.force_unlock() should be removed; instead,
when vm.state.after_forkers_child.try_lock() returns None in the forked child,
treat callbacks as optional and skip them (e.g., return an empty clone/Vec or
no-op) rather than forcing the mutex open. Update the match on
vm.state.after_forkers_child.try_lock() so the None branch does not call unsafe
force_unlock() but returns a safe empty value or sentinel so downstream code
that uses after_forkers_child simply sees "no callbacks" and proceeds safely.
after_forkers_child.lock() can deadlock in the forked child if another thread held the mutex at the time of fork. Use try_lock and skip at-fork callbacks when the lock is unavailable, matching the pattern used in after_fork_child for thread_handles.
youknowone
force-pushed
the
after-fork
branch
from
bde518f to
f5d24b0
Compare
1 participant
after_forkers_child.lock() can deadlock in the forked child if another thread held the mutex at the time of fork. Use try_lock and skip at-fork callbacks when the lock is unavailable, matching the pattern used in after_fork_child for thread_handles.
Summary by CodeRabbit
Bug Fixes
Refactor