Revert #17914

[joyeecheung]

See if this fixes #18190

Checklist

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

Affected core subsystem(s)

[joyeecheung]

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

[apapirovski]

A couple more runs just on Linux:
https://ci.nodejs.org/job/node-test-commit-linux/15669/
https://ci.nodejs.org/job/node-test-commit-linux/15671/

[jasnell]

That test is particularly sensitive. I'd prefer not to revert these commits. The test likely needs to be updated instead.

[apapirovski]

@jasnell I think @joyeecheung is just trying to trace down the cause, rather than submit an official PR to revert. Right now we're not even sure which commit is causing it as there's some contradictory evidence in past CI runs.

[jasnell]

Yep, I get that. There are several things that could cause this. In my experience, it has typically been the addition/removal of a new AsyncWrap Provider type, but there are several other reasons why this can fail in not so obvious ways due to the magic of how async_hooks are implemented. It can be difficult to track down.

[joyeecheung]

@apapirovski @jasnell Yeah I opened this because I had no idea how to run https://ci.nodejs.org/job/node-stress-single-test on shas so I kinda have to do the bisecting this way...

(Although I still have no idea how to run the flaky tests with PR refs, there are tons of EACCESS on common.PIPE..)

[apapirovski]

Well, 4 CI runs on Linux and this test hasn't failed a single time on alpine... it seems like the issue is somewhere in these commits and their interaction with that test. I still don't see any offending code tho... 🤔

Edit: it does seem like the Windows failure could be unrelated tho. So we potentially have two different bugs in that test.

[jasnell]

That particular bit fails when something keeps the event loop open... ping @addaleax who tracked this down previously. When we've seen this before, it was caused by very subtle differences in timing of garbage collection. In one recent case, a change in the node_perf.cc on how the gc callback was scheduled to run caused a bug that only became apparent because the completely unrelated http2 module happened to load just enough string constants that the gc was triggered, and it caused beforeExit to fire twice. Fun eh? @addaleax may be able to give some pointers on how to track this down.

[joyeecheung]

#17914 actually only touches sync APIs after a rebase against #18144 , and the only sync API used in that test is openSync which is not touched by #17914.... :/? So I think this probably has something to do with the timings/load, like the previous flake, not the APIs..

[apapirovski]

@jasnell yeah, I worked on that particular issue so it was the first thing that came to mind but I don't see any changes that are responsible for keeping the event loop open. I can't replicate locally either which makes it all the more difficult to debug.

On that note: is there a particular reason we're checking in beforeExit? Could that check be moved to exit?

Although... anything that keeps the event loop open after beforeExit fires but doesn't originate from the listener is concerning.

[jasnell]

Unfortunately it could be any unref'd handle, it doesn't have to be limited to this change. For instance, in the http2 PR that triggered this, there was zero code in the PR that was directly responsible. Hmm.. going to have to think about that a bit more.

Re: why beforeExit it being used in the test, I'm not sure. You'd have to ask @trevnorris

[apapirovski]

I think we're on the same page. I just find it concerning that we have another instance of some side-effect that brings the loop alive. The SetImmediate bit was definitely a bug. Even if this PR just happened to surface an unrelated bug, it's important to not just sweep it under the rug by reverting, working around it or changing to use an exit listener.

Will try to spend some time digging around to see what other code we have that can bring a loop alive as a side-effect.

[apapirovski]

@joyeecheung @addaleax @jasnell I believe I have a fix in #18241

[joyeecheung]

Closed now that #18241 landed