Skip to content

Commit 0bbf971

Browse files
youknowoneyouknowone
committed
vm: keep the interpreter registry usable across bootstrap, drop and fork
Three gaps where the registry did not describe the interpreters that actually exist, each of which hides an interpreter from the collector's stop-the-world. Register before `initialize()`. Registration ran as the last step of `initialize_vm`, so the whole bootstrap — which executes Python bytecode and allocates GC-tracked objects — was invisible to `live_interpreter_states()`. It still cannot run any earlier than this: the init hooks take `PyRc::get_mut` on the state, which fails as soon as the registry holds a weak reference to it. Stop unregistering in `Interpreter::drop`. The handle does not decide the interpreter's lifetime — every `ThreadedVirtualMachine` from `new_thread()` holds its own `PyRc<PyGlobalState>` — so an interpreter with running workers disappeared from the registry while its threads kept mutating the object graph. The entries are weak, so lifetime is already correct without the removal; dead entries are now reaped when registering instead. Interpreters are consequently released rather than unregistered at a fixed point, so the two tests asserting disappearance now wait for it: a collection in progress legitimately holds a reference to every live interpreter. Repair other interpreters after fork. `py_os_after_fork_child` only fixed the forking interpreter, leaving every other one with slots for threads that did not survive (still ATTACHED if they were running bytecode) plus locks and stop-the-world flags held by them. Since a collection stops all interpreters, the child's first collection would wait for threads that no longer exist. Reset their locks, stop-the-world state and thread tables, drop this thread's cached slots for them, and reinit the registry's own locks first, since enumerating interpreters now takes them. Tests: test_gc, test_threading and test_fork1 pass, as do the vm tests in both the threading and default configurations. Assisted-by: Claude Code:claude-opus-4-8
1 parent c1ba891 commit 0bbf971

4 files changed

Lines changed: 126 additions & 16 deletions

File tree

‎crates/vm/src/stdlib/posix.rs‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -630,6 +630,13 @@ pub mod module {
630630
}
631631

632632
fn py_os_after_fork_child(vm: &VirtualMachine) {
633+
// The interpreter registry is reachable from every thread, so repair it
634+
// before anything enumerates interpreters.
635+
#[cfg(all(unix, feature = "threading"))]
636+
unsafe {
637+
crate::vm::runtime::reinit_after_fork()
638+
};
639+
633640
#[cfg(feature = "threading")]
634641
vm.state.stop_the_world.reset_after_fork();
635642

@@ -639,6 +646,12 @@ pub mod module {
639646
#[cfg(feature = "threading")]
640647
reinit_locks_after_fork(vm);
641648

649+
// The collector stops every interpreter, so interpreters other than the
650+
// forking one must be repaired too; otherwise the child's first
651+
// collection waits for threads that did not survive the fork.
652+
#[cfg(all(unix, feature = "threading"))]
653+
reinit_other_interpreters_after_fork(vm);
654+
642655
// Reinit per-object IO buffer locks on std streams.
643656
// BufferedReader/Writer/TextIOWrapper use PyThreadMutex which can be
644657
// held by dead parent threads, causing deadlocks on any IO in the child.
@@ -727,6 +740,53 @@ pub mod module {
727740
}
728741
}
729742

743+
/// Repair every live interpreter other than the forking one after `fork()`.
744+
///
745+
/// Only the forking thread survives, so each other interpreter is left with
746+
/// slots for threads that no longer exist (still ATTACHED if they were
747+
/// running bytecode) and possibly locks or stop-the-world flags held by
748+
/// them. Since a collection stops all interpreters, that state would hang
749+
/// the child's first collection.
750+
///
751+
/// # Safety
752+
/// Must only be called after `fork()` in the child, when no other threads exist.
753+
#[cfg(all(unix, feature = "threading"))]
754+
fn reinit_other_interpreters_after_fork(vm: &VirtualMachine) {
755+
use rustpython_common::lock::reinit_mutex_after_fork;
756+
757+
for state in crate::vm::runtime::live_interpreter_states() {
758+
if state.interpreter_id == vm.state.interpreter_id {
759+
continue;
760+
}
761+
762+
unsafe {
763+
reinit_mutex_after_fork(&state.before_forkers);
764+
reinit_mutex_after_fork(&state.after_forkers_child);
765+
reinit_mutex_after_fork(&state.after_forkers_parent);
766+
reinit_mutex_after_fork(&state.atexit_funcs);
767+
reinit_mutex_after_fork(&state.global_trace_func);
768+
reinit_mutex_after_fork(&state.global_profile_func);
769+
reinit_mutex_after_fork(&state.type_mutex);
770+
reinit_mutex_after_fork(&state.monitoring);
771+
reinit_mutex_after_fork(&state.thread_frames);
772+
reinit_mutex_after_fork(&state.thread_handles);
773+
reinit_mutex_after_fork(&state.shutdown_handles);
774+
775+
state.codec_registry.reinit_after_fork();
776+
}
777+
778+
state.stop_the_world.reset_after_fork();
779+
780+
// Every thread registered here belongs to the parent, including any
781+
// slot the forking thread itself registered before the fork.
782+
state.thread_frames.lock().clear();
783+
state.thread_handles.lock().clear();
784+
state.shutdown_handles.lock().clear();
785+
}
786+
787+
crate::vm::thread::purge_other_interpreter_slots_after_fork(vm.state.interpreter_id);
788+
}
789+
730790
fn py_os_after_fork_parent(vm: &VirtualMachine) {
731791
#[cfg(feature = "threading")]
732792
vm.state.stop_the_world.start_the_world(&vm.state);

‎crates/vm/src/vm/interpreter.rs‎

Lines changed: 31 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -200,6 +200,13 @@ where
200200
// Call custom init function (can mutate vm.state)
201201
init(&mut vm);
202202

203+
// Register before `initialize()` runs any Python: it allocates GC-tracked
204+
// objects, so a collection on another thread has to be able to stop this
205+
// interpreter while that happens. It cannot be registered earlier — the
206+
// hooks above take `PyRc::get_mut` on the state, which fails once the
207+
// registry holds a weak reference to it.
208+
runtime::register_interpreter(&vm.state);
209+
203210
// `initialize()` runs Python bytecode directly (e.g. importing `codecs`
204211
// and `encodings`) before any `enter_vm` scope exists, so attach this
205212
// thread for the duration so type cache reads see it as ATTACHED.
@@ -209,7 +216,6 @@ where
209216

210217
// Clone global_state for Interpreter after all initialization is done
211218
let global_state = vm.state.clone();
212-
runtime::register_interpreter(&global_state);
213219
(vm, global_state)
214220
}
215221

@@ -380,12 +386,6 @@ pub struct Interpreter {
380386
vm: VirtualMachine,
381387
}
382388

383-
impl Drop for Interpreter {
384-
fn drop(&mut self) {
385-
runtime::unregister_interpreter(self.global_state.interpreter_id);
386-
}
387-
}
388-
389389
impl Interpreter {
390390
/// Create a new interpreter configuration builder.
391391
///
@@ -812,7 +812,27 @@ mod tests {
812812
assert!(ids.contains(&sub2.id()));
813813
}
814814

815-
/// Dropping a subinterpreter unregisters it; main remains.
815+
/// An interpreter stays looked-up-able until nothing holds its state.
816+
///
817+
/// Dropping the handle is not the end of its life: `new_thread()` workers
818+
/// hold their own reference, and a collection in progress holds one for
819+
/// every live interpreter while the world is stopped. So the registry entry
820+
/// goes away eventually rather than at the drop.
821+
fn wait_until_unregistered(id: i64) {
822+
use core::time::Duration;
823+
use std::time::Instant;
824+
825+
let deadline = Instant::now() + Duration::from_secs(5);
826+
while runtime::lookup_interpreter(id).is_some() {
827+
assert!(
828+
Instant::now() < deadline,
829+
"interpreter {id} still registered long after its last reference"
830+
);
831+
std::thread::yield_now();
832+
}
833+
}
834+
835+
/// Dropping a subinterpreter releases it; main remains.
816836
#[test]
817837
fn drop_subinterpreter_unregisters() {
818838
let main = Interpreter::without_stdlib(Default::default());
@@ -822,7 +842,7 @@ mod tests {
822842
assert!(runtime::lookup_interpreter(id).is_some());
823843
id
824844
};
825-
assert!(runtime::lookup_interpreter(sub_id).is_none());
845+
wait_until_unregistered(sub_id);
826846
assert!(runtime::lookup_interpreter(main.id()).is_some());
827847
}
828848

@@ -1203,9 +1223,9 @@ mod tests {
12031223
assert!(runtime::lookup_interpreter(id).is_some());
12041224
assert!(runtime::take_owned_interpreter(id).is_none());
12051225

1206-
// Dropping the reclaimed handle unregisters it.
1226+
// Dropping the reclaimed handle releases it.
12071227
drop(reclaimed);
1208-
assert!(runtime::lookup_interpreter(id).is_none());
1228+
wait_until_unregistered(id);
12091229
}
12101230

12111231
/// `create_owned_subinterpreter` stores the sub and returns only its id.

‎crates/vm/src/vm/runtime.rs‎

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,11 @@ pub(crate) fn register_interpreter(state: &PyRc<PyGlobalState>) {
150150
);
151151
}
152152
let mut entries = registry().entries.lock();
153+
// Entries are weak and an interpreter's lifetime is decided by its last
154+
// `PyRc<PyGlobalState>` — which outlives the `Interpreter` handle whenever
155+
// `new_thread()` workers are still running — so nothing removes them at a
156+
// fixed point. Reap the dead ones here to bound the table instead.
157+
entries.retain(|_, entry| entry.state.strong_count() > 0);
153158
entries.insert(
154159
id,
155160
RegistryEntry {
@@ -159,11 +164,6 @@ pub(crate) fn register_interpreter(state: &PyRc<PyGlobalState>) {
159164
);
160165
}
161166

162-
/// Unregister an interpreter (called when its owning `Interpreter` is dropped).
163-
pub(crate) fn unregister_interpreter(id: i64) {
164-
registry().entries.lock().remove(&id);
165-
}
166-
167167
/// Look up a live interpreter state by id.
168168
#[must_use]
169169
pub fn lookup_interpreter(id: i64) -> Option<PyRc<PyGlobalState>> {
@@ -198,6 +198,23 @@ pub fn interpreter_count() -> usize {
198198
list_interpreters().len()
199199
}
200200

201+
/// Reset the registry's locks after `fork()`.
202+
///
203+
/// The tables are reachable from every thread, so a thread that died in the
204+
/// fork may have left one locked; the child would then deadlock the first time
205+
/// it enumerates interpreters (which the collector now does on every stop).
206+
///
207+
/// # Safety
208+
/// Must only be called after `fork()` in the child process, when no other
209+
/// threads exist and the calling thread holds neither lock.
210+
#[cfg(all(unix, feature = "threading"))]
211+
pub unsafe fn reinit_after_fork() {
212+
unsafe {
213+
crate::common::lock::reinit_mutex_after_fork(&registry().entries);
214+
crate::common::lock::reinit_mutex_after_fork(owned_interpreters());
215+
}
216+
}
217+
201218
/// All live interpreter states, ordered by id.
202219
///
203220
/// Used by the cyclic collector, which must stop every interpreter's threads

‎crates/vm/src/vm/thread.rs‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -946,6 +946,19 @@ pub fn reinit_frame_slot_after_fork(vm: &VirtualMachine) {
946946
});
947947
}
948948

949+
/// Drop this thread's cached slots for every interpreter except `keep_id`.
950+
///
951+
/// After `fork()` only the calling thread survives, and the other
952+
/// interpreters' registries are cleared; a cached slot would otherwise stay
953+
/// current for an interpreter that no longer lists it, hiding the thread from
954+
/// that interpreter's stop-the-world. The next enter builds a fresh slot.
955+
#[cfg(feature = "threading")]
956+
pub fn purge_other_interpreter_slots_after_fork(keep_id: i64) {
957+
INTERP_THREAD_SLOTS.with(|slots| {
958+
slots.borrow_mut().retain(|&id, _| id == keep_id);
959+
});
960+
}
961+
949962
pub fn with_vm<F, R>(obj: &PyObject, f: F) -> Option<R>
950963
where
951964
F: Fn(&VirtualMachine) -> R,

0 commit comments

Comments
 (0)