Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

exit workers gracefully #8206

Merged
merged 15 commits into from
Aug 23, 2019
Merged

exit workers gracefully #8206

merged 15 commits into from
Aug 23, 2019

Conversation

jeysal
Copy link
Contributor

@jeysal jeysal commented Mar 24, 2019

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.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this 🙂

packages/jest-worker/src/base/BaseWorkerPool.ts Outdated Show resolved Hide resolved
packages/jest-worker/src/index.ts Show resolved Hide resolved
@jeysal jeysal force-pushed the worker-graceful-exit branch 2 times, most recently from 00011e2 to 04b13a7 Compare March 25, 2019 22:40
@jeysal jeysal closed this Mar 25, 2019
@jeysal jeysal reopened this Mar 25, 2019
@jeysal
Copy link
Contributor Author

jeysal commented Mar 25, 2019

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 jeysal changed the title WIP exit workers gracefully exit workers gracefully Mar 30, 2019
@jeysal
Copy link
Contributor Author

jeysal commented Mar 30, 2019

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.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fantastic stuff!

CHANGELOG.md Outdated Show resolved Hide resolved
e2e/Utils.ts Outdated Show resolved Hide resolved
e2e/__tests__/workerForceExit.test.ts Show resolved Hide resolved
const cleanup = async () => {
const {forceExited} = await worker.end();
if (forceExited) {
console.log(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stderr instead? not sure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 😄

packages/jest-worker/src/__tests__/index.test.js Outdated Show resolved Hide resolved
@@ -85,15 +90,33 @@ export default class BaseWorkerPool {
throw Error('Missing method createWorker in WorkerPool');
}

end(): void {
async end(): Promise<PoolExitResult> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

packages/jest-worker/src/index.ts Outdated Show resolved Hide resolved
@jeysal
Copy link
Contributor Author

jeysal commented Apr 4, 2019

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

@jeysal
Copy link
Contributor Author

jeysal commented Apr 4, 2019

Incorporated feedback. Open points I've commented on:

  • stderr instead?
  • reject vs throw

@jeysal
Copy link
Contributor Author

jeysal commented Apr 14, 2019

@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
Copy link
Contributor

unional commented Mar 10, 2020

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
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants