Fix _at_fork_reinit to write INIT directly instead of calling unlock() #7312

youknowone

Summary by CodeRabbit

Bug Fixes
- Improved fork-safety: import and interpreter locks are reinitialized in child processes to avoid deadlocks or stalled state after fork in threaded builds.
- Lock reinitialization logic streamlined to ensure consistent unlocked/initialized state in the child, reducing post-fork lock-related failures.

coderabbitai

📝 Walkthrough

Walkthrough

Adds Unix-only fork-safety primitives that reset mutex state in the child after fork: a low-level routine to zero a PyMutex, an import-lock reinitialization path invoked post-fork, and changes to thread lock reinit handlers to atomically reset mutexes instead of unlocking.

Changes

Cohort / File(s)	Summary
Common lock primitive `crates/common/src/lock.rs`	Add `pub unsafe fn reinit_mutex_after_fork<T: ?Sized>(mutex: &PyMutex<T>)` (Unix-only). It zeroes the raw mutex bytes to force an unlocked/initial state for use immediately in the child post-fork.
Import lock reinit `crates/vm/src/stdlib/imp.rs`	Add `pub(crate) unsafe fn reinit_after_fork()` inside `lock` and a `pub(crate) unsafe fn reinit_imp_lock_after_fork()` reexport (Unix + threading gated). Uses low-level swap/zero to reset IMP_LOCK when owning thread came from the parent.
Post-fork hook callsite `crates/vm/src/stdlib/posix.rs`	Invoke `reinit_imp_lock_after_fork()` from `py_os_after_fork_child` when threading feature is enabled to reinitialize the import lock in the child.
Thread lock reinitialization `crates/vm/src/stdlib/thread.rs`	Replace prior unlock-based at-fork reinit with an atomic swap to `RawMutex::INIT` for Lock and RLock handlers; avoid calling `unlock()` after fork to not interact with dead-parent waiter state.

Sequence Diagram(s)

sequenceDiagram
    participant Parent as Parent Process
    participant OS as Operating System
    participant Child as Child Process
    participant ImpLock as Import Lock
    participant ThreadLocks as Thread Locks

    Parent->>OS: fork()
    OS->>Child: child process created
    Child->>Child: py_os_after_fork_child()
    Child->>ImpLock: reinit_imp_lock_after_fork()
    ImpLock->>ImpLock: inspect owner (dead parent thread?)
    ImpLock->>ImpLock: reinit_mutex_after_fork() (zero raw mutex)
    Child->>ThreadLocks: run _at_fork_reinit handlers
    ThreadLocks->>ThreadLocks: atomic swap RawMutex::INIT
    Child->>Child: resume execution

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Use try_lock in py_os_after_fork_child #7178: Modifies post-fork behavior in py_os_after_fork_child around import-lock handling and fork-child locking patterns.
force_unlock_after_fork #7186: Implements fork-time lock recovery across stdlib modules; overlaps in strategies for resetting locks after fork.
various fix to support subprocess/multiprocessing #6567: Changes at-fork reinitialization in thread.rs, focusing on resetting mutex/reentrancy state after fork.

Poem

🐇 I hopped through code where forks divide,
Parent locks asleep, child opens wide.
I nudge the mutex, set INIT for fun,
No tangled waits — the new day’s begun! ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and accurately summarizes the main change: replacing unlock() calls with direct INIT writes in _at_fork_reinit handlers across multiple files.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

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/imp.rs`:
- Around line 47-64: The unsafe reinit_after_fork() inspects and zeroes IMP_LOCK
via layout-dependent casts to AtomicUsize/bytes; replace this with the safer
pattern used in RLock::after_fork_child: set IMP_LOCK to RawRMutex::INIT
atomically (e.g., via AtomicCell::swap) instead of casting/ write_bytes, or else
use the public RawRMutex methods (is_locked()/is_owned_by_current_thread()) to
decide whether to reset via the public API; update reinit_after_fork to avoid
direct field access of RawRMutex and rely on AtomicCell::swap(RawRMutex::INIT)
or public methods to reset state.

ℹ️ Review info

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between baba1f9 and 9be0543.

📒 Files selected for processing (4)

crates/common/src/lock.rs
crates/vm/src/stdlib/imp.rs
crates/vm/src/stdlib/posix.rs
crates/vm/src/stdlib/thread.rs

unlock() goes through unlock_slow() which accesses parking_lot's global hash table to unpark waiters. After fork(), this hash table contains stale entries from dead parent threads, making unlock_slow() unsafe. Writing INIT directly bypasses parking_lot internals entirely.

The import lock is a ReentrantMutex that was never reinit'd after fork(). If a parent thread held it during fork, the child would deadlock on any import. Only reset if the owner is a dead thread; if the surviving thread held it, normal unlock still works.

coderabbitai

Actionable comments posted: 1

♻️ Duplicate comments (1)

crates/vm/src/stdlib/imp.rs (1)

47-55: ⚠️ Potential issue | 🟠 Major

transmute(&IMP_LOCK) to AtomicCell<RawRMutex> is still representation-dependent unsafe.

This cast assumes undocumented compatibility between RawRMutex and AtomicCell<RawRMutex>. If either crate changes layout/aliasing semantics, this can become UB in a critical fork-reinit path.

Is it sound Rust to transmute `&T` into `&crossbeam_utils::atomic::AtomicCell<T>` and call `swap`? Does `AtomicCell<T>` guarantee representation compatibility with `T` across versions?

As per coding guidelines "Follow Rust best practices for error handling and memory management".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/stdlib/imp.rs` around lines 47 - 55, The current unsafe cast
from &IMP_LOCK to &crossbeam_utils::atomic::AtomicCell<RawRMutex> in
reinit_after_fork is representation-dependent and unsound; instead, make the
lock representation explicitly an AtomicCell so you don't transmute: change the
static IMP_LOCK (or introduce a new static wrapper) to be a
crossbeam_utils::atomic::AtomicCell<RawRMutex> and then call AtomicCell::swap
directly (e.g., use IMP_LOCK.swap(RawRMutex::INIT) inside reinit_after_fork), or
if changing the static type is not possible, create a properly typed AtomicCell
static alias that is initialized from the same initial value and use that—avoid
transmute of &IMP_LOCK to AtomicCell entirely and refer to IMP_LOCK,
reinit_after_fork, RawRMutex, and AtomicCell::swap in the fix.

