From f28ea165048750be3454df75bb9e31cf39676c01 Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Sat, 27 Jun 2026 00:05:51 +0900 Subject: [PATCH 1/2] Fix GC weakref clearing order --- .cspell.dict/rust-more.txt | 1 + Cargo.lock | 66 ++++++++++++++++++++++++++++++++++++ Cargo.toml | 1 + Lib/test/test_gc.py | 2 -- crates/vm/Cargo.toml | 1 + crates/vm/src/gc_state.rs | 43 +++++++++++++++-------- crates/vm/src/object/core.rs | 3 +- crates/vm/src/stdlib/gc.rs | 2 -- 8 files changed, 100 insertions(+), 19 deletions(-) diff --git a/.cspell.dict/rust-more.txt b/.cspell.dict/rust-more.txt index c4457723c6c..e7fb056b0c7 100644 --- a/.cspell.dict/rust-more.txt +++ b/.cspell.dict/rust-more.txt @@ -13,6 +13,7 @@ bstr byteorder byteset caseless +cdpt chrono consts cranelift diff --git a/Cargo.lock b/Cargo.lock index e6a4194a056..b053406245a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -144,6 +144,12 @@ version = "1.4.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c3d036a3c4ab069c7b410a2ce876bd74808d2d0888a82667669f8e783a898bf1" +[[package]] +name = "arrayvec" +version = "0.7.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f02882884d3e1bc524fb12c79f107f6ad0e1cfd498c536ffb494301740995dfe" + [[package]] name = "ascii" version = "1.1.0" @@ -460,6 +466,34 @@ dependencies = [ "shlex", ] +[[package]] +name = "cdpt" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9d0eb396bf1dacfc5f92972e6d9d5dec3b11ef6e8e6fa84f161554158662aafe" +dependencies = [ + "arrayvec", + "cdpt-derive", + "cfg-if", + "crossbeam", + "fastrand", + "libc", + "rustc-hash", + "static_assertions", + "windows-sys 0.60.2", +] + +[[package]] +name = "cdpt-derive" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "83bb6b4d5cec861158c8e0e2037f0a94afd01a7739dc118510c854b8f31f14b7" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "cexpr" version = "0.6.0" @@ -962,6 +996,28 @@ dependencies = [ "itertools 0.13.0", ] +[[package]] +name = "crossbeam" +version = "0.8.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1137cd7e7fc0fb5d3c5a8678be38ec56e819125d8d7907411fe24ccb943faca8" +dependencies = [ + "crossbeam-channel", + "crossbeam-deque", + "crossbeam-epoch", + "crossbeam-queue", + "crossbeam-utils", +] + +[[package]] +name = "crossbeam-channel" +version = "0.5.15" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "82b8f8f868b36967f9606790d1903570de9ceaf870a7bf9fbbd3016d636a2cb2" +dependencies = [ + "crossbeam-utils", +] + [[package]] name = "crossbeam-deque" version = "0.8.6" @@ -981,6 +1037,15 @@ dependencies = [ "crossbeam-utils", ] +[[package]] +name = "crossbeam-queue" +version = "0.3.12" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0f58bbc28f91df819d0aa2a2c00cd19754769c2fad90579b3592b1c9ba7a3115" +dependencies = [ + "crossbeam-utils", +] + [[package]] name = "crossbeam-utils" version = "0.8.21" @@ -3803,6 +3868,7 @@ dependencies = [ "ascii", "bitflags 2.13.0", "bstr", + "cdpt", "chrono", "constant_time_eq", "crossbeam-utils", diff --git a/Cargo.toml b/Cargo.toml index 4781c95e101..0f296d5f64f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -201,6 +201,7 @@ bitflagset = "0.0.3" bstr = "1" bzip2 = "0.6" chrono = { version = "0.4.44", default-features = false, features = ["clock", "std"] } +cdpt = "0.1.0" console_error_panic_hook = "0.1" constant_time_eq = "0.5" cranelift = "0.132.0" diff --git a/Lib/test/test_gc.py b/Lib/test/test_gc.py index 855e1c79cc8..0fe63332d15 100644 --- a/Lib/test/test_gc.py +++ b/Lib/test/test_gc.py @@ -236,7 +236,6 @@ def test_function(self): # is 3 because it includes f's code object. self.assertIn(gc.collect(), (2, 3)) - @unittest.expectedFailure # TODO: RUSTPYTHON; - weakref clear ordering differs from 3.15+ def test_function_tp_clear_leaves_consistent_state(self): # https://github.com/python/cpython/issues/91636 code = """if 1: @@ -831,7 +830,6 @@ def __del__(self): rc, out, err = assert_python_ok(TESTFN) self.assertEqual(out.strip(), b'__del__ called') - @unittest.expectedFailure # TODO: RUSTPYTHON def test_get_stats(self): stats = gc.get_stats() self.assertEqual(len(stats), 3) diff --git a/crates/vm/Cargo.toml b/crates/vm/Cargo.toml index b3479e017a1..cc15e380c0b 100644 --- a/crates/vm/Cargo.toml +++ b/crates/vm/Cargo.toml @@ -90,6 +90,7 @@ writeable = { workspace = true } exitcode = { workspace = true } [target.'cfg(not(target_arch = "wasm32"))'.dependencies] +cdpt = { workspace = true } rustyline = { workspace = true } which = { workspace = true } widestring = { workspace = true } diff --git a/crates/vm/src/gc_state.rs b/crates/vm/src/gc_state.rs index 93596a9dad7..9d762b11e4f 100644 --- a/crates/vm/src/gc_state.rs +++ b/crates/vm/src/gc_state.rs @@ -136,6 +136,21 @@ impl GcGeneration { #[derive(Clone, Copy, PartialEq, Eq, Hash)] struct GcPtr(NonNull); +fn clear_weakrefs_and_invoke_callbacks(objects: &[PyObjectRef]) { + let mut all_callbacks = Vec::new(); + for obj_ref in objects { + let callbacks = obj_ref.gc_clear_weakrefs_collect_callbacks(); + all_callbacks.extend(callbacks); + } + for (wr, cb) in all_callbacks { + if let Some(Err(e)) = crate::vm::thread::with_vm(&cb, |vm| cb.call((wr.clone(),), vm)) { + crate::vm::thread::with_vm(&cb, |vm| { + vm.run_unraisable(e.clone(), Some("weakref callback".to_owned()), cb.clone()); + }); + } + } +} + /// Global GC state pub struct GcState { /// 3 generations (0 = youngest, 2 = oldest) @@ -398,6 +413,11 @@ impl GcState { _ => std::time::Instant::now(), }; + // Keep a CDPT guard during collection, following origin/gc's + // coarse-grained guarded collection experiment. + #[cfg(not(target_arch = "wasm32"))] + let _cdpt_guard = cdpt::pin(); + // Memory barrier to ensure visibility of all reference count updates // from other threads before we start analyzing the object graph. core::sync::atomic::fence(Ordering::SeqCst); @@ -602,20 +622,10 @@ impl GcState { }) .collect(); - // 6c: Clear existing weakrefs BEFORE calling __del__ - let mut all_callbacks: Vec<(crate::PyRef, crate::PyObjectRef)> = - Vec::new(); - for obj_ref in &unreachable_refs { - let callbacks = obj_ref.gc_clear_weakrefs_collect_callbacks(); - all_callbacks.extend(callbacks); - } - for (wr, cb) in all_callbacks { - if let Some(Err(e)) = crate::vm::thread::with_vm(&cb, |vm| cb.call((wr.clone(),), vm)) { - crate::vm::thread::with_vm(&cb, |vm| { - vm.run_unraisable(e.clone(), Some("weakref callback".to_owned()), cb.clone()); - }); - } - } + // 6c: Clear weakrefs that existed before finalizers. This prevents a + // later tp_clear side effect from observing callback-free weakrefs to + // garbage in this generation. + clear_weakrefs_and_invoke_callbacks(&unreachable_refs); // 6d: Call __del__ on unreachable objects (skip already-finalized). // try_call_finalizer() internally checks gc_finalized() and sets it, @@ -624,6 +634,11 @@ impl GcState { obj_ref.try_call_finalizer(); } + // 6e: Clear weakrefs after finalizers, but before tp_clear. Finalizers + // can create weakrefs to other unreachable objects, and those must not + // reveal an object while its clear function is running. + clear_weakrefs_and_invoke_callbacks(&unreachable_refs); + // Detect resurrection let mut resurrected_set: HashSet = HashSet::new(); let unreachable_set: HashSet = unreachable.iter().copied().collect(); diff --git a/crates/vm/src/object/core.rs b/crates/vm/src/object/core.rs index 88ca646a4f1..b557ca2e6a6 100644 --- a/crates/vm/src/object/core.rs +++ b/crates/vm/src/object/core.rs @@ -1770,7 +1770,8 @@ impl PyObject { } /// Clear weakrefs but collect callbacks instead of calling them. - /// This is used by GC to ensure ALL weakrefs are cleared BEFORE any callbacks run. + /// GC uses this while handling a garbage set so callbacks run only after all + /// weakrefs in the current pass have been invalidated. /// Returns collected callbacks as (PyRef, callback) pairs. // = handle_weakrefs pub fn gc_clear_weakrefs_collect_callbacks(&self) -> Vec<(PyRef, PyObjectRef)> { diff --git a/crates/vm/src/stdlib/gc.rs b/crates/vm/src/stdlib/gc.rs index 00eea1b39d5..a6492fedf85 100644 --- a/crates/vm/src/stdlib/gc.rs +++ b/crates/vm/src/stdlib/gc.rs @@ -142,8 +142,6 @@ mod gc { vm.ctx.new_int(stat.uncollectable).into(), vm, )?; - dict.set_item("candidates", vm.ctx.new_int(stat.candidates).into(), vm)?; - dict.set_item("duration", vm.ctx.new_float(stat.duration).into(), vm)?; result.push(dict.into()); } From 574e86a54b7c189a24ddc3d6bc950391cd92f1a1 Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Thu, 2 Jul 2026 01:17:06 +0900 Subject: [PATCH 2/2] Run CDPT in cooperative collection mode Select CDPT's cooperative collection mode before the collection guard's first pin, so CDPT never deploys a background collector thread. The guard is only a barrier; a background thread would make the process multi-threaded, breaking fork and signal semantics and burning CPU while idle. Pin CDPT to a git revision that provides the selectable collection mode. Assisted-by: Claude --- Cargo.lock | 16 +++++++--------- Cargo.toml | 2 +- crates/vm/src/gc_state.rs | 16 ++++++++++++++-- 3 files changed, 22 insertions(+), 12 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b053406245a..fe552d1638a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -100,7 +100,7 @@ version = "1.1.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "40c48f72fd53cd289104fc64099abca73db4166ad86ea0b4341abe65af83dadc" dependencies = [ - "windows-sys 0.60.2", + "windows-sys 0.61.2", ] [[package]] @@ -111,7 +111,7 @@ checksum = "291e6a250ff86cd4a820112fb8898808a366d8f9f58ce16d1f538353ad55747d" dependencies = [ "anstyle", "once_cell_polyfill", - "windows-sys 0.60.2", + "windows-sys 0.61.2", ] [[package]] @@ -469,8 +469,7 @@ dependencies = [ [[package]] name = "cdpt" version = "0.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9d0eb396bf1dacfc5f92972e6d9d5dec3b11ef6e8e6fa84f161554158662aafe" +source = "git+https://github.com/youknowone/cdpt?rev=125a2a4c6c5ed3ba4f2550eeb8a3d44b71716a80#125a2a4c6c5ed3ba4f2550eeb8a3d44b71716a80" dependencies = [ "arrayvec", "cdpt-derive", @@ -486,8 +485,7 @@ dependencies = [ [[package]] name = "cdpt-derive" version = "0.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "83bb6b4d5cec861158c8e0e2037f0a94afd01a7739dc118510c854b8f31f14b7" +source = "git+https://github.com/youknowone/cdpt?rev=125a2a4c6c5ed3ba4f2550eeb8a3d44b71716a80#125a2a4c6c5ed3ba4f2550eeb8a3d44b71716a80" dependencies = [ "proc-macro2", "quote", @@ -1210,7 +1208,7 @@ dependencies = [ "libc", "option-ext", "redox_users", - "windows-sys 0.60.2", + "windows-sys 0.61.2", ] [[package]] @@ -2011,7 +2009,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "160f2eade097f30263b548aae5deb12ad349c909baa710fa24b92c9090b2e006" dependencies = [ "scopeguard", - "windows-sys 0.60.2", + "windows-sys 0.61.2", ] [[package]] @@ -4264,7 +4262,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "52d1cfed4120b4d927bf7c0f86d2087a4a7d6027c906d9f9d525a80573b9be51" dependencies = [ "libc", - "windows-sys 0.60.2", + "windows-sys 0.61.2", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 0f296d5f64f..7c5849913f7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -201,7 +201,7 @@ bitflagset = "0.0.3" bstr = "1" bzip2 = "0.6" chrono = { version = "0.4.44", default-features = false, features = ["clock", "std"] } -cdpt = "0.1.0" +cdpt = { git = "https://github.com/youknowone/cdpt", rev = "125a2a4c6c5ed3ba4f2550eeb8a3d44b71716a80" } console_error_panic_hook = "0.1" constant_time_eq = "0.5" cranelift = "0.132.0" diff --git a/crates/vm/src/gc_state.rs b/crates/vm/src/gc_state.rs index 9d762b11e4f..a8b3d24191f 100644 --- a/crates/vm/src/gc_state.rs +++ b/crates/vm/src/gc_state.rs @@ -414,9 +414,21 @@ impl GcState { }; // Keep a CDPT guard during collection, following origin/gc's - // coarse-grained guarded collection experiment. + // coarse-grained guarded collection experiment. Run CDPT in cooperative + // mode so it never spawns a background collector thread: the guard here + // is only a barrier, and a background thread would make the process + // multi-threaded (breaking fork/signal semantics) and burn CPU while + // idle. Selecting the mode before this first `pin()` keeps the thread + // from being deployed at all. #[cfg(not(target_arch = "wasm32"))] - let _cdpt_guard = cdpt::pin(); + let _cdpt_guard = { + // This is the only `cdpt::pin()` in the process, and the mode is + // selected right before it, so cooperative mode always latches and + // no background collector thread is ever deployed. + let cooperative = cdpt::global().set_collection_mode(cdpt::CollectionMode::Cooperative); + debug_assert!(cooperative, "CDPT must run in cooperative mode"); + cdpt::pin() + }; // Memory barrier to ensure visibility of all reference count updates // from other threads before we start analyzing the object graph.