Skip to content

Commit a33c3c6

Browse files
joyeecheungjoyeecheung
committed
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>
1 parent 1c23b6f commit a33c3c6

File tree

16 files changed

+190
-158
lines changed

16 files changed

+190
-158
lines changed

‎lib/internal/bootstrap/node.js

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -254,12 +254,18 @@ Object.defineProperty(process, 'features', {
254254

255255
{
256256
const {
257-
fatalException,
257+
onGlobalUncaughtException,
258258
setUncaughtExceptionCaptureCallback,
259259
hasUncaughtExceptionCaptureCallback
260260
} = require('internal/process/execution');
261261

262-
process._fatalException = fatalException;
262+
// For legacy reasons this is still called `_fatalException`, even
263+
// though it is now a global uncaught exception handler.
264+
// The C++ land node::errors::TriggerUncaughtException grabs it
265+
// from the process object because it has been monkey-patchable.
266+
// TODO(joyeecheung): investigate whether process._fatalException
267+
// can be deprecated.
268+
process._fatalException = onGlobalUncaughtException;
263269
process.setUncaughtExceptionCaptureCallback =
264270
setUncaughtExceptionCaptureCallback;
265271
process.hasUncaughtExceptionCaptureCallback =

‎lib/internal/main/worker_thread.js

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ const {
4242
} = workerIo;
4343

4444
const {
45-
fatalException: originalFatalException
45+
onGlobalUncaughtException
4646
} = require('internal/process/execution');
4747

4848
const publicWorker = require('worker_threads');
@@ -156,24 +156,23 @@ port.on('message', (message) => {
156156
}
157157
});
158158

159-
// Overwrite fatalException
160-
process._fatalException = (error, fromPromise) => {
161-
debug(`[${threadId}] gets fatal exception`);
162-
let caught = false;
159+
function workerOnGlobalUncaughtException(error, fromPromise) {
160+
debug(`[${threadId}] gets uncaught exception`);
161+
let handled = false;
163162
try {
164-
caught = originalFatalException.call(this, error, fromPromise);
163+
handled = onGlobalUncaughtException(error, fromPromise);
165164
} catch (e) {
166165
error = e;
167166
}
168-
debug(`[${threadId}] fatal exception caught = ${caught}`);
167+
debug(`[${threadId}] uncaught exception handled = ${handled}`);
169168

170-
if (!caught) {
169+
if (!handled) {
171170
let serialized;
172171
try {
173172
const { serializeError } = require('internal/error-serdes');
174173
serialized = serializeError(error);
175174
} catch {}
176-
debug(`[${threadId}] fatal exception serialized = ${!!serialized}`);
175+
debug(`[${threadId}] uncaught exception serialized = ${!!serialized}`);
177176
if (serialized)
178177
port.postMessage({
179178
type: ERROR_MESSAGE,
@@ -187,7 +186,11 @@ process._fatalException = (error, fromPromise) => {
187186

188187
process.exit();
189188
}
190-
};
189+
}
190+
191+
// Patch the global uncaught exception handler so it gets picked up by
192+
// node::errors::TriggerUncaughtException().
193+
process._fatalException = workerOnGlobalUncaughtException;
191194

192195
markBootstrapComplete();
193196

‎lib/internal/modules/cjs/loader.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -832,7 +832,7 @@ Module.runMain = function() {
832832
return loader.import(pathToFileURL(process.argv[1]).pathname);
833833
})
834834
.catch((e) => {
835-
internalBinding('task_queue').triggerFatalException(
835+
internalBinding('errors').triggerUncaughtException(
836836
e,
837837
true /* fromPromise */
838838
);

‎lib/internal/process/execution.js

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -115,10 +115,14 @@ function noop() {}
115115
// and exported to be written to process._fatalException, it has to be
116116
// returned as an *anonymous function* wrapped inside a factory function,
117117
// otherwise it breaks the test-timers.setInterval async hooks test -
118-
// this may indicate that node::FatalException should fix up the callback scope
119-
// before calling into process._fatalException, or this function should
120-
// take extra care of the async hooks before it schedules a setImmediate.
121-
function createFatalException() {
118+
// this may indicate that node::errors::TriggerUncaughtException() should
119+
// fix up the callback scope before calling into process._fatalException,
120+
// or this function should take extra care of the async hooks before it
121+
// schedules a setImmediate.
122+
function createOnGlobalUncaughtException() {
123+
// The C++ land node::errors::TriggerUncaughtException() will
124+
// exit the process if it returns false, and continue execution if it
125+
// returns true (which indicates that the exception is handled by the user).
122126
return (er, fromPromise) => {
123127
// It's possible that defaultTriggerAsyncId was set for a constructor
124128
// call that threw and was never cleared. So clear it now.
@@ -206,7 +210,7 @@ module.exports = {
206210
tryGetCwd,
207211
evalModule,
208212
evalScript,
209-
fatalException: createFatalException(),
213+
onGlobalUncaughtException: createOnGlobalUncaughtException(),
210214
setUncaughtExceptionCaptureCallback,
211215
hasUncaughtExceptionCaptureCallback
212216
};

‎lib/internal/process/promises.js

Lines changed: 24 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,6 @@
22

33
const { Object } = primordials;
44

5-
const {
6-
safeToString
7-
} = internalBinding('util');
85
const {
96
tickInfo,
107
promiseRejectEvents: {
@@ -13,10 +10,14 @@ const {
1310
kPromiseResolveAfterResolved,
1411
kPromiseRejectAfterResolved
1512
},
16-
setPromiseRejectCallback,
17-
triggerFatalException
13+
setPromiseRejectCallback
1814
} = internalBinding('task_queue');
1915

16+
const {
17+
noSideEffectsToString,
18+
triggerUncaughtException
19+
} = internalBinding('errors');
20+
2021
// *Must* match Environment::TickInfo::Fields in src/env.h.
2122
const kHasRejectionToWarn = 1;
2223

@@ -127,7 +128,7 @@ function handledRejection(promise) {
127128

128129
const unhandledRejectionErrName = 'UnhandledPromiseRejectionWarning';
129130
function emitUnhandledRejectionWarning(uid, reason) {
130-
const warning = getError(
131+
const warning = getErrorWithoutStack(
131132
unhandledRejectionErrName,
132133
'Unhandled promise rejection. This error originated either by ' +
133134
'throwing inside of an async function without a catch block, ' +
@@ -139,7 +140,8 @@ function emitUnhandledRejectionWarning(uid, reason) {
139140
warning.stack = reason.stack;
140141
process.emitWarning(reason.stack, unhandledRejectionErrName);
141142
} else {
142-
process.emitWarning(safeToString(reason), unhandledRejectionErrName);
143+
process.emitWarning(
144+
noSideEffectsToString(reason), unhandledRejectionErrName);
143145
}
144146
} catch {}
145147

@@ -179,7 +181,9 @@ function processPromiseRejections() {
179181
const { reason, uid } = promiseInfo;
180182
switch (unhandledRejectionsMode) {
181183
case kThrowUnhandledRejections: {
182-
fatalException(reason);
184+
const err = reason instanceof Error ?
185+
reason : generateUnhandledRejectionError(reason);
186+
triggerUncaughtException(err, true /* fromPromise */);
183187
const handled = process.emit('unhandledRejection', reason, promise);
184188
if (!handled) emitUnhandledRejectionWarning(uid, reason);
185189
break;
@@ -209,7 +213,7 @@ function processPromiseRejections() {
209213
pendingUnhandledRejections.length !== 0;
210214
}
211215

212-
function getError(name, message) {
216+
function getErrorWithoutStack(name, message) {
213217
// Reset the stack to prevent any overhead.
214218
const tmp = Error.stackTraceLimit;
215219
Error.stackTraceLimit = 0;
@@ -225,21 +229,17 @@ function getError(name, message) {
225229
return err;
226230
}
227231

228-
function fatalException(reason) {
229-
let err;
230-
if (reason instanceof Error) {
231-
err = reason;
232-
} else {
233-
err = getError(
234-
'UnhandledPromiseRejection',
235-
'This error originated either by ' +
236-
'throwing inside of an async function without a catch block, ' +
237-
'or by rejecting a promise which was not handled with .catch().' +
238-
` The promise rejected with the reason "${safeToString(reason)}".`
239-
);
240-
err.code = 'ERR_UNHANDLED_REJECTION';
241-
}
242-
triggerFatalException(err, true /* fromPromise */);
232+
function generateUnhandledRejectionError(reason) {
233+
const message =
234+
'This error originated either by ' +
235+
'throwing inside of an async function without a catch block, ' +
236+
'or by rejecting a promise which was not handled with .catch().' +
237+
' The promise rejected with the reason ' +
238+
`"${noSideEffectsToString(reason)}".`;
239+
240+
const err = getErrorWithoutStack('UnhandledPromiseRejection', message);
241+
err.code = 'ERR_UNHANDLED_REJECTION';
242+
return err;
243243
}
244244

245245
function listenForRejections() {

‎lib/internal/process/task_queues.js

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,13 @@ const {
99
// Used to run V8's micro task queue.
1010
runMicrotasks,
1111
setTickCallback,
12-
enqueueMicrotask,
13-
triggerFatalException
12+
enqueueMicrotask
1413
} = internalBinding('task_queue');
1514

15+
const {
16+
triggerUncaughtException
17+
} = internalBinding('errors');
18+
1619
const {
1720
setHasRejectionToWarn,
1821
hasRejectionToWarn,
@@ -143,11 +146,9 @@ function runMicrotask() {
143146
try {
144147
callback();
145148
} catch (error) {
146-
// TODO(devsnek): Remove this if
147-
// https://bugs.chromium.org/p/v8/issues/detail?id=8326
148-
// is resolved such that V8 triggers the fatal exception
149-
// handler for microtasks.
150-
triggerFatalException(error, false /* fromPromise */);
149+
// runInAsyncScope() swallows the error so we need to catch
150+
// it and handle it here.
151+
triggerUncaughtException(error, false /* fromPromise */);
151152
} finally {
152153
this.emitDestroy();
153154
}

‎src/api/exceptions.cc

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -244,17 +244,12 @@ Local<Value> WinapiErrnoException(Isolate* isolate,
244244
}
245245
#endif
246246

247+
// Implement the legacy name exposed in node.h. This has not been in fact
248+
// fatal any more, as the user can handle the exception in the
249+
// TryCatch by listening to `uncaughtException`.
250+
// TODO(joyeecheung): deprecate it in favor of a more accurate name.
247251
void FatalException(Isolate* isolate, const v8::TryCatch& try_catch) {
248-
// If we try to print out a termination exception, we'd just get 'null',
249-
// so just crashing here with that information seems like a better idea,
250-
// and in particular it seems like we should handle terminations at the call
251-
// site for this function rather than by printing them out somewhere.
252-
CHECK(!try_catch.HasTerminated());
253-
254-
HandleScope scope(isolate);
255-
if (!try_catch.IsVerbose()) {
256-
FatalException(isolate, try_catch.Exception(), try_catch.Message());
257-
}
252+
errors::TriggerUncaughtException(isolate, try_catch);
258253
}
259254

260255
} // namespace node

‎src/env.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -666,7 +666,7 @@ void Environment::RunAndClearNativeImmediates() {
666666
ref_count++;
667667
if (UNLIKELY(try_catch.HasCaught())) {
668668
if (!try_catch.HasTerminated())
669-
FatalException(isolate(), try_catch);
669+
errors::TriggerUncaughtException(isolate(), try_catch);
670670

671671
// We are done with the current callback. Increase the counter so that
672672
// the steps below make everything *after* the current item part of

‎src/js_stream.cc

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ bool JSStream::IsClosing() {
4949
Local<Value> value;
5050
if (!MakeCallback(env()->isclosing_string(), 0, nullptr).ToLocal(&value)) {
5151
if (try_catch.HasCaught() && !try_catch.HasTerminated())
52-
FatalException(env()->isolate(), try_catch);
52+
errors::TriggerUncaughtException(env()->isolate(), try_catch);
5353
return true;
5454
}
5555
return value->IsTrue();
@@ -65,7 +65,7 @@ int JSStream::ReadStart() {
6565
if (!MakeCallback(env()->onreadstart_string(), 0, nullptr).ToLocal(&value) ||
6666
!value->Int32Value(env()->context()).To(&value_int)) {
6767
if (try_catch.HasCaught() && !try_catch.HasTerminated())
68-
FatalException(env()->isolate(), try_catch);
68+
errors::TriggerUncaughtException(env()->isolate(), try_catch);
6969
}
7070
return value_int;
7171
}
@@ -80,7 +80,7 @@ int JSStream::ReadStop() {
8080
if (!MakeCallback(env()->onreadstop_string(), 0, nullptr).ToLocal(&value) ||
8181
!value->Int32Value(env()->context()).To(&value_int)) {
8282
if (try_catch.HasCaught() && !try_catch.HasTerminated())
83-
FatalException(env()->isolate(), try_catch);
83+
errors::TriggerUncaughtException(env()->isolate(), try_catch);
8484
}
8585
return value_int;
8686
}
@@ -102,7 +102,7 @@ int JSStream::DoShutdown(ShutdownWrap* req_wrap) {
102102
argv).ToLocal(&value) ||
103103
!value->Int32Value(env()->context()).To(&value_int)) {
104104
if (try_catch.HasCaught() && !try_catch.HasTerminated())
105-
FatalException(env()->isolate(), try_catch);
105+
errors::TriggerUncaughtException(env()->isolate(), try_catch);
106106
}
107107
return value_int;
108108
}
@@ -137,7 +137,7 @@ int JSStream::DoWrite(WriteWrap* w,
137137
argv).ToLocal(&value) ||
138138
!value->Int32Value(env()->context()).To(&value_int)) {
139139
if (try_catch.HasCaught() && !try_catch.HasTerminated())
140-
FatalException(env()->isolate(), try_catch);
140+
errors::TriggerUncaughtException(env()->isolate(), try_catch);
141141
}
142142
return value_int;
143143
}

‎src/node.cc

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,6 @@ using options_parser::kDisallowedInEnvironment;
132132

133133
using v8::Boolean;
134134
using v8::EscapableHandleScope;
135-
using v8::Exception;
136135
using v8::Function;
137136
using v8::FunctionCallbackInfo;
138137
using v8::HandleScope;
@@ -218,10 +217,9 @@ MaybeLocal<Value> ExecuteBootstrapper(Environment* env,
218217
arguments->size(),
219218
arguments->data());
220219

221-
// If there was an error during bootstrap then it was either handled by the
222-
// FatalException handler or it's unrecoverable (e.g. max call stack
223-
// exceeded). Either way, clear the stack so that the AsyncCallbackScope
224-
// destructor doesn't fail on the id check.
220+
// If there was an error during bootstrap, it must be unrecoverable
221+
// (e.g. max call stack exceeded). Clear the stack so that the
222+
// AsyncCallbackScope destructor doesn't fail on the id check.
225223
// There are only two ways to have a stack size > 1: 1) the user manually
226224
// called MakeCallback or 2) user awaited during bootstrap, which triggered
227225
// _tickCallback().

‎src/node_api.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ static inline void trigger_fatal_exception(
115115
napi_env env, v8::Local<v8::Value> local_err) {
116116
v8::Local<v8::Message> local_msg =
117117
v8::Exception::CreateMessage(env->isolate, local_err);
118-
node::FatalException(env->isolate, local_err, local_msg);
118+
node::errors::TriggerUncaughtException(env->isolate, local_err, local_msg);
119119
}
120120

121121
class ThreadSafeFunction : public node::AsyncResource {

‎src/node_contextify.cc

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -734,7 +734,7 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
734734
compile_options);
735735

736736
if (v8_script.IsEmpty()) {
737-
DecorateErrorStack(env, try_catch);
737+
errors::DecorateErrorStack(env, try_catch);
738738
no_abort_scope.Close();
739739
if (!try_catch.HasTerminated())
740740
try_catch.ReThrow();
@@ -946,7 +946,7 @@ bool ContextifyScript::EvalMachine(Environment* env,
946946
if (try_catch.HasCaught()) {
947947
if (!timed_out && !received_signal && display_errors) {
948948
// We should decorate non-termination exceptions
949-
DecorateErrorStack(env, try_catch);
949+
errors::DecorateErrorStack(env, try_catch);
950950
}
951951

952952
// If there was an exception thrown during script execution, re-throw it.
@@ -1110,7 +1110,7 @@ void ContextifyContext::CompileFunction(
11101110

11111111
if (maybe_fn.IsEmpty()) {
11121112
if (try_catch.HasCaught() && !try_catch.HasTerminated()) {
1113-
DecorateErrorStack(env, try_catch);
1113+
errors::DecorateErrorStack(env, try_catch);
11141114
try_catch.ReThrow();
11151115
}
11161116
return;

0 commit comments

Comments
 (0)