Skip to content

Commit b696bf0

Browse files
youknowoneyouknowone
committed
Fix GC weakref clearing order
1 parent 0009dd6 commit b696bf0

8 files changed

Lines changed: 100 additions & 19 deletions

File tree

‎.cspell.dict/rust-more.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ bstr
1313
byteorder
1414
byteset
1515
caseless
16+
cdpt
1617
chrono
1718
consts
1819
cranelift

‎Cargo.lock‎

Lines changed: 66 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Cargo.toml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,7 @@ bitflagset = "0.0.3"
201201
bstr = "1"
202202
bzip2 = "0.6"
203203
chrono = { version = "0.4.44", default-features = false, features = ["clock", "std"] }
204+
cdpt = "0.1.0"
204205
console_error_panic_hook = "0.1"
205206
constant_time_eq = "0.5"
206207
cranelift = "0.132.0"

‎Lib/test/test_gc.py‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -236,7 +236,6 @@ def test_function(self):
236236
# is 3 because it includes f's code object.
237237
self.assertIn(gc.collect(), (2, 3))
238238

239-
@unittest.expectedFailure # TODO: RUSTPYTHON; - weakref clear ordering differs from 3.15+
240239
def test_function_tp_clear_leaves_consistent_state(self):
241240
# https://github.com/python/cpython/issues/91636
242241
code = """if 1:
@@ -831,7 +830,6 @@ def __del__(self):
831830
rc, out, err = assert_python_ok(TESTFN)
832831
self.assertEqual(out.strip(), b'__del__ called')
833832

834-
@unittest.expectedFailure # TODO: RUSTPYTHON
835833
def test_get_stats(self):
836834
stats = gc.get_stats()
837835
self.assertEqual(len(stats), 3)

‎crates/vm/Cargo.toml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@ writeable = { workspace = true }
9090
exitcode = { workspace = true }
9191

9292
[target.'cfg(not(target_arch = "wasm32"))'.dependencies]
93+
cdpt = { workspace = true }
9394
rustyline = { workspace = true }
9495
which = { workspace = true }
9596
widestring = { workspace = true }

‎crates/vm/src/gc_state.rs‎

Lines changed: 29 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,21 @@ impl GcGeneration {
136136
#[derive(Clone, Copy, PartialEq, Eq, Hash)]
137137
struct GcPtr(NonNull<PyObject>);
138138

139+
fn clear_weakrefs_and_invoke_callbacks(objects: &[PyObjectRef]) {
140+
let mut all_callbacks = Vec::new();
141+
for obj_ref in objects {
142+
let callbacks = obj_ref.gc_clear_weakrefs_collect_callbacks();
143+
all_callbacks.extend(callbacks);
144+
}
145+
for (wr, cb) in all_callbacks {
146+
if let Some(Err(e)) = crate::vm::thread::with_vm(&cb, |vm| cb.call((wr.clone(),), vm)) {
147+
crate::vm::thread::with_vm(&cb, |vm| {
148+
vm.run_unraisable(e.clone(), Some("weakref callback".to_owned()), cb.clone());
149+
});
150+
}
151+
}
152+
}
153+
139154
/// Global GC state
140155
pub struct GcState {
141156
/// 3 generations (0 = youngest, 2 = oldest)
@@ -398,6 +413,11 @@ impl GcState {
398413
_ => std::time::Instant::now(),
399414
};
400415

416+
// Keep a CDPT guard during collection, following origin/gc's
417+
// coarse-grained guarded collection experiment.
418+
#[cfg(not(target_arch = "wasm32"))]
419+
let _cdpt_guard = cdpt::pin();
420+
401421
// Memory barrier to ensure visibility of all reference count updates
402422
// from other threads before we start analyzing the object graph.
403423
core::sync::atomic::fence(Ordering::SeqCst);
@@ -602,20 +622,10 @@ impl GcState {
602622
})
603623
.collect();
604624

605-
// 6c: Clear existing weakrefs BEFORE calling __del__
606-
let mut all_callbacks: Vec<(crate::PyRef<crate::object::PyWeak>, crate::PyObjectRef)> =
607-
Vec::new();
608-
for obj_ref in &unreachable_refs {
609-
let callbacks = obj_ref.gc_clear_weakrefs_collect_callbacks();
610-
all_callbacks.extend(callbacks);
611-
}
612-
for (wr, cb) in all_callbacks {
613-
if let Some(Err(e)) = crate::vm::thread::with_vm(&cb, |vm| cb.call((wr.clone(),), vm)) {
614-
crate::vm::thread::with_vm(&cb, |vm| {
615-
vm.run_unraisable(e.clone(), Some("weakref callback".to_owned()), cb.clone());
616-
});
617-
}
618-
}
625+
// 6c: Clear weakrefs that existed before finalizers. This prevents a
626+
// later tp_clear side effect from observing callback-free weakrefs to
627+
// garbage in this generation.
628+
clear_weakrefs_and_invoke_callbacks(&unreachable_refs);
619629

620630
// 6d: Call __del__ on unreachable objects (skip already-finalized).
621631
// try_call_finalizer() internally checks gc_finalized() and sets it,
@@ -624,6 +634,11 @@ impl GcState {
624634
obj_ref.try_call_finalizer();
625635
}
626636

637+
// 6e: Clear weakrefs after finalizers, but before tp_clear. Finalizers
638+
// can create weakrefs to other unreachable objects, and those must not
639+
// reveal an object while its clear function is running.
640+
clear_weakrefs_and_invoke_callbacks(&unreachable_refs);
641+
627642
// Detect resurrection
628643
let mut resurrected_set: HashSet<GcPtr> = HashSet::new();
629644
let unreachable_set: HashSet<GcPtr> = unreachable.iter().copied().collect();

‎crates/vm/src/object/core.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1770,7 +1770,8 @@ impl PyObject {
17701770
}
17711771

17721772
/// Clear weakrefs but collect callbacks instead of calling them.
1773-
/// This is used by GC to ensure ALL weakrefs are cleared BEFORE any callbacks run.
1773+
/// GC uses this while handling a garbage set so callbacks run only after all
1774+
/// weakrefs in the current pass have been invalidated.
17741775
/// Returns collected callbacks as (PyRef<PyWeak>, callback) pairs.
17751776
// = handle_weakrefs
17761777
pub fn gc_clear_weakrefs_collect_callbacks(&self) -> Vec<(PyRef<PyWeak>, PyObjectRef)> {

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

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -142,8 +142,6 @@ mod gc {
142142
vm.ctx.new_int(stat.uncollectable).into(),
143143
vm,
144144
)?;
145-
dict.set_item("candidates", vm.ctx.new_int(stat.candidates).into(), vm)?;
146-
dict.set_item("duration", vm.ctx.new_float(stat.duration).into(), vm)?;
147145
result.push(dict.into());
148146
}
149147

0 commit comments

Comments
 (0)