async_wrap: schedule destroy hook as unref

[apapirovski]

Since the DestroyAsyncIdsCallback in Node.js can be scheduled as a result of GC, that means that it can accidentally keep the event loop open when it shouldn't. Replace SetImmediate with the newly introduced SetUnrefImmediate and in addition introduce RunBeforeExit callbacks, of which DestroyAsyncIdsCallback is now the first. These callbacks will run before the beforeExit event is emitted (which will now only be emitted if the event loop is still not active).

Fixes: #18190

Linux CI to assess whether this fixes the above (all green):
https://ci.nodejs.org/job/node-test-commit-linux/15673/
https://ci.nodejs.org/job/node-test-commit-linux/15674/
https://ci.nodejs.org/job/node-test-commit-linux/15675/

CI:
https://ci.nodejs.org/job/node-test-pull-request/12615/
https://ci.nodejs.org/job/node-test-commit/15516/
https://ci.nodejs.org/job/node-test-commit/15517/

CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1212/

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_wrap

[jasnell]

LGTM if this solves the flakiness in the test.

...this does introduce a change in the contract for async hooks... specifically, it means that there is chance that destroy may not be called for at least some set of hooks. It's likely best to document that fact if we can do so in a way that isn't too obscure.

[apapirovski]

...this does introduce a change in the contract for async hooks... specifically, it means that there is chance that destroy may not be called for at least some set of hooks. It's likely best to document that fact if we can do so in a way that isn't too obscure.

The newly introduced RunBeforeExit ensures that the async hooks behaviour remains the same. We already don't guarantee that GC-triggered destroy hooks will be called as the GC is unpredictable and can just as easily occur after the loop is done completely as it can right after beforeExit. This just makes sure that the behaviour is more deterministic as opposed to allowing for race conditions.

[jasnell]

Ah right yeah... Forgot about the RunBeforeExit. All good then

[AndreasMadsen]

LGTM

[Trott]

Is there a chance at all that this might fix v8#45 as well? /cc @fhinkel

[apapirovski]

Pretty sure this fixes the flakes. I ran 6 Linux CIs and 3 full ones, not a single failure of that getasyncid test.

[joyeecheung]

Stress tests after #18245 lands:

master:
RUN_TIMES=10, --repeat 100: https://ci.nodejs.org/job/node-stress-single-test/1733
RUN_TIMES=100, --repeat 1000: https://ci.nodejs.org/job/node-stress-single-test/1734

refs/pull/18241/head (this PR):
RUN_TIMES=10, --repeat 100: https://ci.nodejs.org/job/node-stress-single-test/1736
RUN_TIMES=100, --repeat 1000: https://ci.nodejs.org/job/node-stress-single-test/1735

refs/pull/18245/head
RUN_TIMES=100 and --repeat 1000 : https://ci.nodejs.org/job/node-stress-single-test/1723
(100 OK: 23 NOT OK: 77 TOTAL: 100, took 8hrs)

[apapirovski]

Looks I need to rebase as that job doesn't do it. On it.

Stress test CI: https://ci.nodejs.org/job/node-stress-single-test/1737/

[joyeecheung]

Stress test is green. Should we land this?

[apapirovski]

Landed in 8803b69

[MylesBorins]

Node 9.x is failing to compile with this commit, should we backport?

de/node/v9.x/out/Release/obj.target/node_lib/src/handle_wrap.o.d.raw   -c -o /Users/mborins/code/node/v9.x/out/Release/obj.target/node_lib/src/handle_wrap.o ../src/handle_wrap.cc
../src/async_wrap.cc:674:10: error: no member named 'SetUnrefImmediate' in 'node::Environment'
    env->SetUnrefImmediate(DestroyAsyncIdsCallback, nullptr);
    ~~~  ^
1 error generated.