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

Wait for event loop to drain before exiting workers? #1718

Closed
novemberborn opened this issue Feb 17, 2018 · 7 comments · Fixed by #3260
Closed

Wait for event loop to drain before exiting workers? #1718

novemberborn opened this issue Feb 17, 2018 · 7 comments · Fixed by #3260
Labels
breaking requires a SemVer major release scope:internals scope:scheduling

Comments

@novemberborn
Copy link
Member

I'm considering changing the workers to not automatically exit once the last hook has finished. Ordinarily the event loop will drain and the process will exit on its own. However if some code is still running it could be part of a test. It may even call an assertion that should really cause the test to fail (see #1330).

Changing this behavior will encourage code that cleans up after itself. However I think we'd need an option to revert to the original behavior.

The --timeout option will apply to this to clean up workers that aren't exiting on their own. We should report the offending test files.

Interrupting AVA should do the same (see #583).

Thoughts?

@vadimdemedes
Copy link
Contributor

Could AVA only output some error to tell a user that a certain test suite was still running instead of waiting for all the I/O to finish? If all the tests completed, but something is still running, that test suite should be considered failing right away.

@sindresorhus
Copy link
Member

I'm fine with this as long as we display what tests are blocking AVA from finishing on its own.

@novemberborn
Copy link
Member Author

Could AVA only output some error to tell a user that a certain test suite was still running instead of waiting for all the I/O to finish? If all the tests completed, but something is still running, that test suite should be considered failing right away.

@vadimdemedes I like that, but we'd still need to give the test files a little bit of time to exit on their own. And then we may have to let users configure that…

I'm fine with this as long as we display what tests are blocking AVA from finishing on its own.

@sindresorhus it'd be test files, but yes we should show this.

I think if we land #583 we could set a default timeout (perhaps 10 seconds?). It'd automatically apply to this use case since eventually the worker pool gets filled up with workers that won't exit, and then the timeout is triggered. This feels to me like the cleanest approach.

@sindresorhus
Copy link
Member

sindresorhus commented Mar 4, 2018

And then we may have to let users configure that…

Configure the time, or whether to exit right away or wait to drain?

@novemberborn
Copy link
Member Author

And then we may have to let users configure that…

Configure the time, or whether to exit right away or wait to drain?

We need an option so users can make AVA exit right away. Or an API so they can do it from a test.after.always() hook but I'd rather make it an option.

But the wait-to-drain period also needs to be customizable. There may be a complex shutdown sequence or a remote database teardown that takes seconds. I don't really want an option for that, hence my suggestion of using a default (but long) timeout value and applying that to the entire test run.

@sindresorhus
Copy link
Member

sindresorhus commented May 30, 2018

For inspiration, here's how Jest solved it:

screen shot 2018-05-31 at 01 33 29

https://facebook.github.io/jest/docs/en/cli.html#detectopenhandles

I think we should do something similar.

@novemberborn novemberborn removed this from the 1.0 milestone Aug 11, 2018
@cancerberoSgx
Copy link

I'm having an issue in another scenario but 99% sure is caused by this - and which in jest is solved by --detectOpenHandles. I'm running ava tests with a customized node.js distribution that substitute the default event loop with a custom one, concretly https://github.com/nodegui/qode which uses Qt event loop.

Do you know how to workaround this problem? - tests run - test.after() callback run, but the node.js process won't exit. If I run the same code without ava then it does. any tip is most appreciated - if you want I could open a new issue to track this but I'm sure is the same cause.

@novemberborn novemberborn added this to the v3 milestone Dec 21, 2019
@novemberborn novemberborn removed this from the v3 milestone Jan 2, 2020
novemberborn added a commit that referenced this issue Nov 12, 2023
Fixes #1718.

With worker threads, this seems to stop AVA from detecting that the thread has exited, causing a hang.

Also remove flush logic implemented in #1722. Let's hope that current Node.js versions are better at flushing IPC before exiting.
novemberborn added a commit that referenced this issue Dec 4, 2023
Fixes #1718.

With worker threads, this seems to stop AVA from detecting that the thread has exited, causing a hang.

Also remove flush logic implemented in #1722. Let's hope that current Node.js versions are better at flushing IPC before exiting.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking requires a SemVer major release scope:internals scope:scheduling
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants