src: remove `ClearFatalExceptionHandlers()`#17333

addaleax · 2017-11-27T00:38:18Z

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

addaleax · 2017-11-27T00:39:37Z

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

apapirovski

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

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

addaleax · 2017-11-27T00:46:44Z

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?

apapirovski · 2017-11-27T00:48:32Z

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.

addaleax · 2017-11-27T00:50:05Z

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

TimothyGu · 2017-11-27T01:34:10Z

src/async_wrap.cc

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


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 :)

danbev · 2017-11-27T06:35:37Z

src/node_internals.h


 NO_RETURN voidFatalError(constchar* location, constchar* message);

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


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.)

AndreasMadsen

LGTM

This is really nice, good job :)

bnoordhuis · 2017-11-27T10:11:21Z

src/async_wrap.cc

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


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> voidUSE(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).

bnoordhuis · 2017-11-27T10:13:00Z

src/node_internals.h


 NO_RETURN voidFatalError(constchar* location, constchar* message);

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


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

TimothyGu

Still LGTM.

TimothyGu · 2017-11-27T18:12:28Z

src/node_crypto.cc


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


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

addaleax · 2017-11-29T15:02:05Z

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 <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Andreas Madsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

PR-URL: #17333 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

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 <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Andreas Madsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

PR-URL: #17333 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

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 <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Andreas Madsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

PR-URL: #17333 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

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 pending requests for changes, 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 pending requests for changes, 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 pending requests for changes, 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 pending requests for changes, 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


		NO_RETURN voidFatalError(constchar* location, constchar* message);

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

Uh oh!

src: remove ClearFatalExceptionHandlers()#17333

src: remove ClearFatalExceptionHandlers()#17333

Uh oh!

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

src: remove `ClearFatalExceptionHandlers()`#17333