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

Add a way to downgrade custom errors to native errors (limitation of HTML structured clone algorithm) #37988

Closed
Prinzhorn opened this issue Mar 30, 2021 · 10 comments
Labels
feature request Issues that request new features to be added to Node.js. stale worker Issues and PRs related to Worker support.

Comments

@Prinzhorn
Copy link

Is your feature request related to a problem? Please describe.

I'm using Worker Threads. When the thread crashes with a custom error it is not properly serialized. The following demonstrates the difference (using postMessage, the result is the same as with the error event). I'm using verror because it's very popular library, but this applies to everything that is not a native Error.

index.js

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

const worker = new Worker(path.join(__dirname, 'worker.js'));

worker.on('message', (msg) => {
  console.log('Message:');
  console.log(msg);
});

worker.on('error', (err) => {
  console.error('Error:');
  console.error(err);
});

worker.on('exit', (code) => {
  console.log('Exit code:');
  console.log(code);
});

worker.js

const { parentPort } = require('worker_threads');
const VError = require('verror');

parentPort.postMessage(new Error('Native Error'));
parentPort.postMessage(new VError('VError'));
npm i verror
node index.js

Message:
Error: Native Error
    at Object.<anonymous> (/home/alex/src/issues/node-worker-custom-error/worker.js:4:24)
    at Module._compile (internal/modules/cjs/loader.js:1063:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1092:10)
    at Module.load (internal/modules/cjs/loader.js:928:32)
    at Function.Module._load (internal/modules/cjs/loader.js:769:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12)
    at MessagePort.<anonymous> (internal/main/worker_thread.js:174:24)
    at MessagePort.[nodejs.internal.kHybridDispatch] (internal/event_target.js:354:41)
    at MessagePort.exports.emitMessage (internal/per_context/messageport.js:18:26)

Message:
{ jse_shortmsg: 'VError', jse_info: {}, message: 'VError' }

This can be very unexpected and the stack information is lost. In reality the problem I'm running into is even worse. It's not with verror but with a custom SqliteError that has it's properties as enumerable: false. This means only an empty {} will arrive in the other thread. So even if I've never thrown anything that is not an Error I still end up with not-an-error inside the error event.

Describe the solution you'd like

According to MDN the fact that Error is serializable at all is already a blessing

Native Error types can be cloned in Chrome, and Firefox is working on it.

And I assume it's not possible to serialize custom errors in general because the class might not even exist in the other thread.

What I wish for is one of these two:

  1. Node.js (V8?) walks the prototype chain and sees if it finds an Error. I'm not suggesting doing this for arbitrary objects and MDN says "The prototype chain is not walked or duplicated.". But I think subclasses of Error deserve that special treatment
  2. Something along the lines of valueOf and toJSON that can be used to aid the cloning algorithm, so downgrading the Error would be in the hands of library authors

Describe alternatives you've considered

I've tried downgrading the errors in uncaughtException event and throwing the native Error, but you cannot have an uncaught exception inside uncaughtException and have the thread crash with the new error instead.
And this solutions doesn't help others debugging the same issue and only seeing {} on their error logs.

@github-actions
Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Mar 24, 2022
@github-actions
Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@Prinzhorn
Copy link
Author

I still think this is very valid, it's just that Workers are still rarely used in the grand scheme of the Node.js ecosystem. In addition you also need to extend the Error class to run into issues. As Workers become more popular more (subtle) bugs in libraries will surface as a result of the way the structured cloning algorithm works right now.

@github-actions github-actions bot removed the stale label Mar 29, 2022
@github-actions
Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Sep 26, 2022
@Prinzhorn
Copy link
Author

My dearest bot, this is still a relevant feature request. But my comment above still holds true, Workers + custom error class are a rare combination, but I'm sure people are already running into it and don't realize (e.g. if you use ava and use custom errors you will).

@github-actions github-actions bot removed the stale label Sep 27, 2022
@github-actions
Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Mar 27, 2023
@github-actions
Copy link
Contributor

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Apr 26, 2023
@CiroGomes
Copy link

Same problem here. I have validation errors and a simple if like this: if (err instanceof ValidationError) doesn't work when using workers, because err is an generic error at this point. Is there some workaround?

@BridgeAR
Copy link
Member

Restoring a custom class is almost impossible to do right. There is no native ValidationError and VError is a custom object that does not represent a native error, so restoring it is not possible.

@Prinzhorn
Copy link
Author

@BridgeAR I want to point out that this is not what this issue is about. I never talked about restoring the original class, that is impossible. But currently you get a plain object and not an error at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. stale worker Issues and PRs related to Worker support.
Projects
None yet
Development

No branches or pull requests

5 participants