Skip to content

fix(worker): carry the deserialization failure in a worker's messageerror data - #489

Merged
edusperoni merged 1 commit into
mainfrom
fix/messageerror-data
Oct 5, 2026
Merged

edusperoni merged 1 commit into
mainfrom
fix/messageerror-data

Conversation

@edusperoni

@edusperoni edusperoni commented Oct 5, 2026 •

Copy link
Copy Markdown
Collaborator

A message that cannot be deserialized on the receiving side fires messageerror. On a MessagePort that event's data is the thrown exception (NativeMessagePort::Drain), which is also what Node hands port.on('messageerror') and worker.on('messageerror'). On a Worker — parent-side worker.onmessageerror and the worker scope's onmessageerror — Worker::OnMessageCallback discarded the exception and delivered null.

The node:worker_threads shim forwards event.data as the listener argument, so worker.on('messageerror', err) and parentPort.on('messageerror', err) received null where Node gives the Error, while a plain MessageChannel port on the same runtime gave the Error.

Worker::OnMessageCallback now mirrors the port path: the caught exception when there is one, null otherwise (a thrown undefined included, since #477 made delivery store the payload as given).

Why no new spec

Every receiver-side failure is guarded on the sender or only reachable during teardown: ports are validated against the transfer list before posting, host objects degrade or reject at write time, the consumed_ double-read guard needs a fan-out that cannot carry transferables, and the DOMException rebuild only fails once the builtin can no longer load. The one organic trigger would be V8's recursive deserializer hitting its stack check on a thread with less stack than the sender's, and probing that showed a worker thread overflows its real stack (SIGBUS) before V8's limit fires — a separate problem, reported separately. Exercising this path deterministically needs a corrupt-stream test hook, which is more product surface than this fix warrants.

Full suite on fix/messageerror-data: 1741 specs, 0 failures (iOS 18.5 simulator).

Follow-up to #477.

Summary by CodeRabbit

  • Bug Fixes
    • Message deserialization errors now include the caught exception as the messageerror event data. If no exception is available, the event data is null.

…rror data

A message a worker (or its parent) cannot read fires messageerror with
data === null, while the same failure on a MessagePort delivers the
thrown exception as data (NativeMessagePort::Drain), which is also what
Node hands worker.on('messageerror'). The node:worker_threads shim
forwards event.data unchanged, so its Worker and parentPort surfaces
received null where Node gives the Error.

Worker::OnMessageCallback now mirrors the port path: the caught
exception when there is one, null otherwise.
@coderabbitai

coderabbitai Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 5774e334-3626-42c4-89e2-c443a6b4f375
📥 Commits

Reviewing files that changed from the base of the PR and between 6f06243 and e3471b1.

📒 Files selected for processing (1)
  • NativeScript/runtime/Worker.mm

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

When message deserialization fails, Worker::OnMessageCallback sets the messageerror event’s data to the caught exception when available. If the value is absent or undefined, it sets data to null.

Changes

Worker message error data

Layer / File(s) Summary
Set deserialization error event data
NativeScript/runtime/Worker.mm
On deserialization failure, the messageerror event uses the caught exception as data when available. An absent or undefined value becomes null. The event type and empty ports remain unchanged.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: adrian-niculescu

Merge Risk: ⚪ Minimal · up to e3471

Worker messageerror events now carry the deserialization exception when available and use null otherwise. No material merge risk is identified.

Architecture Summary

Architecture risk: 🔵 Low · up to e3471

The change affects 1 system.

Changed systems: NativeScript

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — NativeScript (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in NativeScript/runtime/Worker.mm: On deserialization failure, data now carries the caught exception when available; an empty or undefined value is converted to null. This replaces the previous behavior of always setting data to null.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: passing deserialization failures in a worker's messageerror data.
  • Autopilot · 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 reads the worker’s note
An error rides where null once sat
If none appears, null takes its place
The message keeps its type and pace
Empty ports remain, neat as a hat
The rabbit hops and checks the stack

Comment @coderabbitai help to get the list of available commands.

@edusperoni
edusperoni merged commit 4ae32eb into main Oct 5, 2026
8 checks passed
@edusperoni
edusperoni deleted the fix/messageerror-data branch October 5, 2026 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant