async_hooks: cleanup emitBefore and emitAfter in timers and nextTick #14050
Conversation
nodejs-github-bot
added
async_hooks
|
CI: https://ci.nodejs.org/job/node-test-pull-request/8962/ |
lib/async_hooks.js
Outdated
| if (asyncId < 0 || triggerAsyncId < 0) { | ||
| fatalError('before(): asyncId or triggerAsyncId is less than zero ' + | ||
| `(asyncId: ${asyncId}, triggerAsyncId: ${triggerAsyncId})`); | ||
| } |
There was a problem hiding this comment.
This is to throw nicely. If the values make it to AsyncWrap::PushAsyncIds() then it will abort (i.e. always core dump).
There was a problem hiding this comment.
Yes. But does this matter. asyncId < 0 and triggerAsyncId < 0 is for sure not a common error. We can keep the check, but then the performance of nextTick and the timers won't be exactly the same.
There was a problem hiding this comment.
emitBeforeS is a (undocumented) user API and is exported via require(). The general rule has been that it shouldn't be possible for the application to abort via any method imported via require() (this does not hold true for process.binding()). But removing this allows require('async_hooks').emitBefore(-1, -1) to cause a CHECK() failure.
There was a problem hiding this comment.
Okay, that makes sense. I will readd the checks later today.
| emitBefore(asyncId, triggerAsyncId); | ||
| else | ||
| pushAsyncIds(asyncId, triggerAsyncId); | ||
| } |
There was a problem hiding this comment.
History lesson: This was introduced originally because of a crazy method to keep track of the id stack. Which was a lot faster than it is currently, but added an obscene amount of complexity. I hope to simplify that approach in the future and reintroduce it, but seems I overlooked these calls can be removed for the time being.
| emitBefore(asyncId, triggerAsyncId); | ||
| else | ||
| pushAsyncIds(asyncId, triggerAsyncId); | ||
| } |
There was a problem hiding this comment.
Reason these were here is for the same reason explained in nextTick.
Timers and nextTick have special emitBefore and emitAfter functions for historic reasons. These function are not needed any more, thus the public emitBefore and emitAfter function can be used.
Using asyncId as the default triggerAsyncId is wrong. The triggerAsyncId can actually never be the asyncId.
|
I reverted the commits that removed the assert and squashed the commits so it isn't confusing. I also rewrote the commit message. |
Timers and nextTick have special emitBefore and emitAfter functions for historic reasons. These function are not needed any more, thus the public emitBefore and emitAfter function can be used. PR-URL: #14050 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Using asyncId as the default triggerAsyncId is wrong. The triggerAsyncId can actually never be the asyncId. PR-URL: #14050 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Timers and nextTick have special emitBefore and emitAfter functions for historic reasons. These function are not needed any more, thus the public emitBefore and emitAfter function can be used. PR-URL: #14050 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Using asyncId as the default triggerAsyncId is wrong. The triggerAsyncId can actually never be the asyncId. PR-URL: #14050 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Timers and nextTick have special emitBefore and emitAfter functions for historic reasons. These function are not needed any more, thus the public emitBefore and emitAfter function can be used. PR-URL: #14050 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Using asyncId as the default triggerAsyncId is wrong. The triggerAsyncId can actually never be the asyncId. PR-URL: #14050 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
async_hooks, process, timers
async_hooks: use common emitBefore and emitAfter
nextTickhave specialemitBeforeandemitAfterfunctions forperformance reasons. But if the assert in
async_hooks.emitBeforeisremoved there is no difference.
edit: the checks are already included in
pushAsyncIds.async_hooks: require parameter in emitBefore
asyncIdas the defaulttriggerAsyncIdis wrong. ThetriggerAsyncIdcan actually never be the
asyncId./cc @nodejs/async_hooks