Uh oh!
There was an error while loading. Please reload this page.
async_hooks: fix default nextTick triggerAsyncId - #14026
Conversation
In the case where triggerAsyncId is null it should default to the current executionAsyncId. This worked but as a side-effect the resource object was changed too. This fix also makes the null check more strict. EmitInitS is not a documented API, thus there is no reason to be flexible in its input.
AndreasMadsen
commented
Jul 1, 2017
I think we should implement this for now. But long term I'm wondering if we should eliminate the polymorphism in |
refack
left a comment
There was a problem hiding this comment.
Makes sense.
Left just some silly nits (ooof I hate to be the nit guy)
| // later | ||
| if (this._allowNoInit) { | ||
| const stub = { uid, type: 'Unknown' }; | ||
| const stub = { uid, type: 'Unknown', handleIsObject: true }; |
There was a problem hiding this comment.
Nit: maybe just isObject? (here and everywhere, obviusly)
There was a problem hiding this comment.
I would much rather be explicit, so many things could be an object.
| @@ -147,7 +152,7 @@ class ActivityCollector { | |||
| // events this makes sense for a few tests in which we enable some hooks | |||
There was a problem hiding this comment.
nit: If you're already here, could you turn this comment to a proper paragraph (capitalization, some punctuation, etc)
| } | ||
| const asyncId = ++async_uid_fields[kAsyncUidCntr]; | ||
| if (triggerAsyncId === null) { |
There was a problem hiding this comment.
Nit: Since it's argument overload handling, could you move it up, probably just after the if (process._exiting) return
| uid, | ||
| type, | ||
| triggerAsyncId, | ||
| // in some cases (Timeout) the handle is a function, thus the usual |
There was a problem hiding this comment.
Nit: (I don't really mind but) start the sentence with a capital I. Also (e.g. `Timeout`)
| @@ -0,0 +1,37 @@ | |||
| 'use strict'; | |||
There was a problem hiding this comment.
Nit: style as in https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure (and but the //Flags on the first line 🤷♂️
CI: https://ci.nodejs.org/job/node-test-pull-request/8948/ |
AndreasMadsen
commented
Jul 5, 2017
landed in 0fd4c73 |
In the case where triggerAsyncId is null it should default to the current executionAsyncId. This worked but as a side-effect the resource object was changed too. This fix also makes the null check more strict. EmitInitS is not a documented API, thus there is no reason to be flexible in its input. Ref: #13548 (comment) PR-URL: #14026 Reviewed-By: Refael Ackermann <refack@gmail.com>
In the case where triggerAsyncId is null it should default to the current executionAsyncId. This worked but as a side-effect the resource object was changed too. This fix also makes the null check more strict. EmitInitS is not a documented API, thus there is no reason to be flexible in its input. Ref: #13548 (comment) PR-URL: #14026 Reviewed-By: Refael Ackermann <refack@gmail.com>
In the case where triggerAsyncId is null it should default to the current executionAsyncId. This worked but as a side-effect the resource object was changed too. This fix also makes the null check more strict. EmitInitS is not a documented API, thus there is no reason to be flexible in its input. Ref: #13548 (comment) PR-URL: #14026 Reviewed-By: Refael Ackermann <refack@gmail.com>
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
async_hooks
In the case where
triggerAsyncIdisnullit should default to thecurrent executionAsyncId. This worked but as a side-effect the resource
object was changed too.
This fix also makes the null check more strict. EmitInitS is not a
documented API, thus there is no reason to be flexible in its input.
/cc @nodejs/async_hooks