fix(worker): route worker_threads errors like the web surface, and fire once listeners once - #491
Conversation
Sorry, something went wrong.
…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.
|
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 4ae32eb and e823dff. 📒 Files selected for processing (3)
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 WalkthroughWorker event emission now reports whether listeners were registered and prevents a once-listener from firing again during nested emission. Worker-scope error forwarding uses rethrowing dispatch. New regression tests cover these event and error cases. ChangesWorker event handling
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: edusperoni Merge Risk: ⚪ Minimal · up to e823d The reviewed worker error and once-listener paths show no actionable merge-blocking issue. Merge after normal checks. 🚥 Pre-merge checks | ✅ 5 ✅ Passed checks (5 passed)
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 taps a message through, Comment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
… 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.
|
Thanks, the once-wrapper and relay changes look right. I checked the once semantics against Node 24.18 and lib/events.js on main: the fired flag matches _onceWrap, and the nested-emit case produces the same call sequence. Two things before merging:
Not for this PR, just recording the remaining gap: Node terminates the worker after an uncaught throw and emits exit with code 1. We keep the worker alive and exit is always 0. No spec covers either side of that. |
Sorry, something went wrong.
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.
An error that a worker.on("error") listener handled was also reported to the parent's global scope as unhandled, because the Worker's onerror returned nothing. It now returns whether a listener ran, which cancels the event, and the emitter reports that the way Node's emit does.
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 Android is NativeScript/android#2065. All three new specs fail on main and pass here, and the full TestRunner suite passes.
Summary by CodeRabbit