GeezerVison is showing github.com — fix(worker): route worker_threads errors like the web surface, and fire once listeners once by edusperoni · Pull Request #2089 · NativeScript/android
Conversation
edusperoni
commented
Nov 5, 2026
•
edited by coderabbitai
Bot
Loading
Uh oh!
There was an error while loading. Please reload this page.
Supersedes #2065, which GitHub closed when its base branch (feat/worker-threads) was deleted at the #2043 squash. The four commits are @adrian-niculescu's, replayed onto main with authorship kept, plus one comment fix.
A parentPort.on("message") listener that throws never reaches the worker's onerror or the parent's worker.on("error"). The relay dispatched without rethrowing, so the throw went to the uncaught-error reporter and stopped there. It now dispatches the way worker-global message delivery does.
A worker.on("error") listener received the runtime's ErrorEvent instead of an Error, and an error it handled was also reported to the parent's global scope as unhandled. The worker now sends the thrown value's name and message with the error payload, and the parent rebuilds an Error from them and the worker's stack, with the built-in constructor when the name is a built-in one. The listener receives that Error, as in Node. The emitter's emit reports whether a listener ran, as Node's does, and the Worker cancels the event when one did. An error nothing handled reaches the parent's global error event carrying the same Error.
A once listener could fire twice: when an earlier listener emitted the same event again, the nested emit fired and removed it, and the outer emit then called it a second time from its snapshot. A once registration now records that it fired, as Node's once wrapper does, so a once listener an earlier listener removed still fires, as in Node.
The same fix for iOS is NativeScript/ios#491. Replaying onto main meant reconciling with #2044's nsworkerended callout. WorkerEvents.h declares both the new EmitError (which returns the rebuilt error, or undefined when handled) and EmitEnded. worker-events.js exports { emitMessage, emitError, emitEnded, kWorkerError }, and the Worker row in docs/worker-threads.md keeps the exit sentence and adds the rebuilt-Error one. worker-events.js and node-worker-threads.js now match iOS main apart from platform comments and iOS's ns:worker_threads require (ios#494, not ported). The extra commit carries the #reportExit comment fix from the ios#491 review, which hadn't reached the Android branch.
Merge after #2088. This branch is stacked on it and #2087, so until those squashes land the diff here also shows their commits. Full device suite with all three applied: 1467 specs, 0 failures.
Summary by CodeRabbit
-
Bug Fixes
- Message delivery now preserves
undefined and other falsy values across messaging channels, and distinguishes delivered data from default event data.
- Unreadable messages report a
messageerror with a consistent null fallback when no error value is available.
- Worker errors retain their name, message, and stack when available. Unhandled errors reach the parent scope; handled errors are suppressed there.
- Objects marked as uncloneable are recognized during cloning, including when marked after serialization setup.
- Worker error listeners now handle reentrant one-time registrations consistently.
-
Documentation
- Clarified how worker errors are reconstructed and delivered to listeners.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
coderabbitai
Bot
commented
Nov 5, 2026
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|

