-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: emitAfter correctly on fatalException #14914
Changes from all commits
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 |
---|---|---|
@@ -0,0 +1,40 @@ | ||
'use strict'; | ||
|
||
const common = require('../common'); | ||
const assert = require('assert'); | ||
const async_hooks = require('async_hooks'); | ||
|
||
const id_obj = {}; | ||
let collect = true; | ||
|
||
const hook = async_hooks.createHook({ | ||
before(id) { if (collect) id_obj[id] = true; }, | ||
after(id) { delete id_obj[id]; }, | ||
}).enable(); | ||
|
||
process.once('uncaughtException', common.mustCall((er) => { | ||
assert.strictEqual(er.message, 'bye'); | ||
collect = false; | ||
})); | ||
|
||
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. what happens is you throw here (where AFAICT the stack is still empty)? 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. The first pass of the script is the "bootstrap", which always has an id of |
||
setImmediate(common.mustCall(() => { | ||
process.nextTick(common.mustCall(() => { | ||
assert.strictEqual(Object.keys(id_obj).length, 0); | ||
hook.disable(); | ||
})); | ||
|
||
// Create a stack of async ids that will need to be emitted in the case of | ||
// an uncaught exception. | ||
const ar1 = new async_hooks.AsyncResource('Mine'); | ||
ar1.emitBefore(); | ||
|
||
const ar2 = new async_hooks.AsyncResource('Mine'); | ||
ar2.emitBefore(); | ||
|
||
throw new Error('bye'); | ||
|
||
// TODO(trevnorris): This test shows that the after() hooks are always called | ||
// correctly, but it doesn't solve where the emitDestroy() is missed because | ||
// of the uncaught exception. Simple solution is to always call emitDestroy() | ||
// before the emitAfter(), but how to codify 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.
Few questions:
emitAfter
?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.
asyncIdStackSize() > 0
at the first iteration?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.
because it fixes the bug :) and it's actually the other way around. previously I was overriding implementation details in
emitAfter()
.emitAfter()
pops the id. that's why it was skipping every other id in the stack.I'm doing it for the same reason as the
require('timers')
call below. Because if the script fails early in the startup process then the module might not have been loaded yet. Andrequire()
already caches the script into an object, doesn't it?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.
Ohh
emitAfter
isemitAfterScript
notemitAfterNative
Ack, but if you do it just before the loop it (1) looks cleaner [to me] (2) still lazy enough.
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.
do you mean creating
let async_hooks = null;
insetupProcessFatal()
then doing something like?:Or just
const async_hooks = NativeModule.require('async_hooks');
before the loop every time?