You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(worker): keep the worker isolate alive while terminate() uses it, and let it stop a closing worker - #2067
Calling terminate() on a Worker that is ending on its own at the same moment, for example through its own close(), can make the parent thread interrupt the worker's isolate after the worker thread has already disposed it. A parent that terminates its child workers during its own shutdown races the same way. Separately, terminate() cannot stop a worker that called close() and kept running, so close(); while (true) {} spins forever.
WorkerWrapper::Terminate reads workerIsolate_ and then calls TerminateExecution() and the event-loop lookup on it with no lock, while BackgroundLooper clears the pointer and disposes the isolate during teardown. Nothing makes the worker thread wait for a Terminate that has already read the pointer. Terminate now holds a mutex across the read and the use, and the worker thread takes the same mutex when it clears the pointer, before disposing the isolate: a terminate that already read it finishes with it first, and a later one finds null. This is the same fix as NativeScript/ios#478 on iOS.
close() only ends the worker once its running callback returns, and Terminate returned early for a closing worker. It no longer returns early for a closing worker, which matches the iOS runtime, so it interrupts one that is still running.
The isolate race window is a few instructions wide, so it has no spec. With a sleep injected between the read and the use, a worker that closes itself while terminate() runs is already disposed when the parent touches its isolate on main, and stays alive until the parent is done with this change. The new spec for a worker spinning after close() fails on main and passes here, and the full device suite passes.
Summary by CodeRabbit
Bug Fixes
Workers that have called close() but continue running can now be terminated reliably. Their ongoing activity stops after termination is requested.
Tests
Added coverage verifying that termination stops a closing worker’s ongoing activity. The test checks that its shared counter remains unchanged after termination.
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.
No actionable comments were generated in the recent review.
Walkthrough
Worker termination can now proceed after a worker calls close(), with isolate access synchronized against shutdown. The app test suite adds a worker test that checks whether termination stops shared-counter increments.
Changes: Worker termination after close
Layer / File(s)
Summary
Synchronize termination and isolate shutdown test-app/runtime/src/main/cpp/WorkerWrapper.h, test-app/runtime/src/main/cpp/WorkerWrapper.cpp
Terminate proceeds unless the worker is disposed and holds workerIsolateMutex_ while using the isolate. Shutdown uses the same mutex when clearing the isolate pointer.
Test termination after close test-app/app/src/main/assets/app/tests/workerCloseThenSpinWorker.js, test-app/app/src/main/assets/app/tests/testWorkerTerminateAfterClose.js, test-app/app/src/main/assets/app/mainpage.js
The worker calls close() and increments a shared counter until signaled to stop. The test terminates the worker after observing increments and checks that the counter remains unchanged. The app loads the test module.
NativeScript/android#2044: Changes the worker termination and shutdown sequence in WorkerWrapper.cpp, including when the worker root and registry entry are cleared.
Suggested reviewers:edusperoni
Merge Risk: Minimal · up to 7e3a4
This change synchronizes worker termination with isolate shutdown and lets terminate() stop a worker that has called close(). A regression test accompanies it. No actionable merge-blocking risk remains.
Pre-merge checks: 4
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title names the two changes in the PR: keeping the worker isolate alive while terminate() uses it, and allowing terminate() to stop a worker that called close(). The raw summary confirms both: the…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Title check
Explanation
The title names the two changes in the PR: keeping the worker isolate alive while terminate() uses it, and allowing terminate() to stop a worker that called close(). The raw summary confirms both: the WorkerWrapper mutex guards isolate use and teardown, and the early return for closing workers is removed. The title is long at 102 characters, but it is specific and accurate.
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Reviewing files that changed from the base of the PR and between 44f3c8c and 7e3a456.
Files selected for processing: 3
test-app/app/src/main/assets/app/mainpage.js
test-app/runtime/src/main/cpp/WorkerWrapper.cpp
test-app/runtime/src/main/cpp/WorkerWrapper.h
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
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.
adrian-niculescu
changed the title
fix(worker): keep the worker isolate alive while terminate() uses it
fix(worker): keep the worker isolate alive while terminate() uses it, and let it stop a closing worker
Oct 6, 2026
…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.
The reason will be displayed to describe this comment to others. Learn more.
Reviewed and verified: merged with current main, full device suite passes on main + #2067 + #2070 + #2069 (1545 specs, 0 failures, 3 runs). The earlier CI Test failure did not reproduce on rerun.
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
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.
adrian-niculescu
changed the title
Calling
terminate()on a Worker that is ending on its own at the same moment, for example through its ownclose(), can make the parent thread interrupt the worker's isolate after the worker thread has already disposed it. A parent that terminates its child workers during its own shutdown races the same way. Separately,terminate()cannot stop a worker that calledclose()and kept running, soclose(); while (true) {}spins forever.WorkerWrapper::TerminatereadsworkerIsolate_and then callsTerminateExecution()and the event-loop lookup on it with no lock, whileBackgroundLooperclears the pointer and disposes the isolate during teardown. Nothing makes the worker thread wait for aTerminatethat has already read the pointer.Terminatenow holds a mutex across the read and the use, and the worker thread takes the same mutex when it clears the pointer, before disposing the isolate: a terminate that already read it finishes with it first, and a later one finds null. This is the same fix as NativeScript/ios#478 on iOS.close()only ends the worker once its running callback returns, andTerminatereturned early for a closing worker. It no longer returns early for a closing worker, which matches the iOS runtime, so it interrupts one that is still running.The isolate race window is a few instructions wide, so it has no spec. With a sleep injected between the read and the use, a worker that closes itself while
terminate()runs is already disposed when the parent touches its isolate onmain, and stays alive until the parent is done with this change. The new spec for a worker spinning afterclose()fails onmainand passes here, and the full device suite passes.Summary by CodeRabbit
close()but continue running can now be terminated reliably. Their ongoing activity stops after termination is requested.