async_hooks: triggerAsyncId can be undefined

[mcollina]

Fixes: #14386
Fixes: #14381
Fixes: #14368

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

async_hooks

[mcollina]

@nodejs/async_hooks @AndreasMadsen please verify. I do not claim this is the right fix or what other consequences this fix could have, but it solves the problem here.

[mcollina]

Side note: the commit does not pass core-validate-commit as it does not recognize 'async-hooks' as a valid subsystem.

[mcollina]

This problem was likely introduced in #14026.

[AndreasMadsen]

I do not claim this is the right fix or what other consequences this fix could have, but it solves the problem here.

We need to find the cause for why triggerAsyncId is undefined. Likely it is supposed to be null under some special condition that we aren't testing for.

[mcollina]

@AndreasMadsen the way 'shot' works is by piggypacking on core, and it does not allocate a full socket. https://github.com/nodejs/node/blob/master/lib/_http_outgoing.js#L281 is expecting to have a real socket, but it has not in this case.

I can write that test and default the value of that symbol to null there if it's the case, but I think this might be the cause of a lot of other breakages. We are expecting something to be there and it is not. Forcing the community to track down crashes like that is not what I define as stable or safe to use. async_hooks are experimental, but this is breaking code that are not using them.

This change avoid all the crashes, at the cost of reporting bad data, why is it wrong?

[AndreasMadsen]

We are expecting something to be there and it is not. Forcing the community to track down crashes like that is not what I define as stable or safe to use. async_hooks are experimental, but this is breaking code that are not using them.

I agree. I suggested to @trevnorris that we skipped the entire assert in PushAsyncIds/PopAsyncIds when async_hooks isn't enabled. But he said that the assert prevented him from losing his sanity (#13548 (comment)).

I can write that test and default the value of that symbol to null there if it's the case, but I think this might be the cause of a lot of other breakages.

Sounds reasonable.

---

As far as I have seen the only two sources of error we have seen after v8.2.0 are from _writeRaw and _write in _http_outgoing. I think once we fix that, the number of issues should be reduced dramatically.

[mcollina]

I'll work on the other PR to null things in _http_outgoing.

[refack]

I belive !Number.isSafeInteger(triggerAsyncId ) || triggerAsyncId < 0 is the "preferred" test

[refack]

If that is actually a valid code path

[mscdex]

If not, double equals would be preferred to combine both checks.

[jasnell]

Could be simplified to just if (triggerAsyncId == null) {

[jasnell]

heh... sorry, just spotted @mscdex's identical comment ;-)

[mcollina]

@trevnorris what do you think of this?

[trevnorris]

Change the statement to if (!Number.isSafeInteger(triggerAsyncId) || triggerAsyncId <= 0). This will fix the current situation while also accommodating solution (2) from #14389 (comment).

[mcollina]

@trevnorris if you prefer this over #14389, I'll patch it right now.

[trevnorris]

@mcollina I prefer this, and am okay landing it any time. It's a change that my planned PR would need to make anyway.

@jasnell is this the only patch that we're looking at for the v8.2.1 release? if so then nm waiting on the release.

[jasnell]

@trevnorris ... I'm still trying to determine if this is the only one :-) need more coffee. If @mcollina is happy with it and it addresses the immediate need, then +1

[mcollina]

I'm happy. I'll add a test also for #14368, so we don't regress on that as well.

[mcollina]

@trevnorris@addaleax@jasnell please review.

I've included also a fix for: #14368.
It's a separate commit, so we can easily remove/revert if it is not wanted.

[mcollina]

CI: https://ci.nodejs.org/job/node-test-pull-request/9271/

[jasnell]

Why this and not assert.strictEqual(chunk.toString(), 'hello world')?

[mcollina]

the chunk contains a full HTTP response, which for brevity I did not want to include. common.mustCall it's also sufficient to validate the test on its own.

[jasnell]

nit...

http.get(`http://localhost:${server.address().port}`);

[mcollina]

done

[jasnell]

Couple of nits but LGTM

[mcollina]

CI: https://ci.nodejs.org/job/node-test-pull-request/9273/

[mcollina]

CI https://ci.nodejs.org/job/node-test-pull-request/9274/ (again)
CITGM https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/918/

[trevnorris]

thanks for the changes

[Fishrock123]

Thanks! landed in 107db33 and 69fdb47

[Fishrock123]

Uh oh, these work just fine on master but cause this error on v8.x-staging:

=== release test-tlswrap ===
Path: async-hooks/test-tlswrap
assert.js:60
throw new errors.AssertionError({
^
AssertionError [ERR_ASSERTION]: Checking invocations at stage "client: when client destroyed":
Called "before" 2 time(s), but expected 3 invocation(s).
at checkHook (/Users/Jeremiah/Documents/node/test/async-hooks/hook-checks.js:51:14)
at Array.forEach (native)
at checkInvocations (/Users/Jeremiah/Documents/node/test/async-hooks/hook-checks.js:28:44)
at tick1 (/Users/Jeremiah/Documents/node/test/async-hooks/test-tlswrap.js:93:5)
at Immediate.ontick (/Users/Jeremiah/Documents/node/test/async-hooks/tick.js:7:37)
at runCallback (timers.js:781:20)
at tryOnImmediate (timers.js:743:5)
at processImmediate [as _immediateCallback] (timers.js:714:5)
Command: out/Release/node /Users/Jeremiah/Documents/node/test/async-hooks/test-tlswrap.js

[Fishrock123]

Hmmmm, that might be flakey. I can't repo it now.

[refack]

Stress on linuxONE: https://ci.nodejs.org/job/node-stress-single-test/1336/nodes=rhel72-s390x/

[Trott]

I stress tested against master locally. It's flaky:

$ tools/test.py -j92 --repeat 920 test/async-hooks/test-tlswrap.js === release test-tlswrap === Path: async-hooks/test-tlswrapassert.js:43 throw new errors.AssertionError({ ^AssertionError [ERR_ASSERTION]: Checking invocations at stage "client: when client destroyed": Called "before" 2 time(s), but expected 3 invocation(s). at checkHook (/Users/trott/io.js/test/async-hooks/hook-checks.js:51:14) at Array.forEach (<anonymous>) at checkInvocations (/Users/trott/io.js/test/async-hooks/hook-checks.js:28:44) at tick1 (/Users/trott/io.js/test/async-hooks/test-tlswrap.js:93:5) at Immediate.ontick (/Users/trott/io.js/test/async-hooks/tick.js:7:37) at runCallback (timers.js:781:20) at tryOnImmediate (timers.js:743:5) at processImmediate [as _immediateCallback] (timers.js:714:5)Command: out/Release/node /Users/trott/io.js/test/async-hooks/test-tlswrap.js

Probably race condition. The comment around before line 93 (where the test fails) kind of suggests that too:

// TODO: why is client not destroyed here even after 5 ticks?// or could it be that it isn't actually destroyed until// the server is closed?