[worker_threads]: Main thread receives Error object when the worker throws a primitive value

[suin]

• Version: v14.7.0
• Platform: Darwin suin 18.7.0 Darwin Kernel Version 18.7.0: Mon Feb 10 21:08:45 PST 2020; root:xnu-4903.278.28~1/RELEASE_X86_64 x86_64
• Subsystem: N/A

What steps will reproduce the bug?

Summary: Since Node v14.7.0, when primitive values are thrown in a worker, the main thread receives it as Error objects.

I'm not sure this is actually a bug. But I couldn't find out the information about this breaking change. So I have created this issue. If this is an expected change, please close this issue.

// main.js
const { Worker } = require('worker_threads')
const worker = new Worker('./worker.js')
console.log(process.versions.node)
worker.on('error', err => {
  console.error(err)
})

// worker.js
throw 'Error was thrown in worker'

in Node v14.6.0:

$ node main.js
14.6.0
Error was thrown in worker

in Node v14.7.0:

$ node main.js
14.7.0
Error [ERR_UNHANDLED_ERROR]: Unhandled error. ('Error was thrown in worker')
    at process.emit (events.js:303:17)
    at emitUnhandledRejectionOrErr (internal/event_target.js:541:11)
    at MessagePort.[nodejs.internal.kHybridDispatch] (internal/event_target.js:356:9)
    at MessagePort.exports.emitMessage (internal/per_context/messageport.js:18:26) {
  code: 'ERR_UNHANDLED_ERROR',
  context: 'Error was thrown in worker'
}

Other primitives:

Undefined

// worker.js
throw undefined

$ node main.js
14.6.0
undefined

$ node main.js
14.7.0
Error [ERR_UNHANDLED_ERROR]: Unhandled error. (undefined)
    at process.emit (events.js:303:17)
    at emitUnhandledRejectionOrErr (internal/event_target.js:541:11)
    at MessagePort.[nodejs.internal.kHybridDispatch] (internal/event_target.js:356:9)
    at MessagePort.exports.emitMessage (internal/per_context/messageport.js:18:26) {
  code: 'ERR_UNHANDLED_ERROR',
  context: undefined
}

Number

// worker.js
throw 0

$ node main.js
14.6.0
0

$ node main.js
14.7.0
Error [ERR_UNHANDLED_ERROR]: Unhandled error. (0)
    at process.emit (events.js:303:17)
    at emitUnhandledRejectionOrErr (internal/event_target.js:541:11)
    at MessagePort.[nodejs.internal.kHybridDispatch] (internal/event_target.js:356:9)
    at MessagePort.exports.emitMessage (internal/per_context/messageport.js:18:26) {
  code: 'ERR_UNHANDLED_ERROR',
  context: 0
}

Symbol

// worker.js
throw Symbol('this is a symbol')

$ node main.js
14.6.0
Symbol(this is a symbol)

$ node main.js
14.7.0
Error [ERR_UNHANDLED_ERROR]: Unhandled error. (Symbol(this is a symbol))
    at process.emit (events.js:303:17)
    at emitUnhandledRejectionOrErr (internal/event_target.js:541:11)
    at MessagePort.[nodejs.internal.kHybridDispatch] (internal/event_target.js:356:9)
    at MessagePort.exports.emitMessage (internal/per_context/messageport.js:18:26)

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

There is no condition.

What is the expected behavior?

I'm not sure which is valid behavior, but at point of view of backward compatibilities, it would be the expected behavior that the main thread receives the unhandled primitive error as the primitive value instead of the Error object like the Node v14.6.0 behavior.

What do you see instead?

The main thread gets the Error object.

Additional information

[aduh95]

Interestingly, when using data: URL, the primitive object is passed to the error handler as in 14.6.0:

const { Worker } = require('worker_threads');

console.log(process.versions.node);
new Worker(new URL('data:text/javascript,throw 4')).on('error', err => {
  console.error(err);
});

$ node main.js
15.0.0-pre
4

[codebytere]

Looks like a consequence of 0aa3809b6b - i don't think this should be expected behavior but @addaleax can maybe confirm?

[addaleax]

Right – this should not be happening, it’s a bug 👍

[addaleax]

Ok, so basically the problem here is that, because the main script inside a Worker is started from within a .on('message') event handler, it ends up in the EventTarget error handling chain, which is basically emitting a process.on('error') event first, and if there are no handlers, wrapping the exception and treating it as an uncaught one.

I basically see 2 ways to solve this:

1. Delay the execution of the LOAD_SCRIPT handler with a nextTick. This would make errors thrown inside of it it, including during the main script, be emitted as regular uncaught exceptions.
2. Don’t use the emitUnhandledRejectionOrErr() mechanism at all for Node.js-style event listeners. It’s somewhat unexpected that the exception handling behavior of a object.on('foo') listener depends on whether object is an EventEmitter or NodeEventTarget.

In particular, this did not just change the behavior for the main script unintentionally, but also for exceptions thrown inside any port.on('message') listeners.

@jasnell @benjamingr Thoughts?

[addaleax]

ping @jasnell @benjamingr any further thoughts? I’d lean towards option 2, if nobody has a strong opinion.

[benjamingr]

I lean towards option #2 as well with no strong opinions.

[jasnell]

Missed the original ping on this... no strong opinions on either option.

[yashLadha]

This is not coming in current master, should we go ahead and create a minor version patch for this?

[benjamingr]

@yashLadha do you mind clarifying what you mean by that?

[yashLadha]

@benjamingr I meant to say that if this issue exists in an older version of node we can backport the fix to that tree, if it feels viable.

[benjamingr]

@yashLadha was this issue actually resolved in master? I don't see any PRs or work relating to it.