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

Shared worker teardown #2734

Open
novemberborn opened this issue Apr 6, 2021 · 2 comments
Open

Shared worker teardown #2734

novemberborn opened this issue Apr 6, 2021 · 2 comments
Labels
bug current functionality does not work as desired enhancement new functionality help wanted scope:shared-workers

Comments

@novemberborn
Copy link
Member

Based on this discussion it'd be great to have a teardown method within the shared worker. E.g. after negotiating the protocol, you can call .teardown() to register a callback.

Under normal use, after a test run, AVA can explicitly message the shared workers to begin their teardown. They must then respond when complete. The worker may then exit on its own or be terminated explicitly.

In watch mode, AVA should keep the workers alive until it's made to exit. It should then tear down the workers gracefully, as per above.

We should no longer unreference the worker:

launched.worker.unref();

Looking at this now, the whole deregistration bit here makes no sense:

let registrationCount = 0;
let signalDeregistered;
const deregistered = new Promise(resolve => {
signalDeregistered = resolve;
});

The shared worker may end up in a "deregistered" state if ever there is a gap between test workers using it.

The API instance should probably track shared worker instances, not in a per-run variable like now, but on the instance itself. This will require some rewiring:

ava/lib/api.js

Line 236 in bdf2cf0

deregisteredSharedWorkers.push(sharedWorkers.observeWorkerProcess(worker, runStatus));

Then, perhaps, we could add a teardown() method to the API itself, which we can call after its run completes:

ava/lib/cli.js

Line 467 in bdf2cf0

const runStatus = await api.run({filter});

Watch mode requires the process to be "killed" using a SIGINT signal. This is currently handled internally be the API instance:

ava/lib/api.js

Line 55 in d742672

process.on('SIGINT', () => this._interruptHandler());

And then per run:

ava/lib/api.js

Line 96 in d742672

this._interruptHandler = () => {

It might make more sense to register for SIGINT in the lib/cli.js file and then call an interrupt() method on the API instance, after which we can kick off the teardown.

When SIGINT is received a second time we should forcibly exit the process, without waiting for a graceful teardown of the shared workers.

@novemberborn novemberborn added bug current functionality does not work as desired enhancement new functionality help wanted scope:shared-workers labels Apr 6, 2021
@novemberborn
Copy link
Member Author

@timdp so I had a look, and this is quite involved. Let me know if you're still keen, or perhaps I should do some refactoring first so just the teardown work itself remains?

@timdp
Copy link

timdp commented Apr 6, 2021

I'm of course eager to have the teardown behavior but it's not my decision. 😉 Like I said earlier, happy to contribute if at all possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug current functionality does not work as desired enhancement new functionality help wanted scope:shared-workers
Projects
None yet
Development

No branches or pull requests

2 participants