Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 5b92eb4

Browse filesBrowse files
joyeecheungtargos
joyeecheung
authored and
targos
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 b6a7052 commit 5b92eb4
Copy full SHA for 5b92eb4

File tree

Expand file treeCollapse file tree

16 files changed

+190
-158
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

16 files changed

+190
-158
lines changed
Open diff view settings
Collapse file

‎lib/internal/bootstrap/node.js‎

Copy file name to clipboardExpand all lines: lib/internal/bootstrap/node.js
+8-2Lines 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 =
Collapse file

‎lib/internal/main/worker_thread.js‎

Copy file name to clipboardExpand all lines: lib/internal/main/worker_thread.js
+13-10Lines 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');
@@ -151,24 +151,23 @@ port.on('message', (message) => {
151151
}
152152
});
153153

154-
// Overwrite fatalException
155-
process._fatalException = (error, fromPromise) => {
156-
debug(`[${threadId}] gets fatal exception`);
157-
let caught = false;
154+
function workerOnGlobalUncaughtException(error, fromPromise) {
155+
debug(`[${threadId}] gets uncaught exception`);
156+
let handled = false;
158157
try {
159-
caught = originalFatalException.call(this, error, fromPromise);
158+
handled = onGlobalUncaughtException(error, fromPromise);
160159
} catch (e) {
161160
error = e;
162161
}
163-
debug(`[${threadId}] fatal exception caught = ${caught}`);
162+
debug(`[${threadId}] uncaught exception handled = ${handled}`);
164163

165-
if (!caught) {
164+
if (!handled) {
166165
let serialized;
167166
try {
168167
const { serializeError } = require('internal/error-serdes');
169168
serialized = serializeError(error);
170169
} catch {}
171-
debug(`[${threadId}] fatal exception serialized = ${!!serialized}`);
170+
debug(`[${threadId}] uncaught exception serialized = ${!!serialized}`);
172171
if (serialized)
173172
port.postMessage({
174173
type: ERROR_MESSAGE,
@@ -182,7 +181,11 @@ process._fatalException = (error, fromPromise) => {
182181

183182
process.exit();
184183
}
185-
};
184+
}
185+
186+
// Patch the global uncaught exception handler so it gets picked up by
187+
// node::errors::TriggerUncaughtException().
188+
process._fatalException = workerOnGlobalUncaughtException;
186189

187190
markBootstrapComplete();
188191

Collapse file

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

Copy file name to clipboardExpand all lines: lib/internal/modules/cjs/loader.js
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -829,7 +829,7 @@ Module.runMain = function() {
829829
return loader.import(pathToFileURL(process.argv[1]).pathname);
830830
})
831831
.catch((e) => {
832-
internalBinding('task_queue').triggerFatalException(
832+
internalBinding('errors').triggerUncaughtException(
833833
e,
834834
true /* fromPromise */
835835
);
Collapse file

‎lib/internal/process/execution.js‎

Copy file name to clipboardExpand all lines: lib/internal/process/execution.js
+9-5Lines 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
};
Collapse file

‎lib/internal/process/promises.js‎

Copy file name to clipboardExpand all lines: lib/internal/process/promises.js
+24-24Lines 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() {
Collapse file

‎lib/internal/process/task_queues.js‎

Copy file name to clipboardExpand all lines: lib/internal/process/task_queues.js
+8-7Lines 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
}
Collapse file

‎src/api/exceptions.cc‎

Copy file name to clipboardExpand all lines: src/api/exceptions.cc
+5-10Lines 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
Collapse file

‎src/env.cc‎

Copy file name to clipboardExpand all lines: src/env.cc
+1-1Lines 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
Collapse file

‎src/js_stream.cc‎

Copy file name to clipboardExpand all lines: src/js_stream.cc
+5-5Lines 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
}
Collapse file

‎src/node.cc‎

Copy file name to clipboardExpand all lines: src/node.cc
+3-5Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,6 @@ using options_parser::kDisallowedInEnvironment;
121121

122122
using v8::Boolean;
123123
using v8::EscapableHandleScope;
124-
using v8::Exception;
125124
using v8::Function;
126125
using v8::FunctionCallbackInfo;
127126
using v8::HandleScope;
@@ -212,10 +211,9 @@ MaybeLocal<Value> ExecuteBootstrapper(Environment* env,
212211
arguments->size(),
213212
arguments->data());
214213

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

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.