fix(worker): carry the deserialization failure in a worker's messageerror data - #489
edusperoni merged 1 commit into
Conversation
Sorry, something went wrong.
…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.
|
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
Reviewing files that changed from the base of the PR and between 6f06243 and e3471b1. 📒 Files selected for processing (1)
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 WalkthroughWhen 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. ChangesWorker message error data
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 SummaryArchitecture risk: 🔵 Low · up to e3471 The change affects 1 system. Changed systems: NativeScript Architecture concerns Systems and components
Before / after behavior
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. ❤️ ShareA rabbit reads the worker’s note Comment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
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