worker_threads parentPort extends EventTarget in Node 14 but extends EventEmitter in Node 12

[snowinferno]

• Version: v14.15.0
• Platform: Darwin SuperSecretHostname 19.6.0 Darwin Kernel Version 19.6.0: Mon Aug 31 22:12:52 PDT 2020; root:xnu-6153.141.2~1/RELEASE_X86_64 x86_64
• Subsystem: worker_threads

What steps will reproduce the bug?

Run this script under node v14.15.0

const { isMainThread, parentPort, Worker } = require('worker_threads');

if (isMainThread) {
    const worker = new Worker(__filename);
    worker.on('error', (message) => console.dir(message));
} else {
    parentPort.emit('error', "This is my error message.");
}

How often does it reproduce? Is there a required condition?

Every time the script is run.

Required condition: Node v14.15.0

What is the expected behavior?

The error should reflect Error [ERR_UNHANDLED_ERROR]: Unhandled error. ('This is my error message.') with a stack trace.

What do you see instead?

The error reflects TypeError [Error]: parentPort.emit is not a function with a stack trace.
parentPort seemingly does not extend EventEmitter as documentation indicates and behavior under node v12.19.0 shows.

Additional information

This appears to be related to commit 0aa3809b6be on the v14.x branch

This would be a breaking change in a stable API since this same script behaves as expected under Node v12.19.0.

Inspecting parentPort I find that contrary to expectation and documentation, it extends EventTarget instead of EventEmitter.

MessagePort [EventTarget] {
  [Symbol(kEvents)]: Map(2) {
    'newListener' => { size: 1, next: [Listener] },
    'removeListener' => { size: 1, next: [Listener] }
  },
  [Symbol(kMaxListeners)]: 10,
  [Symbol(kMaxListenersWarned)]: false,
  [Symbol(kNewListener)]: [Function (anonymous)],
  [Symbol(kRemoveListener)]: [Function (anonymous)]
}

[indutny]

cc @addaleax @jasnell

[addaleax]

So … I’m conflicted – on the one hand, this is working as expected. MessagePort instances are supposed to be EventTargets, not EventEmitters, we just didn’t have EventTarget when we introduced it in the first place.

That being said, the fact that .emit() was being dropped was not something I had considered when I made the switch, and I’m surprised to realize that it’s missing from NodeEventTarget. I’m not sure if we want to implement that or not, given that it’s not exactly a straightforward thing to do once Web-style listeners come into play (e.g. what subclass of Event should a .addEventListener() listener receive when somebody uses .emit('error', error)?) /cc @benjamingr @jasnell

(Either way, I’ll have to go update my slides for nodeconf.remote next week … 😄)

[addaleax]

Also: Yes, this is definitely missing documentation updates. I’ll get onto that.

@snowinferno Is this causing any real-world trouble? I’m surprised that parentPort is being used to emit custom events in any case…

[snowinferno]

It's not causing real world trouble yet.

The company I work for is mainly on Node 10 and 12 while we prepare for and test Node 14.

I happened to be using node 14 when I ran a test with worker_thread code to perform some tasks. In the event an error happens during processing, we're using parentPort.emit('error', error) to bubble it back to the main thread through worker.on('error',...).
This seemed like the most fitting way to get the error back to the main thread instead of sending a message like

{
  "message": "some error message",
  "isWarning": true|false,
  "isError": true|flase,
  "isFatal": true|false,
  "error": { Error object }
}

[snowinferno]

The module I'm working on is in a state where it is being picked up by application builds but the method of triggering it is not yet user-friendly.

If the decision is to leave the worker_thread implementation as-is, we will have to prioritize changing how errors get bubbled up before we make Node 14 available for production applications.

[benjamingr]

. I’m not sure if we want to implement that or not, given that it’s not exactly a straightforward thing to do once Web-style listeners come into play.

I think it's fine to shim this (emit) for convenience.

(e.g. what subclass of Event should a .addEventListener() listener receive when somebody uses .emit('error', error)?)

Probably CustomEvent with Error in its detail (like browser unhandledrejection events) though we don't have that yet I guess?

(Either way, I’ll have to go update my slides for nodeconf.remote next week … 😄)

Good luck with your talk!

[addaleax]

In the event an error happens during processing, we're using parentPort.emit('error', error) to bubble it back to the main thread through worker.on('error',...).
This seemed like the most fitting way to get the error back to the main thread instead of sending a message like

Ok, but just so we’re on the same page – parentPort.emit('error', error) does not lead to worker.on('error') being emitted. worker.on('error') is only emitted for uncaught exceptions inside the Worker. What happens is that emitting an 'error' on any EventEmitter that does not have an 'error' listener will be turned into an exception. So what you’re doing is essentially equivalent to just writing throw error;.

[benjamingr]

@addaleax

I think it's fine to shim this (emit) for convenience.

BTW Would it help (you) at all if I worked this? (made a PR that adds emit to NodeEventTarget and wraps a CustomEvent?)

(I am happy to do it if it's helpful)

[addaleax]

@benjamingr I mean, I’d get to it at some point, but feel free to pick this up if you like :)

[benjamingr]

Hmm, one possible difference is support for multiple args - I'll do what I can and I'll make a PR to dicusss the "product" of how "emit" can look.

[benjamingr]

@addaleax how would you feel about a NodeCustomEvent type (that would play the role of MessageEvents except higher up?

It would be the default for kCreateEvent that MessagePort overrides.

Also - we currently use data on MessageEvents but the DOM uses .detail on CustomEvent, which would be better as a property name?

[addaleax]

@addaleax how would you feel about a NodeCustomEvent type (that would play the role of MessageEvents except higher up?

The name sounds good to me :)

It would be the default for kCreateEvent that MessagePort overrides.

That’s fine, although we should make sure that the kCreateEvent in MessagePort calls that base class function if it doesn’t recognize type (i.e. type is not in message, messageerror).

Also - we currently use data on MessageEvents but the DOM uses .detail on CustomEvent, which would be better as a property name?

I think detail is just fine if we stick with CustomEvent – .data is just what we use because it’s what MessageEvents are supposed to have :)

[benjamingr]

Opened a PR in #35851 with a bunch of questions

[snowinferno]

Ok, but just so we’re on the same page – parentPort.emit('error', error) does not lead to worker.on('error') being emitted. worker.on('error') is only emitted for uncaught exceptions inside the Worker. What happens is that emitting an 'error' on any EventEmitter that does not have an 'error' listener will be turned into an exception. So what you’re doing is essentially equivalent to just writing throw error;.

@addaleax Ahh, I see. So we are actually creating a side-effect that just happened to bubble up through worker.on('error'...).
In light of that, I'll schedule some work to remove this side-effect logic and handle bubbling the errors up in a more appropriate way.