Skip to content

Commit ea68d61

Browse files
committed
Fix post-merge subinterpreter lifecycle issues
Preserve logical memoryview descriptors across interpreter transfers, make channel-end release idempotent, and scope runtime-owned interpreter cleanup to the creating top-level runtime. Re-read owned interpreters during shutdown so finalizers cannot leave newly-created children behind. Add regression coverage for sliced and cast memoryviews, repeated channel release, and multiple embedded runtimes. Assisted-by: Codex:GPT-5
1 parent a68e3b0 commit ea68d61

6 files changed

Lines changed: 118 additions & 9 deletions

File tree

‎crates/vm/src/builtins/memory.rs‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,9 @@ impl PyMemoryView {
128128
/// `send_buffer` so the destination memoryview sees the same memory.
129129
#[must_use]
130130
pub fn clone_buffer(&self) -> PyBuffer {
131-
self.buffer.clone()
131+
let mut buffer = self.buffer.clone();
132+
buffer.desc = self.desc.clone();
133+
buffer
132134
}
133135

134136
// PyMemoryView_FromObjectAndFlags

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

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -230,6 +230,9 @@ pub(crate) mod _interpchannels {
230230
}
231231

232232
fn release_end(&mut self, index: usize, send: bool) {
233+
if !self.list(send)[index].1 {
234+
return;
235+
}
233236
self.list(send)[index].1 = false;
234237
if send {
235238
self.numsendopen -= 1;
@@ -1295,4 +1298,28 @@ pub(crate) mod _interpchannels {
12951298
}
12961299
Ok(())
12971300
}
1301+
1302+
#[cfg(test)]
1303+
mod tests {
1304+
use super::ChannelEnds;
1305+
1306+
#[test]
1307+
fn releasing_an_end_twice_is_a_noop() {
1308+
let mut ends = ChannelEnds::default();
1309+
ends.add(1, true);
1310+
ends.add(1, false);
1311+
1312+
let send = ends.find(1, true).unwrap();
1313+
ends.release_end(send, true);
1314+
ends.release_end(send, true);
1315+
assert_eq!(ends.numsendopen, 0);
1316+
assert_eq!(ends.numrecvopen, 1);
1317+
assert!(ends.is_open());
1318+
1319+
let recv = ends.find(1, false).unwrap();
1320+
ends.release_end(recv, false);
1321+
assert_eq!(ends.numrecvopen, 0);
1322+
assert!(!ends.is_open());
1323+
}
1324+
}
12981325
}

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

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,7 @@ where
148148
let warnings = WarningsState::init_state(&ctx);
149149

150150
let interpreter_id = runtime::alloc_interpreter_id();
151+
let runtime_root_id = parent_state.map_or(interpreter_id, |parent| parent.runtime_root_id);
151152

152153
// Process main OS thread identity is process-global; subinterpreters inherit
153154
// it from the parent so `is_main_thread()` stays correct when running on the
@@ -162,6 +163,7 @@ where
162163
let global_state = PyRc::new(PyGlobalState {
163164
gc: crate::gc_state::GcInterpreterState::new(&ctx),
164165
interpreter_id,
166+
runtime_root_id,
165167
whence,
166168
is_main,
167169
config,
@@ -735,9 +737,9 @@ fn finalize_subinterpreters(vm: &VirtualMachine) {
735737
if !vm.state.is_main {
736738
return;
737739
}
738-
let ids = runtime::owned_interpreter_ids();
740+
let root_id = vm.state.runtime_root_id;
739741
// Bail out if there are no subinterpreters left.
740-
if ids.is_empty() {
742+
if runtime::owned_interpreter_ids_for(root_id).is_empty() {
741743
return;
742744
}
743745
// Warn the user if they forgot to clean up subinterpreters.
@@ -751,7 +753,12 @@ fn finalize_subinterpreters(vm: &VirtualMachine) {
751753
None,
752754
vm,
753755
);
754-
for id in ids {
756+
// A subinterpreter's finalizers may create another subinterpreter, so
757+
// re-read the owner table after each destruction like CPython does.
758+
while let Some(id) = runtime::owned_interpreter_ids_for(root_id)
759+
.into_iter()
760+
.next()
761+
{
755762
let _ = runtime::destroy_owned_interpreter(id);
756763
}
757764
}
@@ -1557,6 +1564,25 @@ mod tests {
15571564
assert!(!sub.is_main());
15581565
}
15591566

1567+
/// Finalizing one embedded runtime must leave another runtime's owned
1568+
/// subinterpreters alone.
1569+
#[cfg(feature = "threading")]
1570+
#[test]
1571+
fn owned_subinterpreter_cleanup_is_scoped_to_its_runtime() {
1572+
let main1 = Interpreter::without_stdlib(Default::default());
1573+
let main2 = Interpreter::without_stdlib(Default::default());
1574+
let sub1 = main1.create_owned_subinterpreter();
1575+
let sub2 = main2.create_owned_subinterpreter();
1576+
1577+
main1.enter(finalize_subinterpreters);
1578+
1579+
assert!(!runtime::is_owned_interpreter(sub1));
1580+
assert!(runtime::is_owned_interpreter(sub2));
1581+
1582+
let sub2 = runtime::take_owned_interpreter(sub2).expect("owned by second runtime");
1583+
let _ = sub2.finalize(None);
1584+
}
1585+
15601586
/// A collection must stop every interpreter, not just the collecting one:
15611587
/// the generation lists are process-global, so the reachability walk reads
15621588
/// objects owned by other interpreters while their threads would otherwise

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -759,6 +759,8 @@ pub(crate) struct CallableCache {
759759
pub struct PyGlobalState {
760760
/// Unique process-global interpreter id (main is [`MAIN_INTERPRETER_ID`]).
761761
pub interpreter_id: i64,
762+
/// Top-level interpreter whose runtime owns this interpreter.
763+
pub runtime_root_id: i64,
762764
/// How this interpreter was created.
763765
pub whence: runtime::InterpreterWhence,
764766
/// True for every top-level (non-sub) interpreter, each of which keeps its

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

Lines changed: 34 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -452,19 +452,32 @@ pub fn live_interpreter_states() -> Vec<PyRc<PyGlobalState>> {
452452
/// via [`owned_new_thread`], which only clones the shared interpreter fields
453453
/// (`sys`, builtins, `PyGlobalState`).
454454
#[cfg(feature = "threading")]
455-
fn owned_interpreters() -> &'static Mutex<HashMap<i64, crate::Interpreter>> {
455+
struct OwnedInterpreter {
456+
root_id: i64,
457+
interpreter: crate::Interpreter,
458+
}
459+
460+
#[cfg(feature = "threading")]
461+
fn owned_interpreters() -> &'static Mutex<HashMap<i64, OwnedInterpreter>> {
456462
use std::sync::OnceLock;
457-
static OWNED: OnceLock<Mutex<HashMap<i64, crate::Interpreter>>> = OnceLock::new();
463+
static OWNED: OnceLock<Mutex<HashMap<i64, OwnedInterpreter>>> = OnceLock::new();
458464
OWNED.get_or_init(|| Mutex::new(HashMap::new()))
459465
}
460466

461467
/// Transfer ownership of `interp` to the runtime, returning its id.
462468
#[cfg(feature = "threading")]
463469
pub fn store_owned_interpreter(interp: crate::Interpreter) -> i64 {
464470
let id = interp.id();
471+
let root_id = interp.global_state.runtime_root_id;
465472
// Ids are strictly monotonic, so this never displaces (and drops) an
466473
// existing entry under the lock.
467-
owned_interpreters().lock().insert(id, interp);
474+
owned_interpreters().lock().insert(
475+
id,
476+
OwnedInterpreter {
477+
root_id,
478+
interpreter: interp,
479+
},
480+
);
468481
id
469482
}
470483

@@ -475,7 +488,10 @@ pub fn store_owned_interpreter(interp: crate::Interpreter) -> i64 {
475488
#[cfg(feature = "threading")]
476489
#[must_use]
477490
pub fn take_owned_interpreter(id: i64) -> Option<crate::Interpreter> {
478-
owned_interpreters().lock().remove(&id)
491+
owned_interpreters()
492+
.lock()
493+
.remove(&id)
494+
.map(|owned| owned.interpreter)
479495
}
480496

481497
/// Create a thread-state VM for a runtime-owned interpreter.
@@ -487,7 +503,7 @@ pub fn owned_new_thread(id: i64) -> Option<crate::vm::thread::ThreadedVirtualMac
487503
owned_interpreters()
488504
.lock()
489505
.get(&id)
490-
.map(|interp| interp.new_thread())
506+
.map(|owned| owned.interpreter.new_thread())
491507
}
492508

493509
/// Whether `id` refers to a runtime-owned interpreter.
@@ -513,6 +529,19 @@ pub fn owned_interpreter_ids() -> Vec<i64> {
513529
ids
514530
}
515531

532+
/// Ids of runtime-owned interpreters belonging to `root_id`, oldest first.
533+
#[cfg(feature = "threading")]
534+
#[must_use]
535+
pub fn owned_interpreter_ids_for(root_id: i64) -> Vec<i64> {
536+
let mut ids: Vec<i64> = owned_interpreters()
537+
.lock()
538+
.iter()
539+
.filter_map(|(&id, owned)| (owned.root_id == root_id).then_some(id))
540+
.collect();
541+
ids.sort_unstable();
542+
ids
543+
}
544+
516545
/// Finalize and drop a runtime-owned interpreter.
517546
///
518547
/// The caller must already have checked that the interpreter is not the
Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
import _interpchannels
2+
import _interpreters
3+
4+
5+
def roundtrip_buffer(view):
6+
cid = _interpchannels.create(3)
7+
try:
8+
_interpchannels.send_buffer(cid, view, blocking=False)
9+
received, unbound = _interpchannels.recv(cid)
10+
assert unbound is None
11+
return received
12+
finally:
13+
_interpchannels.destroy(cid)
14+
15+
16+
data = bytearray(range(12))
17+
for view in (memoryview(data)[2:10:2], memoryview(data).cast("I")):
18+
received = roundtrip_buffer(view)
19+
assert received.tolist() == view.tolist()
20+
assert received.shape == view.shape
21+
assert received.strides == view.strides
22+
assert received.format == view.format
23+
assert received.itemsize == view.itemsize

0 commit comments

Comments
 (0)