Unref shared worker when all test workers have deregistered

[codetheweb]

The symptom of this bug was that when shared workers are used and AVA's global timeout is triggered, ava exits with 0 instead of 1 (we make heavy use of shared workers and didn't realize until recently that our test workflow in CI is frequently marked as passing when a test times out). Similar symptom to #3098, however I think the underlying issue is quite different.

Added a test that reproduces the error and a tentative fix.

Potentially related: #3098

[novemberborn]

The changes look good, but it's unclear what the root cause of the bug is. Is AVA exiting earlier than we can set the exit code?

[codetheweb]

I still don't completely understand the internals of AVA, so some of this might be wrong but this is what I was experiencing when running the test reproduction:

1. The main process was stalling on

   ava/lib/api.js

   Line 308 in 4ecfe7d

   await Promise.all(deregisteredSharedWorkers);

2. The Promise array wasn't resolving because the de-registration Promise here:

   ava/lib/plugin-support/shared-workers.js

   Line 54 in 4ecfe7d

   const deregistered = new Promise(resolve => {

   never resolved.
3. This Promise never resolved because the main process sent {type: "deregister-test-worker"} to the shared worker loader, but the shared worker loader never sent back {type: "deregistered-test-worker"} in response. I believe this was because the shared worker loader was being un-referenced too early and getting unloaded.
4. Because the main process didn't get confirmation that the shared worker was deregistered from the test worker, registrationCount remained at 1 and the de-registration Promise never resolved.

My patch fixes the reproduction and doesn't break any other tests, but I'm not completely convinced it's the correct solution. Shouldn't the main process exit with 1 if it times out for any reason (including getting stuck at

ava/lib/api.js

Line 308 in 4ecfe7d

await Promise.all(deregisteredSharedWorkers);

?)

[novemberborn]

The exit code is only set when the run completes:

ava/lib/cli.js

Line 496 in 4ecfe7d

process.exitCode = runStatus.suggestExitCode({matching: match.length > 0});

But Node.js will exit even when you're awaiting a pending promise, if there's nothing left in the event loop that would let that promise be resolved.

Therefore we need to make sure Node.js doesn't exit on its own, otherwise we'll never set the exit code.

https://nodejs.org/api/worker_threads.html#workerunref states that the worker thread will exit when the event loop in the main thread is empty. Delaying the unref() call would therefore keep the main thread alive.

Shared workers may need to do cleanup, so we want them to (be able to) perform tasks when a test worker "goes away". That's what we're awaiting at the end of the run. (But, note that we're counting registrations of possibly different shared workers per test file.)

So the question is, why did the event loop become empty when we were still (potentially) performing cleanup? And I don't know, but it seems to be that this could be a race condition when waiting for the various events from the various worker threads.

Long story short, I think this is the correct fix 😄

[novemberborn]

I'll try and release this today or tomorrow.

[novemberborn]

I'll try and release this today or tomorrow.

[codetheweb]

Just a heads up that this seems to cause another bug in our main repo. Still investigating and I haven't been able to reproduce in AVA yet, but might want to hold off on releasing this for a few days--sorry!