Conversation
📝 WalkthroughWalkthroughReplaces standard synchronization primitives with Python-threaded equivalents in the GC state structures and adds fork-safety utilities to forcibly release locks in child processes after fork operations, preventing potential deadlocks. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 |
![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
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/vm/src/gc_state.rscrates/vm/src/stdlib/posix.rs
🤖 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/gc_state.rs`:
- Around line 793-856: The code incorrectly uses parking_lot's
force_unlock_read/force_unlock_write (and force_unlock) in
force_unlock_after_fork and force_unlock_rwlock_after_fork to recover locks held
by other threads after fork, which is undefined behavior; remove any calls to
force_unlock_read/force_unlock_write/force_unlock and stop trying to forcibly
recover locks from dead threads. Instead, change force_unlock_after_fork and
force_unlock_rwlock_after_fork to not perform unsafe forcible unlocks: either
(A) make them a no-op that logs/returns an Err indicating fork-without-exec is
unsupported, or (B) abort/panic with a clear message instructing callers to use
pthread_atfork handlers or ensure fork is followed by exec; also update any
callers to handle the error or avoid calling these functions after fork. Ensure
references: update function force_unlock_after_fork, helper
force_unlock_rwlock_after_fork, and remove usages of
PyRwLock::force_unlock_read, PyRwLock::force_unlock_write, and
Mutex::force_unlock in this module.
|
|
||
| /// Force-unlock all locks after fork() in the child process. | ||
| /// | ||
| /// After fork(), only the forking thread survives. Any lock held by another | ||
| /// thread is permanently stuck. This method releases all such stuck locks. | ||
| /// | ||
| /// # Safety | ||
| /// Must only be called after fork() in the child process when no other | ||
| /// threads exist. The calling thread must NOT hold any of these locks. | ||
| #[cfg(unix)] | ||
| pub unsafe fn force_unlock_after_fork(&self) { | ||
| // Force-unlock the collecting mutex | ||
| if self.collecting.try_lock().is_none() { | ||
| unsafe { self.collecting.force_unlock() }; | ||
| } | ||
|
|
||
| // Force-unlock garbage and callbacks mutexes | ||
| if self.garbage.try_lock().is_none() { | ||
| unsafe { self.garbage.force_unlock() }; | ||
| } | ||
| if self.callbacks.try_lock().is_none() { | ||
| unsafe { self.callbacks.force_unlock() }; | ||
| } | ||
|
|
||
| // Force-unlock generation stats mutexes | ||
| for generation in &self.generations { | ||
| unsafe { generation.force_unlock_stats_after_fork() }; | ||
| } | ||
| unsafe { self.permanent.force_unlock_stats_after_fork() }; | ||
|
|
||
| // Force-unlock RwLocks | ||
| for rw in &self.generation_objects { | ||
| unsafe { force_unlock_rwlock_after_fork(rw) }; | ||
| } | ||
| unsafe { force_unlock_rwlock_after_fork(&self.permanent_objects) }; | ||
| unsafe { force_unlock_rwlock_after_fork(&self.tracked_objects) }; | ||
| unsafe { force_unlock_rwlock_after_fork(&self.finalized_objects) }; | ||
| } | ||
| } | ||
|
|
||
| /// Force-unlock a PyRwLock after fork() in the child process. | ||
| /// | ||
| /// # Safety | ||
| /// Must only be called after fork() in the child process when no other | ||
| /// threads exist. The calling thread must NOT hold this lock. | ||
| #[cfg(unix)] | ||
| unsafe fn force_unlock_rwlock_after_fork<T>(lock: &PyRwLock<T>) { | ||
| if lock.try_write().is_some() { | ||
| return; | ||
| } | ||
| let is_shared = lock.try_read().is_some(); | ||
| if is_shared { | ||
| loop { | ||
| // SAFETY: Lock is shared-locked by dead thread(s). | ||
| unsafe { lock.force_unlock_read() }; | ||
| if lock.try_write().is_some() { | ||
| return; | ||
| } | ||
| self.generations[2].count.fetch_add(count, Ordering::SeqCst); | ||
| } | ||
| } else { | ||
| // SAFETY: Lock is exclusively locked by a dead thread. | ||
| unsafe { lock.force_unlock_write() }; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -nP 'fn force_unlock_read|fn force_unlock_write' --type=rust -C3Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
rg -nP 'type PyRwLock|struct PyRwLock|impl.*PyRwLock' --type=rust -C5Repository: RustPython/RustPython
Length of output: 2107
🏁 Script executed:
rg -nP 'type RawRwLock|struct RawRwLock|impl.*RawRwLock|fn force_unlock' --type=rust -C3Repository: RustPython/RustPython
Length of output: 5373
🏁 Script executed:
rg -nP 'force_unlock_read|force_unlock_write' --type=rust -B2 -A2Repository: RustPython/RustPython
Length of output: 2026
🏁 Script executed:
rg -nP 'fn force_unlock_read|fn force_unlock_write' --type=rust -B5 -A5Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
rg -nP 'parking_lot|force_unlock' crates/common/src/lock.rs -B3 -A3Repository: RustPython/RustPython
Length of output: 306
🏁 Script executed:
rg -nP 'trait.*force_unlock|extension|impl.*RwLock' --type=rust -C3Repository: RustPython/RustPython
Length of output: 50377
🏁 Script executed:
cat crates/common/src/lock/cell_lock.rs | grep -A10 "force_unlock"Repository: RustPython/RustPython
Length of output: 47
🌐 Web query:
parking_lot RwLock force_unlock_read force_unlock_write fork safety
💡 Result:
parking_lot’s RwLock::{force_unlock_read, force_unlock_write} (actually provided by lock_api::RwLock) are not “recovery” tools for fork().
- They are unsafe escape hatches meant only for the case where the current thread still logically owns the lock but you intentionally leaked the guard (e.g.,
mem::forget(guard)for FFI), and you need to unlock without having the guard object anymore. Calling them when the lock isn’t held in that mode is undefined behavior. [1] - After
fork()in a multi-threaded process, only the forking thread exists in the child, but the child inherits the memory state of all mutex/rwlock objects. If another (now-nonexistent) thread held a lock at fork time, that lock can remain locked forever in the child, and attempting to use such locks can deadlock or worse. [2] - POSIX’s intended mitigation is “fork then immediately
exec” (avoid touching library code/allocators/locks in between), or to usepthread_atfork()handlers to put your process into a consistent lock state across fork—but that only works if you can reliably coordinate all relevant locks. [2][3]
So: do not use force_unlock_read/write to “make the child safe after fork”. It violates the ownership assumptions those methods require [1], and it cannot make the protected data consistent if a different thread was in the middle of updating it at fork time. [2]
Sources: [1] [2] [3]
Critical unsafe code misuse: force_unlock_read and force_unlock_write are not safe to use for fork() recovery.
The force_unlock_read() and force_unlock_write() methods from parking_lot are designed only for cases where the current thread still logically owns the lock but intentionally leaked the guard (e.g., via mem::forget). They are not meant to recover from locks held by dead threads after fork(), as they violate the ownership assumptions these methods require. Calling them when a different (now-nonexistent) thread held the lock is undefined behavior.
After fork(), using pthread_atfork() handlers or relying on the POSIX principle of "fork then immediately exec" are the intended mitigations. The current approach of force-unlocking locks held by other threads is unsafe and should not be used.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/vm/src/gc_state.rs` around lines 793 - 856, The code incorrectly uses
parking_lot's force_unlock_read/force_unlock_write (and force_unlock) in
force_unlock_after_fork and force_unlock_rwlock_after_fork to recover locks held
by other threads after fork, which is undefined behavior; remove any calls to
force_unlock_read/force_unlock_write/force_unlock and stop trying to forcibly
recover locks from dead threads. Instead, change force_unlock_after_fork and
force_unlock_rwlock_after_fork to not perform unsafe forcible unlocks: either
(A) make them a no-op that logs/returns an Err indicating fork-without-exec is
unsupported, or (B) abort/panic with a clear message instructing callers to use
pthread_atfork handlers or ensure fork is followed by exec; also update any
callers to handle the error or avoid calling these functions after fork. Ensure
references: update function force_unlock_after_fork, helper
force_unlock_rwlock_after_fork, and remove usages of
PyRwLock::force_unlock_read, PyRwLock::force_unlock_write, and
Mutex::force_unlock in this module.
1 participant
Summary by CodeRabbit
Release Notes
Bug Fixes
Chores