-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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_wrap: remove erroneous destroy list clear() #13353
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,42 @@ | ||
'use strict'; | ||
// Test that async ids that are added to the destroy queue while running a | ||
// `destroy` callback are handled correctly. | ||
|
||
const common = require('../common'); | ||
const assert = require('assert'); | ||
const async_hooks = require('async_hooks'); | ||
|
||
const initCalls = new Set(); | ||
let destroyResCallCount = 0; | ||
let res2; | ||
|
||
async_hooks.createHook({ | ||
init: common.mustCallAtLeast((id, provider, triggerId) => { | ||
if (provider === 'foobar') | ||
initCalls.add(id); | ||
}, 2), | ||
destroy: common.mustCallAtLeast((id) => { | ||
if (!initCalls.has(id)) return; | ||
|
||
switch (destroyResCallCount++) { | ||
case 0: | ||
// Trigger the second `destroy` call. | ||
res2.emitDestroy(); | ||
break; | ||
case 2: | ||
assert.fail('More than 2 destroy() invocations'); | ||
break; | ||
} | ||
}, 2) | ||
}).enable(); | ||
|
||
const res1 = new async_hooks.AsyncResource('foobar'); | ||
res2 = new async_hooks.AsyncResource('foobar'); | ||
res1.emitDestroy(); | ||
|
||
process.on('exit', () => assert.strictEqual(destroyResCallCount, 2)); | ||
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. You can instead do: 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 know, I don’t think that would be clearer here though. |
||
|
||
// Keep the event loop alive for a small bit so that the `destroy` callback | ||
// can run. This can be removed once https://github.com/nodejs/node/issues/13262 | ||
// is resolved. | ||
setTimeout(() => {}, 100); |
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.
I assume this is just for scoping, but it made me think is there any difference in
const
/let
vars, I donno GC related or something?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.
Hm? This test is independent of GC.
res2
isn’t const because it needs to be accessed from inside the hook, yes.