async_wrap: remove erroneous destroy list clear()#13353
async_wrap: remove erroneous destroy list clear()#13353addaleax wants to merge 1 commit intonodejs:masterfrom
Conversation
nodejs-github-bot
added
async_wrap
c++
There was a problem hiding this comment.
😀 that's a good use for mustCallAtLeast I feel validated.
There was a problem hiding this comment.
@refack Yeah, I kind of like it. 👍 You might want to re-review though, I’ve updated the test file quite a bit after Trevor’s comments in #13286 (comment)
addaleax
force-pushed
the
async-hooks-13286-a
branch
from
3c9015b to
1e90b3f
Compare
Remove a `.clear()` call on the list of destroy ids that may inadvertently swallow `destroy` events. The list is already cleared earlier in the `DestroyIdsCb` function, so usually this was a no-op; but when the garbage collection or its equivalent was active during a `destroy` hook itself, it was possible that `destroy` hooks were scheduled but cleared before the next event loop iteration in which they would have been emitted. Ref: nodejs#13286
addaleax
force-pushed
the
async-hooks-13286-a
branch
from
1e90b3f to
a052d4a
Compare
| }).enable(); | ||
|
|
||
| const res1 = new async_hooks.AsyncResource('foobar'); | ||
| res2 = new async_hooks.AsyncResource('foobar'); |
There was a problem hiding this comment.
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.
Hm? This test is independent of GC. res2 isn’t const because it needs to be accessed from inside the hook, yes.
| res2 = new async_hooks.AsyncResource('foobar'); | ||
| res1.emitDestroy(); | ||
|
|
||
| process.on('exit', () => assert.strictEqual(destroyResCallCount, 2)); |
There was a problem hiding this comment.
You can instead do:
const sentinel = common.mustCall(() =>{}, 2);
and call sentinel() after the if in line 20
There was a problem hiding this comment.
I know, I don’t think that would be clearer here though.
|
I think we’re superseding this by #13369, so I’m closing. Thanks for the review, though. 😄 |
5 participants
Breaking this out from #13286, Trevor said he had a better fix for the other problem and I believe him. :) @nodejs/async_hooks
Remove a
.clear()call on the list of destroy ids that mayinadvertently swallow
destroyevents.The list is already cleared earlier in the
DestroyIdsCbfunction,so usually this was a no-op; but when the garbage collection or
its equivalent was active during a
destroyhook itself, it waspossible that
destroyhooks were scheduled but cleared before thenext event loop iteration in which they would have been emitted.
Ref: #13286
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
async_wrap