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

src: refactor uncaught exception handling #28257

Closed
wants to merge 2 commits into from

Conversation

@joyeecheung
Copy link
Member

commented Jun 17, 2019

The C++ land node::FatalException() is not in fact fatal anymore.
It gives the user a chance to handle the uncaught exception
globally by listening to the uncaughtException event. This patch
renames it to TriggerUncaughtException in C++ to avoid the confusion.

In addition rename the JS land handler to onGlobalUncaughtException
to reflect its purpose - we have to keep the alias
process._fatalException and use that for now since it has been
monkey-patchable in the user land.

This patch also

  • Adds more comments to the global uncaught exception handling routine
  • Puts a few other C++ error handling functions into the errors
    namespace
  • Moves error-handling-related bindings to the errors binding.

Refs: 2b252ac

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
src: refactor uncaught exception handling
The C++ land `node::FatalException()` is not in fact fatal anymore.
It gives the user a chance to handle the uncaught exception
globally by listening to the `uncaughtException` event. This patch
renames it to `TriggerUncaughtException` in C++ to avoid the confusion.

In addition rename the JS land handler to `onGlobalUncaughtException`
to reflect its purpose - we have to keep the alias
`process._fatalException` and use that for now since it has been
monkey-patchable in the user land.

This patch also

- Adds more comments to the global uncaught exception handling routine
- Puts a few other C++ error handling functions into the `errors`
  namespace
- Moves error-handling-related bindings to the `errors` binding.

Refs: 2b252ac
@nodejs-github-bot

This comment has been minimized.

Copy link

commented Jun 17, 2019

Sadly, an error occurred when I tried to trigger a build. :(

triggerFatalException(error, false /* fromPromise */);
// runInAsyncScope() swallows the error so we need to catch
// it and handle it here.
triggerUncaughtException(error, false /* fromPromise */);

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Jun 17, 2019

Author Member

@devsnek I tried a cctest on V8 master and it seems the issue still exist. However we still need to catch the error even when it's fixed since runInAsyncScope() will swallow the error.

src/node_errors.cc Outdated Show resolved Hide resolved
src/node_errors.cc Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2019

joyeecheung added a commit that referenced this pull request Jun 19, 2019
src: refactor uncaught exception handling
The C++ land `node::FatalException()` is not in fact fatal anymore.
It gives the user a chance to handle the uncaught exception
globally by listening to the `uncaughtException` event. This patch
renames it to `TriggerUncaughtException` in C++ to avoid the confusion.

In addition rename the JS land handler to `onGlobalUncaughtException`
to reflect its purpose - we have to keep the alias
`process._fatalException` and use that for now since it has been
monkey-patchable in the user land.

This patch also

- Adds more comments to the global uncaught exception handling routine
- Puts a few other C++ error handling functions into the `errors`
  namespace
- Moves error-handling-related bindings to the `errors` binding.

Refs: 2b252ac

PR-URL: #28257
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

Landed in a33c3c6

targos added a commit that referenced this pull request Jul 2, 2019
src: refactor uncaught exception handling
The C++ land `node::FatalException()` is not in fact fatal anymore.
It gives the user a chance to handle the uncaught exception
globally by listening to the `uncaughtException` event. This patch
renames it to `TriggerUncaughtException` in C++ to avoid the confusion.

In addition rename the JS land handler to `onGlobalUncaughtException`
to reflect its purpose - we have to keep the alias
`process._fatalException` and use that for now since it has been
monkey-patchable in the user land.

This patch also

- Adds more comments to the global uncaught exception handling routine
- Puts a few other C++ error handling functions into the `errors`
  namespace
- Moves error-handling-related bindings to the `errors` binding.

Refs: 2b252ac

PR-URL: #28257
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@targos targos referenced this pull request Jul 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.