Warning
Review limit reached
You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.
Next included review available in 29 minutes.
Check out review usage here.
View limit details
-
Limit details: You’ve used all 4 included reviews currently available.
Learn how review limits work.
Review configuration:
Run configuration
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
9e629320-ab8a-40ba-bedb-c89fa5056da8
Commits
-
Reviewing files that changed from the base of the PR and between b94958f and 1ec06a9.
Files selected for processing: 18
-
docs/ns-builtin-modules.md
docs/worker-threads.md
eslint.config.mjs
test-app/app/src/main/assets/app/tests/messaging/domExceptionThrowingWorker.js
test-app/app/src/main/assets/app/tests/messaging/onerrorRethrowingWorker.js
test-app/app/src/main/assets/app/tests/messaging/parentPortThrowingWorker.js
test-app/app/src/main/assets/app/tests/testMessaging.js
test-app/runtime/src/main/cpp/ArgConverter.h
test-app/runtime/src/main/cpp/CallbackHandlers.cpp
test-app/runtime/src/main/cpp/NativeScriptException.cpp
test-app/runtime/src/main/cpp/NsBuiltinModules.cpp
test-app/runtime/src/main/cpp/WorkerEvents.cpp
test-app/runtime/src/main/cpp/WorkerEvents.h
test-app/runtime/src/main/cpp/WorkerWrapper.cpp
test-app/runtime/src/main/cpp/WorkerWrapper.h
test-app/runtime/src/main/cpp/js/node-worker-threads.js
test-app/runtime/src/main/cpp/js/primordials.js
test-app/runtime/src/main/cpp/js/worker-events.js
Walkthrough
-
Message delivery now preserves payload values such as undefined and updates handling of uncloneable objects. Worker error forwarding carries thrown values and details across isolates, then rebuilds and routes errors to parent-side listeners.
Changes: Messaging and worker events
| Layer / File(s) |
Summary |
Message payload delivery and cloning test-app/runtime/src/main/cpp/js/message-event.js, test-app/runtime/src/main/cpp/js/message-channel.js, test-app/runtime/src/main/cpp/js/broadcast-channel.js, test-app/runtime/src/main/cpp/js/node-worker-threads.js, test-app/runtime/src/main/cpp/StructuredSerialization.cpp, test-app/runtime/src/main/cpp/Messaging.cpp, test-app/runtime/src/main/cpp/WorkerEvents.cpp, test-app/runtime/src/main/cpp/WorkerEvents.h, test-app/app/src/main/assets/app/tests/messaging/*, test-app/app/src/main/assets/app/tests/testMessaging.js |
A new message-event factory preserves the supplied data for delivered events, including undefined. Deserialization now uses null for missing or undefined messageerror data, and transferred values are cleared only on the single-receiver path. Tests cover falsy payloads and objects marked uncloneable during cloning. |
Worker exception capture and forwarding test-app/runtime/src/main/cpp/ArgConverter.h, test-app/runtime/src/main/cpp/CallbackHandlers.cpp, test-app/runtime/src/main/cpp/NativeScriptException.cpp, test-app/runtime/src/main/cpp/WorkerWrapper.cpp, test-app/runtime/src/main/cpp/WorkerWrapper.h, test-app/app/src/main/assets/app/tests/messaging/domExceptionThrowingWorker.js, test-app/app/src/main/assets/app/tests/messaging/onerrorRethrowingWorker.js, test-app/app/src/main/assets/app/tests/messaging/parentPortThrowingWorker.js |
The worker path extracts error names and messages from thrown values and forwards the value and details to the parent. It preserves UTF-16 text and catches errors from stack access without replacing the original exception. |
Worker error reconstruction and routing test-app/runtime/src/main/cpp/WorkerEvents.cpp, test-app/runtime/src/main/cpp/WorkerEvents.h, test-app/runtime/src/main/cpp/WorkerWrapper.cpp, test-app/runtime/src/main/cpp/js/worker-events.js, test-app/runtime/src/main/cpp/js/node-worker-threads.js, test-app/runtime/src/main/cpp/js/primordials.js, test-app/runtime/src/main/cpp/NsBuiltinModules.cpp, docs/ns-builtin-modules.md, docs/worker-threads.md, eslint.config.mjs, test-app/app/src/main/assets/app/tests/testMessaging.js |
Worker errors are rebuilt with matching built-in constructors where available, and other names are set on a generic Error. An "error" listener receives the rebuilt error and cancels global reporting when registered. The emitter also prevents a once listener from firing again during reentrant emissions. Documentation and tests describe and check these behaviors. |
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix
Possibly related PRs
Merge Risk: Moderate · previous review b9495
-
Cloning an object whose getter first marks another object uncloneable can use a stale handle and potentially crash. Fix the handle lifetime before merging.
Pre-merge checks: 4 · 1
-
Failed checks: 1 warning
| Check name |
Status |
Explanation |
Resolution |
| Docstring Coverage |
 |
Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… |
Write docstrings for the functions missing them to satisfy the coverage threshold. |
Passed checks: 4 passed
-
| Check name |
Status |
Explanation |
| Description Check |
 |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check |
 |
The title names the two changes in this PR: routing worker_threads errors the way the web worker surface does, and making once listeners fire at most once. The changeset covers both. The error routing… |
| 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 this PR: routing worker_threads errors the way the web worker surface does, and making once listeners fire at most once. The changeset covers both. The error routing spans WorkerWrapper, WorkerEvents, worker-events.js, node-worker-threads.js, and the error tests. The once-listener change is in WorkerEmitter in node-worker-threads.js, with tests in testMessaging.js. The title is specific and does not overstate the change, though the PR also includes message-delivery, structured-clone, and documentation work that the title omits, which is acceptable since it need not cover every detail.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 24 functions across 24 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- 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.
Share
-
A rabbit sends undefined along, The little message keeps its form. A worker’s error crosses the stream, Rebuilt for listeners on the other end. I thump my paws: the tests all gleam.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…errors like the web surface
A parentPort listener that threw was dispatched without rethrowing, so the error went to the uncaught-error reporter and never reached the worker's onerror or its parent. The relay now dispatches the way worker-global message delivery does.
The worker_threads Worker's onerror handler returned nothing, so an error its 'error' listeners took was also reported to the parent's global scope as unhandled. It returns whether a listener ran, which cancels the event.
… reentrant emit
An earlier listener that emitted the same event again let the nested emit fire and remove a later once listener, and the outer emit then called it a second time from its snapshot. A once registration now records that it fired, as Node's once wrapper does.
…rom the thrown value
A worker_threads 'error' listener received the runtime's ErrorEvent, not an Error, and the message it carried was V8's "Uncaught Error: ..." text. The worker now reads the thrown value's name and message and forwards them with the error payload. The parent rebuilds an Error from them with the worker's stack, using the built-in constructor the name belongs to, and the parent's global error event carries that same Error when nothing handled it.
The Worker shim cancels an error its listeners took with preventDefault() rather than through the truthy-return contract of onerror.
…r throws
Reading the stack of the error a worker's onerror threw could run a `stack` getter, and a getter that threw replaced that error in the TryCatch holding it, so the parent rebuilt the getter's error instead. The stack read now runs under its own TryCatch.
The thrown value's name and message travel to the parent as UTF-16, so an unpaired surrogate arrives as thrown rather than as U+FFFD. ArgConverter::ConvertToString copies by length, so an embedded NUL no longer cuts a converted string short.
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Nov 11, 2026
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
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.
|