Skip to content

Commit 8083355

Browse files
committed
Park stop-the-world threads even if STOP_BIT is cleared
STOP_BIT is process-wide, but each interpreter's start_the_world clears it. Parallel tests then skip check_signals and never park. Assisted-by: Grok:4.6
1 parent 94298ca commit 8083355

2 files changed

Lines changed: 93 additions & 17 deletions

File tree

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

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1733,6 +1733,84 @@ for _ in range(40):
17331733
worker.join().expect("worker panicked");
17341734
}
17351735

1736+
/// `STOP_BIT` is process-wide and `start_the_world` in any interpreter
1737+
/// clears it. A worker must still park from its own `stop_requested`.
1738+
#[cfg(all(feature = "threading", feature = "rustpython-compiler"))]
1739+
#[test]
1740+
fn stop_the_world_parks_when_global_stop_bit_is_cleared() {
1741+
use crate::compiler::Mode;
1742+
use alloc::sync::Arc;
1743+
use core::{
1744+
sync::atomic::{AtomicBool, AtomicU64, Ordering},
1745+
time::Duration,
1746+
};
1747+
1748+
let main = Interpreter::without_stdlib(Default::default());
1749+
let sub = main.create_subinterpreter();
1750+
let sub_state = sub.enter(|vm| vm.state.clone());
1751+
1752+
let progress = Arc::new(AtomicU64::new(0));
1753+
let stop = Arc::new(AtomicBool::new(false));
1754+
let flip = Arc::new(AtomicBool::new(true));
1755+
1756+
let progress_worker = Arc::clone(&progress);
1757+
let stop_worker = Arc::clone(&stop);
1758+
let worker = sub.enter(|vm| {
1759+
let thread_vm = vm.new_thread();
1760+
std::thread::spawn(move || {
1761+
thread_vm.run(|vm| {
1762+
let source = "x = 1 + 1\n";
1763+
let code = vm
1764+
.compile(source, Mode::Exec, "<spin>")
1765+
.map_err(|err| err.into_pyexception(vm, Some(source)))
1766+
.unwrap();
1767+
while !stop_worker.load(Ordering::Acquire) {
1768+
let scope = vm.new_scope_with_builtins();
1769+
vm.run_code_obj(code.clone(), scope).unwrap();
1770+
progress_worker.fetch_add(1, Ordering::Release);
1771+
}
1772+
});
1773+
})
1774+
});
1775+
1776+
while progress.load(Ordering::Acquire) == 0 {
1777+
std::thread::yield_now();
1778+
}
1779+
1780+
let flip_flag = Arc::clone(&flip);
1781+
let flicker = std::thread::spawn(move || {
1782+
while flip_flag.load(Ordering::Acquire) {
1783+
crate::signal::set_stop_bit();
1784+
crate::signal::clear_stop_bit();
1785+
}
1786+
});
1787+
1788+
main.enter(|_vm| {
1789+
sub_state.stop_the_world.stop_the_world(&sub_state);
1790+
1791+
let parked_at = progress.load(Ordering::Acquire);
1792+
std::thread::sleep(Duration::from_millis(50));
1793+
assert_eq!(
1794+
progress.load(Ordering::Acquire),
1795+
parked_at,
1796+
"subinterpreter thread kept running after STOP_BIT was cleared"
1797+
);
1798+
1799+
sub_state.stop_the_world.start_the_world(&sub_state);
1800+
});
1801+
1802+
flip.store(false, Ordering::Release);
1803+
flicker.join().expect("stop-bit flicker panicked");
1804+
1805+
let resumed_from = progress.load(Ordering::Acquire);
1806+
while progress.load(Ordering::Acquire) == resumed_from {
1807+
std::thread::yield_now();
1808+
}
1809+
1810+
stop.store(true, Ordering::Release);
1811+
worker.join().expect("worker panicked");
1812+
}
1813+
17361814
/// Entering a subinterpreter from inside the parent's `enter` must attach
17371815
/// the subinterpreter's thread slot (and detach the parent's). Otherwise the
17381816
/// thread runs the sub's bytecode with a DETACHED slot, and a collector

‎crates/vm/src/vm/mod.rs‎

Lines changed: 15 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -567,11 +567,9 @@ impl StopTheWorldState {
567567
drop(registry);
568568
self.thread_countdown.store(0, Ordering::Release);
569569
self.requester.store(0, Ordering::Relaxed);
570-
// Every non-requester thread's `stop_requested` was just cleared
571-
// above, so the shared fast-path bit can be cleared too. Safe here
572-
// (and only here / `reset_after_fork`) because both run under the
573-
// single stop-the-world exclusion, released below, so no new
574-
// requester can be setting the bit concurrently.
570+
// Drop the process-wide stop hint. Another interpreter may still
571+
// have `stop_requested` threads; those keep parking via the
572+
// per-thread check in `eval_breaker_tripped`.
575573
crate::signal::clear_stop_bit();
576574
#[cfg(debug_assertions)]
577575
self.debug_assert_all_non_requester_detached(state);
@@ -3500,20 +3498,20 @@ impl VirtualMachine {
35003498
self.get_method(obj, method_name)
35013499
}
35023500

3503-
/// Single relaxed load of the shared eval-breaker word, folding in every
3504-
/// condition that used to require its own check: pending signals, QSBR
3505-
/// reclamation, scheduled GC, and (under `threading`) finalization and
3506-
/// stop-the-world. Finalizing sets `FINALIZING_BIT` and any per-thread
3507-
/// `stop_requested` store also sets `STOP_BIT` (see signal.rs), so this
3508-
/// no longer needs to read `state.finalizing` or the
3509-
/// `CURRENT_STOP_REQUESTED` thread-local itself — both are folded into
3510-
/// the same atomic word `eval_breaker_pending` already loads for
3511-
/// signals/QSBR/GC. The slow path (`check_signals`) re-derives the exact
3512-
/// per-thread answer (this thread's own `stop_requested`, `finalizing` +
3513-
/// `is_main_thread`) once it is taken, so semantics are unchanged; only
3514-
/// the common (nothing pending) case gets cheaper.
3501+
/// Fast path for the bytecode loop: pending signals, QSBR, scheduled GC,
3502+
/// finalization, and stop-the-world.
3503+
///
3504+
/// `STOP_BIT` is a process-wide hint set when any interpreter asks a
3505+
/// thread to park. Each interpreter's `start_the_world` clears that hint,
3506+
/// even if another interpreter still has `stop_requested` threads, so the
3507+
/// per-thread flag is checked first. Missing it lets a worker skip
3508+
/// `check_signals` and never park, so `stop_the_world` waits forever.
35153509
#[inline]
35163510
pub(crate) fn eval_breaker_tripped(&self) -> bool {
3511+
#[cfg(feature = "threading")]
3512+
if thread::stop_requested_for_current_thread() {
3513+
return true;
3514+
}
35173515
#[cfg(not(target_arch = "wasm32"))]
35183516
{
35193517
crate::signal::eval_breaker_pending()

0 commit comments

Comments
 (0)