JSC: keep a TerminationException pending in the BunPerformMicrotaskJob handler - #491
Conversation
…b handler The BunPerformMicrotaskJob arm of runInternalMicrotask catches every exception the job throws with a plain clearException(), then reports it through Bun__reportUnhandledError. When the caught exception is the VM's TerminationException (a worker.terminate() trap serviced inside the job), that clear consumes it. VMTraps::handleTraps already cleared the NeedTermination bit, so nothing raises it again. runMicrotask() never sees the termination and the checkpoint keeps draining. A microtask-only loop that queues such a job on every turn never returns to the event loop, and terminate() never completes. Use clearExceptionExceptTermination() and return with the termination pending, the same way the PromiseReactionJob arm does. runMicrotask() then stops the checkpoint and the embedder takes the termination at the drain boundary.
|
Warning Review limit reachedYour included review limit has been reached. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 29 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset (next review available in 6 minutes), then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
There was a problem hiding this comment.
LGTM — brings BunPerformMicrotaskJob in line with every other arm of runInternalMicrotask that already uses clearExceptionExceptTermination().
What was reviewed:
- Confirmed the
scope.release(); return;pattern on termination matches the identical usage at 7 other sites in this file (e.g.JSMicrotask.cpp:367,:887,:1959). - Verified
scoperefers to the outerDECLARE_THROW_SCOPEatrunInternalMicrotaskentry, andasyncContextScope.restoreEarly()has already run before the early return. - Checked that the captured
exceptionlocal remains valid forBun__reportUnhandledErrorin the non-termination path after the clear.
Extended reasoning...
Overview
Single-file, 9-line change to Source/JavaScriptCore/runtime/JSMicrotask.cpp in the BunPerformMicrotaskJob arm of runInternalMicrotask. Replaces a bare catchScope.clearException() with catchScope.clearExceptionExceptTermination(), and on termination releases the outer throw scope and returns early with the TerminationException still pending. Also adds [[unlikely]] hints and an explanatory comment.
Security risks
None. This is purely about VM termination-exception propagation during microtask draining. No input parsing, no allocation changes, no auth/permission surface.
Level of scrutiny
Low-to-medium. The change is mechanical: it swaps one exception-clear call for the termination-preserving variant and follows the exact idiom already used in 7+ other places in the same function (verified at lines 238, 266, 289, 367, 623, 887, 930, 1738, 1959). The PR description is thorough — it explains the hang mechanism (VMTraps::handleTraps clears the NeedTermination bit once, so swallowing the resulting exception loses the only signal), gives a repro, and describes watchpoint-based verification.
Other factors
scopeis theDECLARE_THROW_SCOPE(vm)at the top ofrunInternalMicrotask(line 1804);scope.release()before returning with a pending exception is the standard JSC pattern to satisfy exception-scope verification.asyncContextScope.restoreEarly()runs before the exception check, so async context is restored on both paths; thecatchScope(aTopExceptionScope) destructor runs at block exit on the early return.- The
exceptionpointer is captured before the clear, so passing it toBun__reportUnhandledErrorin the non-termination branch is unchanged in behavior. - Regression test lands in the Bun repo per the description; no JSC-side test is expected here since the behavior requires the Bun embedder callback and worker termination.
- No prior human or bot reviews to reconcile with (CodeRabbit was rate-limited).
Preview Builds
|
|
Bun side: oven-sh/bun#40128 pins the preview build of this PR ( |
…vent listeners, and emit()
worker.terminate() never settled when the termination trap was serviced
inside native code that caught the TerminationException and dropped it.
A microtask-only loop (`for (;;) { ...; await 0; }`) never returns to the
event loop, so the pending exception was the only way out of the
checkpoint. The trap bit is consumed when the exception is thrown, so
there is no second delivery.
Three catch sites dropped it:
- The BunPerformMicrotaskJob arm of runInternalMicrotask in JSC (the
job behind queueMicrotask() and the C++ web streams' start and pull
reactions) cleared it with a plain clearException(). Fixed in
oven-sh/WebKit#491; this pins that build.
- JSEventListener::handleEvent (EventTarget and AbortSignal listeners)
and EventEmitter::innerInvokeEventListeners (the native emit behind
process.on) called JSC::call through its NakedPtr<Exception>
overload, which clears unconditionally, then handed the exception to
a reporter that ignores terminations. Both now call the plain
overload and leave a termination pending (tryClearException and
clearExceptionExceptTermination), so emit() and dispatchEvent()
unwind. A termination that is already pending refuses the next
listener, for a node:vm timeout that keeps the VM's gate open.
The test covers six loop shapes: queueMicrotask, Response body read and
tee, EventTarget, AbortSignal, and process.emit.
Problem
worker.terminate()never completes while the worker runs a microtask-only loop that queues aBunPerformMicrotaskJobon every turn. Bun queues that job forqueueMicrotask(), for the C++ web streams start/pull reactions, and for promise-reject deferrals. Repro in Bun:(async () => { while (true) { new Response('abc').body.getReader().read(); await 0; } })()in anode:worker_threadsWorker. Bun 1.3.14 terminates it in 30 ms; current main hangs 8/9 runs.BunPerformMicrotaskJobarm ofrunInternalMicrotask(Source/JavaScriptCore/runtime/JSMicrotask.cpp). It catches the job's exception with a plaincatchScope.clearException(). When the exception is the VM'sTerminationException, that clear consumes it.VMTraps::handleTrapsalready cleared theNeedTerminationbit, so nothing raises it again.runMicrotask()never sees the termination and the checkpoint keeps draining.Fix
clearExceptionExceptTermination(). On a termination, release the throw scope and return with the exception pending. This is what thePromiseReactionJobarm in the same function does.runMicrotask()then takes theclearExceptionExceptTermination() == falsepath,drainImplclears the queue, andVM::drainMicrotasksreturns to the embedder with the termination pending. Bun takes it at the drain boundary and the worker shuts down.VM::m_exceptionin a hung worker: the only write afterthrowTerminationExceptioncame fromTopExceptionScope::clearExceptionin this arm. In a run that terminated, no such write happened. The Bun-side regression test lands in oven-sh/bun with the pin bump.Background
TerminationExceptionis the exception JSC throws to unwind script when another thread callsVM::notifyNeedTermination().VMTraps::handleTrapsservices the trap once, at the next trap check (aRETURN_IF_EXCEPTIONin native code, or a loop back-edge in JS), and clears the trap bit. There is no second delivery.runInternalMicrotaskkeeps a termination pending forMicrotaskQueue::runMicrotask, which stops the checkpoint on it.BunPerformMicrotaskJobwas the only arm with a bareclearException().Bun__reportUnhandledError(the Bun embedder callback) already ignores a termination exception. It relies on the caller to leave it pending.