Skip to content

RunLoopBun: break the DispatchTimer self-ref when stop() disarms a still-active timer #305

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
robobun wants to merge 1 commit into
base: main
Choose a base branch
from
Open

RunLoopBun: break the DispatchTimer self-ref when stop() disarms a still-active timer #305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions Source/WTF/wtf/RunLoop.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,11 @@ class WTF_CAPABILITY("is current") RunLoop final : public GuaranteedSerialFuncti
#elif USE(BUN_EVENT_LOOP)
// WTFTimer in Timer.zig
struct Bun__WTFTimer;
// Called from stop() once the backend has removed a still-armed timer
// from its heap, i.e. fired() is not running and will not run. Used by
// DispatchTimer to drop the self-ref dispatchAfter() stores in
// m_function; other subclasses have nothing to release.
virtual void didStopWhileActive() { }
#endif

ASCIILiteral description() const { return m_description; }
Expand Down Expand Up @@ -337,6 +342,13 @@ class WTF_CAPABILITY("is current") RunLoop final : public GuaranteedSerialFuncti
}
private:
void fired() final { m_function(); }
#if USE(BUN_EVENT_LOOP)
// dispatchAfter() captures a Ref to this timer inside m_function;
// dropping it here breaks the cycle when a caller stops the timer
// before it fires. stop() only calls this once the Bun-side heap has
// removed the still-armed node, so m_function is not on any stack.
void didStopWhileActive() final { m_function = { }; }
#endif

Function<void()> m_function;
};
Expand Down
7 changes: 5 additions & 2 deletions Source/WTF/wtf/bun/RunLoopBun.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@
extern "C" __attribute__((weak)) void WTFTimer__deinit(RunLoop::TimerBase::Bun__WTFTimer*);
extern "C" __attribute__((weak)) bool WTFTimer__isActive(const RunLoop::TimerBase::Bun__WTFTimer*);
extern "C" __attribute__((weak)) double WTFTimer__secondsUntilTimer(const RunLoop::TimerBase::Bun__WTFTimer*);
extern "C" __attribute__((weak)) void WTFTimer__cancel(RunLoop::TimerBase::Bun__WTFTimer*);
// Returns whether the timer was still armed in Bun's wtf_timers heap (i.e.
// fired() has not been and will not be dispatched for it).
extern "C" __attribute__((weak)) bool WTFTimer__cancel(RunLoop::TimerBase::Bun__WTFTimer*);

// Weak, so that Bun can override it
extern "C" __attribute__((weak)) bool Bun__thisThreadHasVM();
Expand Down Expand Up @@ -88,9 +90,10 @@
case Kind::Generic:
return stopGeneric();
case Kind::Bun:
if (auto* ref = std::get_if<std::reference_wrapper<Bun__WTFTimer>>(&m_impl)) {
WTFTimer__cancel(&ref->get());
if (WTFTimer__cancel(&ref->get()))
didStopWhileActive();
}

Check warning on line 96 in Source/WTF/wtf/bun/RunLoopBun.cpp

View check run for this annotation

Claude / Claude Code Review

didStopWhileActive() enables cross-thread ~TimerBase / WTFTimer__deinit 

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
Comment on lines 93 to 96

@claude claude Bot Jul 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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)

  1. Thread A calls Atomics.waitAsync(buf, i, v, timeout). WaiterListManager::waitAsyncImpl calls RunLoop::currentSingleton().dispatchAfter(...) on thread A. dispatchAfter does adoptRef(*new DispatchTimer) (refcount = 1), then setFunction([timer = timer.copyRef(), ...]) (refcount = 2), and returns the outer Ref, which is moved into Waiter::m_timer. Net: refcount = 2 — Waiter::m_timer + the self-ref inside m_function. The Bun__WTFTimer was created via WTFTimer__create against thread A's event loop.
  2. Thread B calls Atomics.notify on the same address before the timeout. WaiterListManager::notifyWaiter → notifyWaiterImpl → Waiter::scheduleWorkAndClear → clearTimer(listLocker) all run synchronously on thread B while holding list->lock.
  3. clearTimer (WaiterListManager.h:98) does m_timer->stop(). In RunLoopBun.cpp, WTFTimer__cancel returns true (node was still ACTIVE in the heap), so didStopWhileActive() runs and does m_function = { }. Destroying the lambda destroys its captured Ref<DispatchTimer>: refcount 2 → 1.
  4. clearTimer then does m_timer = nullptr on the very next line: refcount 1 → 0. ~DispatchTimer → ~TimerBase runs on thread B and calls WTFTimer__deinit(&ref->get()) on a Bun__WTFTimer that 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 called WTFTimer__cancel; the self-ref in m_function survived, so after m_timer = nullptr the refcount was still 1 and the timer leaked. ~TimerBase / WTFTimer__deinit were unreachable on this path.
  • In the fires-first path, fired() runs on thread A, protectedTimer on thread A's stack is the last ref, and ~TimerBase runs on thread A.
  • So cross-thread WTFTimer__deinit did not exist before this PR and is introduced here.

Why the PR's own argument doesn't cover it

The description says:

~DispatchTimer is not reached here because the caller (Waiter::clearTimer) still holds m_timer across stop().

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.

return;
}
}
Expand Down
Loading