fix(gc): release side-table locks before visiting prototype slots (collector self-deadlock)#6150
Conversation
…lf-deadlock)
Every runtime-scoped cargo-test run since this morning's merge train wedged at
the 3h job timeout. lldb on the parked test binary shows a single-thread
self-deadlock in the copying minor collector:
visit_closure_static_prototype_slot_mut (holds closure-prototypes Mutex)
-> visit -> move_young (prototype closure is young)
-> gc_type_after_payload_move
-> closure_dynamic_props_owner_moved (locks the SAME Mutex; parks)
std::sync::Mutex is non-reentrant, so the collector parks forever inside
gc::tests::runtime_roots::callback_scanners::
test_geisterhand_callback_then_json_reviver_copied_minor_gc, and every other
test queues behind the copying-nursery isolation lock — cargo-test then sits
silent until the job timeout kills it.
visit_object_static_prototype_slot_mut (PerryTS#2820 Object.setPrototypeOf side
table) has the identical shape: an ordinary-object move re-enters
object_static_prototype_owner_moved on the same lock.
Fix both with the pattern their sibling visitors already use
(visit_closure_dynamic_prop_values_mut, scan_closure_dynamic_props_roots_mut):
remove the entry, drop the lock, run the visit, re-insert — re-keying to the
forwarded owner address in case the visit moved the owner itself.
Verified: gc::tests::runtime_roots::callback_scanners 26/26 (was: wedged
indefinitely), full perry-runtime lib suite 1162/1162 in 3.6s.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughBoth ChangesPrototype Slot Lock Refactor
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant VisitFn as visit_static_prototype_slot_mut
participant Map as PrototypesMap
participant Visitor as visit callback
Caller->>VisitFn: request slot visit(owner)
VisitFn->>Map: lock and remove(owner)
Map-->>VisitFn: proto_bits
VisitFn->>Visitor: visit(&mut proto_bits) (lock released)
Visitor-->>VisitFn: possibly forwarded owner
VisitFn->>Map: lock and reinsert(forwarded_or_original_owner, proto_bits)
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Unblocks every open PR: all runtime-scoped
cargo-testruns since this morning's merge train (#6079, #6083, #6130, and the post-merge nextTick branch run) wedged at the 3-hour job timeout. This was misattributed to each PR in turn — the deadlock is on main.Root cause
lldb on the parked test binary shows a single-thread self-deadlock in the copying minor collector:
std::sync::Mutexis non-reentrant, so the collector parks at 0% CPU insidetest_geisterhand_callback_then_json_reviver_copied_minor_gc(a forced minor GC from within a JSON reviver), every othercallback_scannerstest queues behind the copying-nursery isolation lock, and libtest sits silent until the job timeout.visit_object_static_prototype_slot_mut(#2820Object.setPrototypeOfside table) has the identical bug — an ordinary-object move re-entersobject_static_prototype_owner_movedon the same lock.The hazardous shape dates to #2345 (May); one of this morning's runtime merges made the young-movable-prototype case fire deterministically. The fix is correct regardless of trigger.
Fix
The pattern the two sibling visitors in the same file already use (
visit_closure_dynamic_prop_values_mut,scan_closure_dynamic_props_roots_mut): remove the entry → drop the lock → visit → re-insert, re-keying to the forwarded owner address in case the visit moved the owner itself (self-referential prototype).Audited the remaining
gc_type_after_payload_movehooks (overflow fields, Map/Set, exotic expando, error side tables): their visitors either collect slots before visiting (RefCell pattern) or use remove/merge — no other instance of visit-under-Mutex.Verification
cargo test -p perry-runtime --lib gc::tests::runtime_roots::callback_scanners— 26/26, 0.4s, twice (was: wedged indefinitely, reproduced locally at 0% CPU with the exact CI stack)cargo test -p perry-runtime --lib— 1162/1162 in 3.6s🤖 Generated with Claude Code
Summary by CodeRabbit