src: remove `ClearFatalExceptionHandlers()` by addaleax · Pull Request #17333 · nodejs/node · GitHub

At its call sites, ClearFatalExceptionHandlers() was used to
make the process crash as soon as possible once an exception occurred,
without giving JS land a chance to interfere.

ClearFatalExceptionHandlers() awkwardly removed the current domain
and any uncaughtException handlers, whereas a clearer way is to
execute the relevant reporting (and exit()) code directly.

Refs: #17159
Refs: #17324

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

src

At its call sites, `ClearFatalExceptionHandlers()` was used to make the process crash as soon as possible once an exception occurred, without giving JS land a chance to interfere. `ClearFatalExceptionHandlers()` awkwardly removed the current domain and any `uncaughtException` handlers, whereas a clearer way is to execute the relevant reporting (and `exit()`) code directly. Refs: nodejs#17159 Refs: nodejs#17324

CI: https://ci.nodejs.org/job/node-test-commit/14381/

This is a very clean way of doing this. Nice 👍 🥇

One question: would this still emit an exit event on the process?

One question: would this still emit an exit event on the process?

Hm; I think the answer is “no, and that is what we want, even if it happened before” – all of these sound like they should be hard crashes due to Node being in an invalid state?

Hm; I think the answer is “no, and that is what we want, even if it happened before” – all of these sound like they should be hard crashes due to Node being in an invalid state?

Yea, that's fine but I just want to flag the difference in case there's any reason we would care about it. I don't know if anyone could really be relying on that event in this particular situation.

@apapirovski I’m glad you pointed it out. :)

Should ToLocalChecked() be used here? It would make the program hard-crash before the FatalTryCatch got time to terminate the program with error message... right?

@TimothyGu Yeah, you’re right. Updated :)

Nit: Perhaps this could rephrased to something like "Like a TryCatch but crashes the process if an exception was caught"?

I'd also write "exits" instead of "crashes" because the latter suggests segfaults and aborts (in my mind at least.)

LGTM

This is really nice, good job :)

The (void)... is to suppress the -Wunused-result warning? That works with clang but gcc still emits a warning, AFAIK.

@bnoordhuis It appears this depends on the gcc version: gcc 6 doesn’t complain, gcc 7 does? I think older versions of it were also fine with it…

I’ve went with a dummy .FromMaybe call, that doesn’t seem quite as pretty but I’m not sure what a better way would be

It used to be a long-standing gcc bug that was only partially fixed: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=25509 from 2005.

I suppose now might be a good a time as any to introduce a Use() or USE() function:

template <typename T> void USE(T&&) {}

Good idea, bad idea? If bad idea, I'm fine with (void)....

It’s a good idea, I’ve added a new commit with that (both for the code bits here and those where we use (void).

I'd also write "exits" instead of "crashes" because the latter suggests segfaults and aborts (in my mind at least.)

Still LGTM.

You could move this comment to where USE() is defined.

Yup, done.

Landed in a012672, 04e3aa2

At its call sites, `ClearFatalExceptionHandlers()` was used to make the process crash as soon as possible once an exception occurred, without giving JS land a chance to interfere. `ClearFatalExceptionHandlers()` awkwardly removed the current domain and any `uncaughtException` handlers, whereas a clearer way is to execute the relevant reporting (and `exit()`) code directly. PR-URL: #17333 Refs: #17159 Refs: #17324 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

PR-URL: #17333 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

At its call sites, `ClearFatalExceptionHandlers()` was used to make the process crash as soon as possible once an exception occurred, without giving JS land a chance to interfere. `ClearFatalExceptionHandlers()` awkwardly removed the current domain and any `uncaughtException` handlers, whereas a clearer way is to execute the relevant reporting (and `exit()`) code directly. PR-URL: #17333 Refs: #17159 Refs: #17324 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

PR-URL: #17333 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

At its call sites, `ClearFatalExceptionHandlers()` was used to make the process crash as soon as possible once an exception occurred, without giving JS land a chance to interfere. `ClearFatalExceptionHandlers()` awkwardly removed the current domain and any `uncaughtException` handlers, whereas a clearer way is to execute the relevant reporting (and `exit()`) code directly. PR-URL: #17333 Refs: #17159 Refs: #17324 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

PR-URL: #17333 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

addaleax added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v6.x lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 27, 2017

addaleax requested a review from apapirovski November 27, 2017 00:38

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 27, 2017

addaleax mentioned this pull request Nov 27, 2017

process: add capture function for uncaught exceptions #17159

Closed

4 tasks

apapirovski approved these changes Nov 27, 2017

View reviewed changes

apapirovski mentioned this pull request Nov 27, 2017

events: optimize for performance, remove _events use outside of events #17324

Closed

4 tasks

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Nov 27, 2017

TimothyGu reviewed Nov 27, 2017

View reviewed changes

addaleax removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Nov 27, 2017

[squash] TimothyGu comment

9ca5ccb

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Nov 27, 2017

TimothyGu approved these changes Nov 27, 2017

View reviewed changes

danbev reviewed Nov 27, 2017

View reviewed changes

danbev approved these changes Nov 27, 2017

View reviewed changes

AndreasMadsen approved these changes Nov 27, 2017

View reviewed changes

bnoordhuis reviewed Nov 27, 2017

View reviewed changes

addaleax added 2 commits November 27, 2017 14:36

[squash] danbev & bnoordhuis nits

b41845a

src: introduce USE() for silencing compiler warnings

64acf54

bnoordhuis approved these changes Nov 27, 2017

View reviewed changes

TimothyGu approved these changes Nov 27, 2017

View reviewed changes

[squash] TimothyGu nit

a2e4472

jasnell approved these changes Nov 27, 2017

View reviewed changes

cjihrig approved these changes Nov 29, 2017

View reviewed changes

addaleax removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Nov 29, 2017

addaleax closed this Nov 29, 2017

addaleax deleted the no-clear-fatal-exception-handlers branch November 29, 2017 15:02

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

-                }
+                FatalTryCatch try_catch(env);
+                fn->Call(env->context(), Undefined(env->isolate()), 1, &async_id_value)
+                    .ToLocalChecked();


		NO_RETURN void FatalError(const char* location, const char* message);

		// Like a `TryCatch` but it crashes the process if it did catch an exception.

-                  UNREACHABLE();
-                }
+                FatalTryCatch try_catch(env);
+                (void)fn->Call(env->context(), Undefined(env->isolate()), 1, &async_id_value);

                 MarkPopErrorOnReturn mark_pop_error_on_return;
-                (void) &mark_pop_error_on_return;  // Silence compiler warning.
+                USE(&mark_pop_error_on_return);  // Silence compiler warning.

src: remove `ClearFatalExceptionHandlers()` by addaleax · Pull Request #17333 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: remove ClearFatalExceptionHandlers() - #17333

src: remove ClearFatalExceptionHandlers()#17333addaleax wants to merge 5 commits intonodejs:masternodejs/node:masterfrom addaleax:no-clear-fatal-exception-handlersaddaleax/node:no-clear-fatal-exception-handlersCopy head branch name to clipboard

Conversation

addaleax commented Nov 27, 2017

Checklist

Affected core subsystem(s)

Uh oh!

addaleax commented Nov 27, 2017

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Nov 27, 2017

Uh oh!

apapirovski commented Nov 27, 2017

Uh oh!

addaleax commented Nov 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndreasMadsen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Nov 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

src: remove ClearFatalExceptionHandlers()#17333
addaleax wants to merge 5 commits into
nodejs:masterfrom
addaleax:no-clear-fatal-exception-handlers