src: remove ClearFatalExceptionHandlers()
#17333
Conversation
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
There was a problem hiding this comment.
This is a very clean way of doing this. Nice 👍 🥇
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? |
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. :) |
src/async_wrap.cc
Outdated
| } | ||
| FatalTryCatch try_catch(env); | ||
| fn->Call(env->context(), Undefined(env->isolate()), 1, &async_id_value) | ||
| .ToLocalChecked(); |
There was a problem hiding this comment.
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?
src/node_internals.h
Outdated
|
|
||
| NO_RETURN void FatalError(const char* location, const char* message); | ||
|
|
||
| // Like a `TryCatch` but it crashes the process if it did catch an exception. |
There was a problem hiding this comment.
Nit: Perhaps this could rephrased to something like "Like a TryCatch but crashes the process if an exception was caught"?
There was a problem hiding this comment.
I'd also write "exits" instead of "crashes" because the latter suggests segfaults and aborts (in my mind at least.)
There was a problem hiding this comment.
LGTM
This is really nice, good job :)
src/async_wrap.cc
Outdated
| UNREACHABLE(); | ||
| } | ||
| FatalTryCatch try_catch(env); | ||
| (void)fn->Call(env->context(), Undefined(env->isolate()), 1, &async_id_value); |
There was a problem hiding this comment.
The (void)... is to suppress the -Wunused-result warning? That works with clang but gcc still emits a warning, AFAIK.
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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)....
There was a problem hiding this comment.
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).
src/node_internals.h
Outdated
|
|
||
| NO_RETURN void FatalError(const char* location, const char* message); | ||
|
|
||
| // Like a `TryCatch` but it crashes the process if it did catch an exception. |
There was a problem hiding this comment.
I'd also write "exits" instead of "crashes" because the latter suggests segfaults and aborts (in my mind at least.)
src/node_crypto.cc
Outdated
|
|
||
| MarkPopErrorOnReturn mark_pop_error_on_return; | ||
| (void) &mark_pop_error_on_return; // Silence compiler warning. | ||
| USE(&mark_pop_error_on_return); // Silence compiler warning. |
There was a problem hiding this comment.
You could move this comment to where USE() is defined.
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>
At its call sites,
ClearFatalExceptionHandlers()was used tomake the process crash as soon as possible once an exception occurred,
without giving JS land a chance to interfere.
ClearFatalExceptionHandlers()awkwardly removed the current domainand any
uncaughtExceptionhandlers, whereas a clearer way is toexecute the relevant reporting (and
exit()) code directly.Refs: #17159
Refs: #17324
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
src