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): tear down a worker runtime whose Java bootstrap failed - #2069
A worker whose bootstrap fails after its native runtime is up, for example because internal/ts_helpers.js throws, reports the error to its parent but leaves the native runtime and its isolate allocated for the rest of the process.
Runtime.initWorkerRuntime creates the native runtime in runtime.init() and then runs ts_helpers.js. When that throws, the Java side rolls back its own caches and rethrows, so the call never returns the runtime id, WorkerWrapper::runtime_ stays null, and BackgroundLooper skips the teardown that disposes the isolate. The native runtime is still the thread's current runtime (PrepareV8Runtime sets it last, and UnwindFailedInit clears it when native init itself fails), so shutdown now takes it from there when runtime_ is null and tears it down on the existing path, detaching it from Java by its own id. That id matters when the failure comes after the Java runtime finished initializing, for example a logger that throws on its last line: Java still holds that runtime and its GC subscription, and detaching with the -1 the call never replaced would leave both pointing at the deleted native runtime.
With ts_helpers.js patched to throw in workers, a failed worker on main leaves its runtime current after shutdown, and with this change the runtime is gone and the error still reaches the parent once. That needs a patched asset, so there is no spec for it. The full device suite passes.
Summary by CodeRabbit
Bug Fixes
Improved cleanup when app startup fails, helping prevent a native runtime from being left undisposed. This includes cases where a native runtime is created but startup does not return its identifier; shutdown can now locate the runtime and complete the existing cleanup steps.
When initWorkerRuntime built the native runtime and then failed in Java, for example because internal/ts_helpers.js threw, it threw before returning the runtime id, so the worker never took ownership and the native runtime and its isolate were never disposed. Worker shutdown now takes that runtime from the thread's current one and tears it down like any other.
No actionable comments were generated in the recent review.
Walkthrough
WorkerWrapper::BackgroundLooper now recovers the current runtime and its ID during shutdown if Java bootstrap fails before assigning runtime_. The recovered runtime can then follow the existing detach and disposal path.
Changes: Runtime shutdown recovery
Layer / File(s)
Summary
Recover runtime during shutdown test-app/runtime/src/main/cpp/WorkerWrapper.cpp
When Java bootstrap fails before assigning runtime_, shutdown obtains the current thread’s runtime and ID when available. The existing cleanup path then handles the recovered runtime.
NativeScript/android#2092: This PR changes the thread-local runtime lookup used by the shutdown recovery path.
Suggested reviewers:edusperoni
Merge Risk: Minimal · up to 11269
This change tears down a worker runtime whose Java bootstrap failed, so the runtime no longer leaks after shutdown. The fallback is narrow and guarded, and no merge-blocking risk was identified.
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 change: a fix that tears down a worker runtime when its Java bootstrap fails. The raw summary says BackgroundLooper now recovers the current runtime and its ID during shutdown so t…
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 change: a fix that tears down a worker runtime when its Java bootstrap fails. The raw summary says BackgroundLooper now recovers the current runtime and its ID during shutdown so the existing detach and disposal cleanup runs for a partially initialized worker. The objectives describe the same leak and fix. The title is specific and matches the main change.
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 84d8a99 and 1126907.
Files selected for processing: 1
test-app/runtime/src/main/cpp/WorkerWrapper.cpp
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.
The bootstrap can also fail after the Java runtime finished initializing, when the logger throws on its last line, and the Java side then still holds the runtime and its GC subscription. Detaching with the id the call never returned left both behind, so a later GC notification could reach the deleted native runtime.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
The reason will be displayed to describe this comment to others. Learn more.
Reviewed and verified. Reproduced on device with internal/ts_helpers.js patched to throw on worker threads: three failed bootstraps each recovered the runtime, tore it down (current runtime null afterwards), reported exactly one error to the parent and fired nsworkerended, with no crash. Full device suite passes on main + #2067 + #2070 + #2069 (1545 specs, 0 failures, 3 runs).
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.
edusperoni
left a comment
A worker whose bootstrap fails after its native runtime is up, for example because
internal/ts_helpers.jsthrows, reports the error to its parent but leaves the native runtime and its isolate allocated for the rest of the process.Runtime.initWorkerRuntimecreates the native runtime inruntime.init()and then runsts_helpers.js. When that throws, the Java side rolls back its own caches and rethrows, so the call never returns the runtime id,WorkerWrapper::runtime_stays null, andBackgroundLooperskips the teardown that disposes the isolate. The native runtime is still the thread's current runtime (PrepareV8Runtimesets it last, andUnwindFailedInitclears it when native init itself fails), so shutdown now takes it from there whenruntime_is null and tears it down on the existing path, detaching it from Java by its own id. That id matters when the failure comes after the Java runtime finished initializing, for example a logger that throws on its last line: Java still holds that runtime and its GC subscription, and detaching with the-1the call never replaced would leave both pointing at the deleted native runtime.With
ts_helpers.jspatched to throw in workers, a failed worker onmainleaves its runtime current after shutdown, and with this change the runtime is gone and the error still reaches the parent once. That needs a patched asset, so there is no spec for it. The full device suite passes.Summary by CodeRabbit