Repository navigation
fix(worker): leave the inspector pause loop when the worker is terminating - #2071
Open
adrian-niculescu wants to merge 4 commits into
Open
adrian-niculescu wants to merge 4 commits into
adrian-niculescu wants to merge 4 commits into
Conversation
Terminate() read the worker isolate and then interrupted it with no lock, while the worker thread could clear the pointer and dispose that isolate in the same window: a worker that ends through its own close() while its parent calls terminate(), or a parent that terminates its children during its own shutdown. Terminate() now holds a mutex across the read and the use, and the worker thread takes it when it withdraws the isolate, before disposing it.
close() only ends the worker once its running callback returns, and terminate() returned early for a closing worker, so a worker that kept running after close() could not be stopped. terminate() now interrupts it like any other running worker.
…spec in its timeout A run where terminate() failed left the worker spinning past the spec. The loop now also checks a stop flag the spec raises in afterEach, the spec gets its own Jasmine timeout with a shorter start deadline, and the start-deadline branch fails through an expectation rather than the fail() global this Jasmine does not provide.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (7)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughThe worker runtime now permits termination after a worker begins closing and coordinates isolate access during termination and shutdown. A new test checks that the worker’s shared counter stops advancing after termination. ChangesWorker termination
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant WorkerTest
participant workerCloseThenSpinWorker
participant WorkerWrapper
participant WorkerInspectorClient
WorkerTest->>workerCloseThenSpinWorker: Send shared counter buffer
workerCloseThenSpinWorker->>workerCloseThenSpinWorker: Call close and increment counter
WorkerTest->>WorkerWrapper: Terminate worker
WorkerWrapper->>WorkerWrapper: Interrupt isolate and notify event loop
WorkerInspectorClient->>WorkerInspectorClient: Check termination flag and exit pause loop
WorkerTest->>WorkerTest: Check that the counter remains unchanged
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change fixes a worker termination deadlock and allows a worker that has called close() to be terminated. No merge-blocking risk is evident from the supplied context. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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. A rabbit watched the worker spin, Comment |
…ating A breakpoint hit while console.log converts its arguments pauses the worker inside ConsoleLog, which holds the worker's inspector mutex. terminate() could only end that pause through NotifyTerminating, which takes the same mutex, so the parent's terminate() blocked until the debugger resumed. The pause loop now also leaves on the worker's own termination flag, which terminate() sets before it takes the mutex.
adrian-niculescu
force-pushed
the
fix/worker-terminate-while-paused
branch
from
October 6, 2026 23:29
fb1b203 to
e8d3dab
Compare
adrian-niculescu
marked this pull request as ready for review
October 6, 2026 23:32
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In a debug build, a worker paused at a breakpoint inside
console.log(for example adebuggerstatement in a getter that the message text conversion runs) makes the parent'sworker.terminate()block until the debugger resumes, freezing the parent's thread.WorkerWrapper::ConsoleLogholdsinspectorMutex_acrossWorkerInspectorClient::consoleLog, so the pause loop runs with that mutex held.Terminate()ends a pause throughNotifyTerminating(), which it reaches by taking the same mutex, so it waits for the pause, and the pause only waits for the debugger. The pause loop now also leaves on the worker's ownisTerminating_flag, whichTerminate()sets before it takes the mutex and which the loop checks on each pass while it waits. The paused worker then returns fromconsoleLog, the mutex is released, andTerminate()continues.This builds on #2067, whose commits are the first three here. Leaving the pause on the flag lets the worker run on to its own teardown while the parent may still be interrupting its isolate, and #2067 is what keeps the isolate alive until the parent is done with it.
With the pause entered by hand at that point, the parent's
terminate()onmainblocks inmutex::lock()while the worker sits inrunMessageLoopOnPause, and with this change it returns and the worker ends. Reproducing it for real needs a DevTools session paused inside the getter, so there is no spec for it. The full device suite passes.Summary by CodeRabbit
close(), including while paused in debugging.