exit workers gracefully

[jeysal]

Summary

This PR makes child process workers avoid process.exit, instead gracefully exiting them by clearing open handles. If they fail to exit gracefully (usually because the user code leaks) after a delay, they are force exited using SIGTERM and if that doesn't help after another delay using SIGKILL. For Node worker threads, the force exit method is terminate().
The primary use for this (besides being cleaner) is that we can at least print a warning if tests leak open handles (which this PR also does) instead of effectively just cancelling the whole event loop by terminating the worker. Many users have complained about bugs like console.logs being lost and this is likely one of the reasons why stuff like that happens, hopefully exiting gracefully can reduce such hard-to-debug behavior a bit.
This is not a breaking change because right now jest-worker doesn't guarantee either that the worker processes have exited when end() returns (and why should the user care about some leftover worker processes, as long as they will eventually exit).

See also #7731 (comment)

• jest-runner: warn if a worker had to be force exited
• e2e test that has to be force exited
• NodeThreadsWorker (type errors)
• manually run e2e test with NodeThreadsWorker (not done on CI because worker threads cause many other errors)
• jest-worker/README.md
• changelog
• PleaseCanItJustWorkOnWindowsThanx - Azure says it does 🎉

Test plan

Updated and added some jest-worker tests, added an e2e test for the warning and to check that force exit works.
Also run the e2e test and a manual Jest execution with worker threads enabled in jest-runner and the Node flag.
On master, the e2e test "force exits a worker" passes (because master always force exits them using process.exit), but "prints a warning" fails.

[SimenB]

I like this 🙂

[jeysal]

The diff is getting quite large. NodeThreadsWorker still missing but I hope that's not gonna be too big.
Could extract the jest-runner / e2e part into another PR if anyone wants me to so it gets easier to review.
Side note: I feel like once CI gets past typecheck, this might die horribly on Windows. Let's hope not.

[jeysal]

Alright, this is ready now 👍I'll request reviews from a couple more people than usual since it's a "low level" change, although it's not very invasive.

[SimenB]

fantastic stuff!

[SimenB]

stderr instead? not sure

[jeysal]

Dunno, last time I added a warning was jest-circus/jest-jasmine2 which also used console.log(chalk.yellow()). You know the code base better than I do, what is more commonly used for warnings? 😄

[SimenB]

I'm not sure if this should be considered a breaking change or not (thus should only land in 25).

(I know I asked you to make it async, but changing the return type to be a promise, async fn or not, is a change in signature)

25 shouldn't be far out, thoughts on holding off?

[jeysal]

TBH I'm not sure about the change from throw to reject you asked me to make.
The argument was that it's annoying if a function can both throw and reject.
I think that this function should only ever throw, never reject. It has the case of invalid usage (end() called multiple times), but it should never "fail to end" the pool. If there's no other way, SIGKILL everything. It should always be able to clean up.
If we'd revert that back then it's definitely not breaking because the workers were always ended asynchronously, there was just no way to find out when that happened (and the signature itself is backward compatible).

[jeysal]

@SimenB would you be fine with changing this back? I think calling this with invalid preconditions should throw immediately and not worst case even result in an uncaught rejection

[jeysal]

Thanks for the reviews, I'll try to get to it asap :)

[jeysal]

Incorporated feedback. Open points I've commented on:

• stderr instead?
• reject vs throw

[jeysal]

@SimenB I think this should go into the (probably last) 24 minor. I don't want to overload the 25 major with large refactoring changes.
Even with your async rejection change, the only case where this would break someone that has been brought up is if you call end() twice and rely on the second call throwing an error to catch it and then ... do something? Idk, I think this is as non-breaking as "large" changes can get ^^

[unional]

I have encountered A worker process has failed to exit gracefully and has been force exited. because I do have a handle opened (a timeout handler in a testing framework).

Is there a way to tell jest or the worker to call certain function before exiting so that I can stop the timeout handler?

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.