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

if uncaughtExceptionMonitor handler throws, the process dies without chance for recovery by uncaughtException or uncaughtRejection #38280

Open
MadaraUchiha opened this issue Apr 18, 2021 · 4 comments
Labels
process Issues and PRs related to the process subsystem.

Comments

@MadaraUchiha
Copy link
Contributor

  • Version: v15.14.0
  • Platform: all
  • Subsystem: process/execution

What steps will reproduce the bug?

  1. Register an uncaughtExceptionMonitor handler on process, which itself throws
  2. Throw an error without catching it

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

Every time.

What is the expected behavior?

uncaughtException event on process to be fired, if error is handled don't crash the process.

  • Ideally with an AggregateError containing both errors
  • But at least with the original error

What do you see instead?

The process simply dies with error code 7. uncaughtException isn't emitted.

Additional information

Example file:

process.addListener('uncaughtExceptionMonitor', (e) => {
  console.error('Monitored Error!', e);
  throw new Error('Oopsie!');

});

process.addListener('uncaughtException', e => {
  console.error('Handled error!', e);
})
let i = 0;
setInterval(() => console.log(i++), 1000);
throw new Error('Thrown!');

Expected

Monitored: ...
Handled: ...
0
1
2
...

Actual

Monitored: ...
/path/to/test.js:3
  throw new Error('Oopsie!');
  ^

Error: Oopsite!
    at ...

$ # process died, error code is 7
@MadaraUchiha
Copy link
Contributor Author

I am working on a PR for this, to be submitted in the near future.

@Ayase-252 Ayase-252 added the process Issues and PRs related to the process subsystem. label Apr 18, 2021
@benjamingr
Copy link
Member

I'm not sure this is not by design @nodejs/post-mortem

@Linkgoron
Copy link
Member

Linkgoron commented Apr 19, 2021

I'm not sure this is not by design @nodejs/post-mortem

#31257 (comment)

There was some discussion around it, but maybe now that AggregateError exists, there are other options.

@MadaraUchiha
Copy link
Contributor Author

Additionally, today, the process exits with error code 7 (which is different from the normal 1 of an uncaught exception that the uncaughtException handler declined to handle).

I believe this behavior should be preserved, since it's an outwards facing API that's enforced by an existing test. See https://github.com/nodejs/node/blob/master/test/parallel/test-process-uncaught-exception-monitor.js#L32.

WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
process Issues and PRs related to the process subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants