Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Errors emitted on worker are not native errors #48716

Open
novemberborn opened this issue Jul 9, 2023 · 3 comments
Open

Errors emitted on worker are not native errors #48716

novemberborn opened this issue Jul 9, 2023 · 3 comments
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. worker Issues and PRs related to Worker support.

Comments

@novemberborn
Copy link

novemberborn commented Jul 9, 2023

Version

20.4.0

Platform

Darwin user 22.5.0 Darwin Kernel Version 22.5.0: Thu Jun 8 22:22:20 PDT 2023; root:xnu-8796.121.3~7/RELEASE_ARM64_T6000 arm64 arm Darwin

Subsystem

worker_threads

What steps will reproduce the bug?

import {Worker} from 'node:worker_threads'
import {once} from 'node:events'
import types from 'node:util/types'

const worker = new Worker('setImmediate(() => { throw new Error("error"); })', {eval: true})
const [error] = await once(worker, 'message')

console.log(error instanceof Error) // true
console.log(types.isNativeError(error)) // false

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

Always, as long as the error is emitted by the error event on the worker. Any errors sent via postMessage() are identified correctly.

What is the expected behavior? Why is that the expected behavior?

types.isNativeError() returns true for the emitted error.

What do you see instead?

types.isNativeError() returns false for the emitted error.

Additional information

The problem seems to be in the error deserialization:

return ObjectCreate(ctor.prototype, properties);

Reading the code, I believe it's roughly equivalent to:

const error = Object.create(Error.prototype, {message:{value:'foo'}})

According to the spec, merely extending the error prototype does not create a native error.

Of course, because it does extend the Error prototype, error instanceof Error returns true.

I don't have the context for why the serialization is so complex, but FWIW the built-in v8 serde does result in proper native error objects.

@novemberborn
Copy link
Author

One more thing, this logic here sets a string tag:

ObjectDefineProperty(properties, SymbolToStringTag, {
__proto__: null,
value: { __proto__: null, value: 'Error', configurable: true },
enumerable: true,
});

Regular errors don't have string tags:

console.log((new Error())[Symbol.toStringTag]) // undefined

@atlowChemi atlowChemi added errors Issues and PRs related to JavaScript errors originated in Node.js core. worker Issues and PRs related to Worker support. labels Jul 9, 2023
@nivb52
Copy link

nivb52 commented Jul 10, 2023

@atlowChemi
Hi, seems interesting,
Can I work on it ?

Thanks,
Niv

@atlowChemi
Copy link
Member

@nivb52 Sure, PRs are welcomed 🙂

novemberborn added a commit to avajs/ava that referenced this issue Jul 30, 2023
They're not native due to nodejs/node#48716, but we want to treat them as such anyway.
novemberborn added a commit to avajs/ava that referenced this issue Jul 31, 2023
* Track worker errors. They're not native due to nodejs/node#48716, but we want to treat them as such anyway.
* Only treat native errors as errors
* Remove is-error dependency
* Document edge case where `error instanceof Error` can be true, yet AVA does not recognize `error` as an error

See also #2911 for an earlier attempt.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. worker Issues and PRs related to Worker support.
Projects
None yet
Development

No branches or pull requests

3 participants