-
Notifications
You must be signed in to change notification settings - Fork 29.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
async_hooks: simplify fatalError #14051
Changes from all commits
17aafd7
278b163
610d365
1d3f69b
527a4f6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,19 +60,13 @@ async_wrap.setupHooks({ init, | |
destroy: emitDestroyN }); | ||
|
||
// Used to fatally abort the process if a callback throws. | ||
function fatalError(e) { | ||
if (typeof e.stack === 'string') { | ||
process._rawDebug(e.stack); | ||
} else { | ||
const o = { message: e }; | ||
Error.captureStackTrace(o, fatalError); | ||
process._rawDebug(o.stack); | ||
} | ||
if (process.execArgv.some( | ||
(e) => /^--abort[_-]on[_-]uncaught[_-]exception$/.test(e))) { | ||
process.abort(); | ||
} | ||
process.exit(1); | ||
// This is JavaScript version of the ClearFatalExceptionHandlers function | ||
// in C++. | ||
function clearFatalExceptionHandlers(e) { | ||
process.domain = undefined; | ||
// Don't use removeAllListeners in case there is a removeListener listener | ||
// that would revert it. | ||
process._events.uncaughtException = undefined; | ||
} | ||
|
||
|
||
|
@@ -334,7 +328,14 @@ function emitInitS(asyncId, type, triggerAsyncId, resource) { | |
if (!Number.isSafeInteger(triggerAsyncId) || triggerAsyncId < 0) | ||
throw new RangeError('triggerAsyncId must be an unsigned integer'); | ||
|
||
init(asyncId, type, triggerAsyncId, resource); | ||
// Remove prepear for a fatal exception in case an error occurred | ||
var didThrow = true; | ||
try { | ||
init(asyncId, type, triggerAsyncId, resource); | ||
didThrow = false; | ||
} finally { | ||
if (didThrow) clearFatalExceptionHandlers(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure it's correct to move this line from the |
||
} | ||
|
||
// Isn't null if hooks were added/removed while the hooks were running. | ||
if (tmp_active_hooks_array !== null) { | ||
|
@@ -345,15 +346,10 @@ function emitInitS(asyncId, type, triggerAsyncId, resource) { | |
|
||
function emitBeforeN(asyncId) { | ||
processing_hook = true; | ||
// Use a single try/catch for all hook to avoid setting up one per iteration. | ||
try { | ||
for (var i = 0; i < active_hooks_array.length; i++) { | ||
if (typeof active_hooks_array[i][before_symbol] === 'function') { | ||
active_hooks_array[i][before_symbol](asyncId); | ||
} | ||
for (var i = 0; i < active_hooks_array.length; i++) { | ||
if (typeof active_hooks_array[i][before_symbol] === 'function') { | ||
active_hooks_array[i][before_symbol](asyncId); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Allowing an exception to bubble then be handled by a Promise or similar means that it's possible that not all hook callbacks execute, but users that have those unexecuted callbacks won't know that it hasn't been called. Consider the following: const map = new Map();
createHook({
before(id) { map.set(id, /* some object */) },
after(id) { map.delete(id) },
}).enable(); You've just introduced a vector for a memory leak. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BTW If you're touching this you could use |
||
} | ||
} catch (e) { | ||
fatalError(e); | ||
} | ||
processing_hook = false; | ||
|
||
|
@@ -371,31 +367,37 @@ function emitBeforeS(asyncId, triggerAsyncId = asyncId) { | |
|
||
// Validate the ids. | ||
if (asyncId < 0 || triggerAsyncId < 0) { | ||
fatalError('before(): asyncId or triggerAsyncId is less than zero ' + | ||
`(asyncId: ${asyncId}, triggerAsyncId: ${triggerAsyncId})`); | ||
clearFatalExceptionHandlers(); | ||
throw new Error( | ||
'before(): asyncId or triggerAsyncId is less than zero ' + | ||
`(asyncId: ${asyncId}, triggerAsyncId: ${triggerAsyncId})` | ||
); | ||
} | ||
|
||
pushAsyncIds(asyncId, triggerAsyncId); | ||
|
||
if (async_hook_fields[kBefore] === 0) | ||
return; | ||
emitBeforeN(asyncId); | ||
|
||
// Remove prepear for a fatal exception in case an error occurred | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/prepear/prepare |
||
var didThrow = true; | ||
try { | ||
emitBeforeN(asyncId); | ||
didThrow = false; | ||
} finally { | ||
if (didThrow) clearFatalExceptionHandlers(); | ||
} | ||
} | ||
|
||
|
||
// Called from native. The asyncId stack handling is taken care of there before | ||
// this is called. | ||
function emitAfterN(asyncId) { | ||
processing_hook = true; | ||
// Use a single try/catch for all hook to avoid setting up one per iteration. | ||
try { | ||
for (var i = 0; i < active_hooks_array.length; i++) { | ||
if (typeof active_hooks_array[i][after_symbol] === 'function') { | ||
active_hooks_array[i][after_symbol](asyncId); | ||
} | ||
for (var i = 0; i < active_hooks_array.length; i++) { | ||
if (typeof active_hooks_array[i][after_symbol] === 'function') { | ||
active_hooks_array[i][after_symbol](asyncId); | ||
} | ||
} catch (e) { | ||
fatalError(e); | ||
} | ||
processing_hook = false; | ||
|
||
|
@@ -409,8 +411,16 @@ function emitAfterN(asyncId) { | |
// kIdStackIndex. But what happens if the user doesn't have both before and | ||
// after callbacks. | ||
function emitAfterS(asyncId) { | ||
if (async_hook_fields[kAfter] > 0) | ||
emitAfterN(asyncId); | ||
if (async_hook_fields[kAfter] > 0) { | ||
// Remove prepear for a fatal exception in case an error occurred | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/prepear/prepare There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @AndreasMadsen is the comment a TODO? Otherwise I'm not sure I understand what It means. |
||
var didThrow = true; | ||
try { | ||
emitAfterN(asyncId); | ||
didThrow = false; | ||
} finally { | ||
if (didThrow) clearFatalExceptionHandlers(); | ||
} | ||
} | ||
|
||
popAsyncIds(asyncId); | ||
} | ||
|
@@ -427,15 +437,10 @@ function emitDestroyS(asyncId) { | |
|
||
function emitDestroyN(asyncId) { | ||
processing_hook = true; | ||
// Use a single try/catch for all hook to avoid setting up one per iteration. | ||
try { | ||
for (var i = 0; i < active_hooks_array.length; i++) { | ||
if (typeof active_hooks_array[i][destroy_symbol] === 'function') { | ||
active_hooks_array[i][destroy_symbol](asyncId); | ||
} | ||
for (var i = 0; i < active_hooks_array.length; i++) { | ||
if (typeof active_hooks_array[i][destroy_symbol] === 'function') { | ||
active_hooks_array[i][destroy_symbol](asyncId); | ||
} | ||
} catch (e) { | ||
fatalError(e); | ||
} | ||
processing_hook = false; | ||
|
||
|
@@ -460,18 +465,13 @@ function emitDestroyN(asyncId) { | |
// exceptions. | ||
function init(asyncId, type, triggerAsyncId, resource) { | ||
processing_hook = true; | ||
// Use a single try/catch for all hook to avoid setting up one per iteration. | ||
try { | ||
for (var i = 0; i < active_hooks_array.length; i++) { | ||
if (typeof active_hooks_array[i][init_symbol] === 'function') { | ||
active_hooks_array[i][init_symbol]( | ||
asyncId, type, triggerAsyncId, | ||
resource | ||
); | ||
} | ||
for (var i = 0; i < active_hooks_array.length; i++) { | ||
if (typeof active_hooks_array[i][init_symbol] === 'function') { | ||
active_hooks_array[i][init_symbol]( | ||
asyncId, type, triggerAsyncId, | ||
resource | ||
); | ||
} | ||
} catch (e) { | ||
fatalError(e); | ||
} | ||
processing_hook = false; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is everyone trying to kill this 😨
I like it as a way to bail from a callback or a
Promise.catch
Ref: #13839 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just because it is useful for
Promise.catch
doesn't mean that is useful here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably. I'll copy it to
test/common/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Letting an error bubble with
try {} finally {}
have always been the standard way of handling throws.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except what async_hooks is doing isn't standard. This is a guard to prevent an abort from C++ if it detects stack corruption that can likely result from the user try catching/Promise wrapping the call, and to get useful core dumps. It also doesn't follow how this is handled in C++ (https://github.com/nodejs/node/blob/v8.1.3/src/env-inl.h#L144-L160). I'm a big -1 on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By "exporting
FatalException
" do you mean expose it to JS?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If anything I'd like to export the code in
Environment::AsyncHooks::pop_ids()
and use that as the uniform way to exit the process.EDIT: Except it should show a JS stack instead of a C++ stack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. At the very least I would like a unified way of making a fatal exception. Currently, there are a couple of different implementations floating around in nodecore. It makes things like
--abort-on-uncaught-exception
difficult to test.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AndreasMadsen
Sounds good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So area this is currently a WIP?