🧹 Nitpick comments (1)

crates/vm/src/stdlib/imp.rs (1)

60-63: Add a short # Safety doc for the crate-visible unsafe wrapper.

Document caller obligations directly on reinit_imp_lock_after_fork() to keep the fork contract explicit at the call boundary.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/stdlib/imp.rs` around lines 60 - 63, Add a short `# Safety` doc
on the crate-visible unsafe wrapper `reinit_imp_lock_after_fork()` that states
the caller obligations: that the function is unsafe because it must only be
invoked in the post-fork child when no other threads are running (or otherwise
guarantee single-threadedness), that it must not be called from contexts that
invoke non-async-signal-safe operations, and that it simply forwards to
`lock::reinit_after_fork()` to reinitialize internal locks; keep it concise and
place it immediately above the `pub(crate) unsafe fn
reinit_imp_lock_after_fork()` declaration.

🤖 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/common/src/lock.rs`:
- Around line 72-79: reinit_mutex_after_fork currently performs raw byte writes
assuming the raw mutex is at offset 0 and all-zero equals initial state; update
it to call the unsafe Mutex::raw() accessor on the PyMutex to get a &RawMutex
and reinitialize by writing RawMutex::INIT into that memory (use
core::ptr::write to overwrite the raw lock with RawMutex::INIT) so you no longer
depend on layout or zero bytes; target the function reinit_mutex_after_fork and
the PyMutex type and use RawMutex::INIT (or the corresponding raw type's INIT)
when performing the write.

---

Duplicate comments:
In `@crates/vm/src/stdlib/imp.rs`:
- Around line 47-55: The current unsafe cast from &IMP_LOCK to
&crossbeam_utils::atomic::AtomicCell<RawRMutex> in reinit_after_fork is
representation-dependent and unsound; instead, make the lock representation
explicitly an AtomicCell so you don't transmute: change the static IMP_LOCK (or
introduce a new static wrapper) to be a
crossbeam_utils::atomic::AtomicCell<RawRMutex> and then call AtomicCell::swap
directly (e.g., use IMP_LOCK.swap(RawRMutex::INIT) inside reinit_after_fork), or
if changing the static type is not possible, create a properly typed AtomicCell
static alias that is initialized from the same initial value and use that—avoid
transmute of &IMP_LOCK to AtomicCell entirely and refer to IMP_LOCK,
reinit_after_fork, RawRMutex, and AtomicCell::swap in the fix.

---

Nitpick comments:
In `@crates/vm/src/stdlib/imp.rs`:
- Around line 60-63: Add a short `# Safety` doc on the crate-visible unsafe
wrapper `reinit_imp_lock_after_fork()` that states the caller obligations: that
the function is unsafe because it must only be invoked in the post-fork child
when no other threads are running (or otherwise guarantee single-threadedness),
that it must not be called from contexts that invoke non-async-signal-safe
operations, and that it simply forwards to `lock::reinit_after_fork()` to
reinitialize internal locks; keep it concise and place it immediately above the
`pub(crate) unsafe fn reinit_imp_lock_after_fork()` declaration.

ℹ️ Review info

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9be0543 and 2893430.

📒 Files selected for processing (4)

crates/common/src/lock.rs
crates/vm/src/stdlib/imp.rs
crates/vm/src/stdlib/posix.rs
crates/vm/src/stdlib/thread.rs

🚧 Files skipped from review as they are similar to previous changes (1)

crates/vm/src/stdlib/thread.rs

…() (RustPython#7312) * Fix _at_fork_reinit to write INIT directly instead of calling unlock() unlock() goes through unlock_slow() which accesses parking_lot's global hash table to unpark waiters. After fork(), this hash table contains stale entries from dead parent threads, making unlock_slow() unsafe. Writing INIT directly bypasses parking_lot internals entirely. * Add import lock (IMP_LOCK) reinit after fork The import lock is a ReentrantMutex that was never reinit'd after fork(). If a parent thread held it during fork, the child would deadlock on any import. Only reset if the owner is a dead thread; if the surviving thread held it, normal unlock still works.

youknowone marked this pull request as ready for review March 2, 2026 11:30

coderabbitai Bot reviewed Mar 2, 2026

View reviewed changes

youknowone added 2 commits March 2, 2026 20:55

youknowone force-pushed the fork-reinit branch from 9be0543 to 2893430 Compare March 2, 2026 11:56

coderabbitai Bot reviewed Mar 2, 2026

View reviewed changes

youknowone deleted the fork-reinit branch March 3, 2026 00:08

This was referenced Mar 3, 2026

reinit-after-fork #7321

Merged

Add monitoring lock reinit and use raw() accessor #7335

Merged

coderabbitai Bot mentioned this pull request Mar 23, 2026

Add global type mutex for thread-safe type operations #7416

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix _at_fork_reinit to write INIT directly instead of calling unlock() #7312