RunLoopBun: break the DispatchTimer self-ref when stop() disarms a still-active timer#305
RunLoopBun: break the DispatchTimer self-ref when stop() disarms a still-active timer#305robobun wants to merge 1 commit into
Conversation
…ill-active timer RunLoop::dispatchAfter stores a lambda in DispatchTimer::m_function that captures a Ref to the timer itself. That self-ref is only moved out when fired() runs; a caller that stops the timer before it fires (WaiterListManager::clearTimer on Atomics.notify/unregister) orphans the cycle and leaks both the DispatchTimer and the Bun-side WTFTimer box it owns. Add a virtual TimerBase::didStopWhileActive() hook (USE(BUN_EVENT_LOOP) only). DispatchTimer overrides it to clear m_function. TimerBase::stop() calls it when WTFTimer__cancel reports the timer was still in Bun's wtf_timers heap, which means Bun's drain_due_wtf_timers has not popped it (the ACTIVE to FIRED transition is serialised by the heap lock) and so fired() is not executing on any thread. WTFTimer__cancel's signature changes from void to bool; oven-sh/bun#34456 provides the matching Rust side.
…en-sh/WebKit#305) The current prebuilt WTF declares WTFTimer__cancel as void and ignores the return; once WEBKIT_VERSION includes oven-sh/WebKit#305, RunLoopBun's TimerBase::stop() reads it to drive the new virtual didStopWhileActive() hook and Bun__breakDispatchTimerCycle becomes a no-op second clear (to be deleted with the bump).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughBun-backed RunLoop timers now report when stopping removes an armed timer. ChangesBun timer cleanup
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (auto* ref = std::get_if<std::reference_wrapper<Bun__WTFTimer>>(&m_impl)) { | ||
| WTFTimer__cancel(&ref->get()); | ||
| if (WTFTimer__cancel(&ref->get())) | ||
| didStopWhileActive(); | ||
| } |
There was a problem hiding this comment.
🟡 This change makes ~TimerBase / WTFTimer__deinit newly reachable on a foreign thread: in the Atomics.notify-before-timeout path, clearTimer on thread B now drops the self-ref inside stop() and then the last ref via m_timer = nullptr, so WTFTimer__deinit runs on thread B against a Bun__WTFTimer created for thread A's event loop. Pre-PR this destructor was unreachable here (leak), so this is a new cross-thread lifecycle path — the PR's safety argument covers WTFTimer__cancel under the heap lock but doesn't mention WTFTimer__deinit. Worth confirming bun#34456 makes wtf_deinit safe off the owning thread, or deferring the self-ref release back to the timer's RunLoop.
Extended reasoning...
What changed
didStopWhileActive() now clears DispatchTimer::m_function inside stop() when WTFTimer__cancel returns true. m_function holds the timer.copyRef() self-reference that RunLoop::dispatchAfter captures (RunLoop.cpp:176), so clearing it drops one ref on the DispatchTimer. That is exactly the leak fix — but it also moves where the last ref can fall to zero.
Step-by-step trace (the exact scenario the PR targets)
- Thread A calls
Atomics.waitAsync(buf, i, v, timeout).WaiterListManager::waitAsyncImplcallsRunLoop::currentSingleton().dispatchAfter(...)on thread A.dispatchAfterdoesadoptRef(*new DispatchTimer)(refcount = 1), thensetFunction([timer = timer.copyRef(), ...])(refcount = 2), and returns the outerRef, which is moved intoWaiter::m_timer. Net: refcount = 2 —Waiter::m_timer+ the self-ref insidem_function. TheBun__WTFTimerwas created viaWTFTimer__createagainst thread A's event loop. - Thread B calls
Atomics.notifyon the same address before the timeout.WaiterListManager::notifyWaiter→notifyWaiterImpl→Waiter::scheduleWorkAndClear→clearTimer(listLocker)all run synchronously on thread B while holdinglist->lock. clearTimer(WaiterListManager.h:98) doesm_timer->stop(). InRunLoopBun.cpp,WTFTimer__cancelreturnstrue(node was stillACTIVEin the heap), sodidStopWhileActive()runs and doesm_function = { }. Destroying the lambda destroys its capturedRef<DispatchTimer>: refcount 2 → 1.clearTimerthen doesm_timer = nullptron the very next line: refcount 1 → 0.~DispatchTimer→~TimerBaseruns on thread B and callsWTFTimer__deinit(&ref->get())on aBun__WTFTimerthat was created for thread A's event loop.
There are no other refs that would keep the timer alive past step 4: RunLoop::m_registeredTimers is a HashSet<TimerBase*> of raw pointers, and the Zig-side Bun__WTFTimer holds a raw TimerBase* back-pointer, not a Ref.
Why this is new behavior
- Pre-PR,
stop()only calledWTFTimer__cancel; the self-ref inm_functionsurvived, so afterm_timer = nullptrthe refcount was still 1 and the timer leaked.~TimerBase/WTFTimer__deinitwere unreachable on this path. - In the fires-first path,
fired()runs on thread A,protectedTimeron thread A's stack is the last ref, and~TimerBaseruns on thread A. - So cross-thread
WTFTimer__deinitdid not exist before this PR and is introduced here.
Why the PR's own argument doesn't cover it
The description says:
~DispatchTimeris not reached here because the caller (Waiter::clearTimer) still holdsm_timeracrossstop().
That's true for the duration of stop() itself (so didStopWhileActive() can't destroy *this mid-call), but m_timer = nullptr is the immediately following statement in clearTimer, still on thread B. The description and bun#34456 discuss making WTFTimer__cancel return bool and be lock-safe, but say nothing about WTFTimer__deinit being called from a thread other than the one that called WTFTimer__create.
Impact and fix
If the Zig-side wtf_deinit touches thread-A-local state (the owning VirtualMachine / event-loop's wtf_timers bookkeeping, a per-thread allocator, etc.) without the same lock wtf_disarm takes, this trades a leak for cross-thread corruption. It may well already be safe — WTFTimer__cancel was already called cross-thread pre-PR, so the Zig side already locks the heap, and deinit after a successful cancel may just be a locked unlink + free — but that's worth confirming explicitly since it isn't stated. If it isn't safe, one option is to post the self-ref release back to m_runLoop (e.g. m_runLoop->dispatch([f = WTF::move(m_function)] { })) so the last deref and WTFTimer__deinit happen on the owning thread.
Preview Builds
|
1 participant
Every
Atomics.waitAsync(buf, i, v, timeout)woken byAtomics.notify(or byWaiterListManager::unregister) before its timeout fires leaks aWTF::RunLoop::DispatchTimerand the Bun-sideWTFTimerbox it owns.RunLoop::dispatchAfterstores its fire lambda inDispatchTimer::m_function, and the lambda capturestimer.copyRef()to the timer itself. That self-ref is only moved out whenfired()runs.Waiter::clearTimerjust doesm_timer->stop(); m_timer = nullptr;, which drops the waiter's ref but leaves the self-ref intact.Change
TimerBasegets a virtualdidStopWhileActive()hook (underUSE(BUN_EVENT_LOOP), default no-op).DispatchTimeroverrides it to clearm_function, releasing the self-ref (and the capturedRef<Waiter>).WTFTimer__cancelreturnsbool: whether it removed a still-ACTIVEnode from Bun'swtf_timersheap.TimerBase::stop()callsdidStopWhileActive()whenWTFTimer__cancelreturns true.The return-true path proves
fired()is not executing: Bun'sdrain_due_wtf_timerspops a node and sets its state toFIREDunder the same lock thatwtf_disarmchecks forACTIVE. Som_functionis not on any stack when it is cleared. WhenWTFTimer__cancelreturns false (already popped, or reached from inside the fire lambda's ownprotectedTimer->stop()), we leavem_functionalone;fired()breaks the cycle itself.Other
TimerBasesubclasses inherit the empty default; they have no self-ref to release.~DispatchTimeris not reached here because the caller (Waiter::clearTimer) still holdsm_timeracrossstop().Coordination
Requires oven-sh/bun#34456, which changes the Rust-side
WTFTimer__cancelto returnbool. The bun PR also carries a temporary bun-side cycle break so it is safe to land beforeWEBKIT_VERSIONis bumped to include this.