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

fix: handle worker termination after worker exit #14

Merged
merged 1 commit into from
Jul 28, 2020
Merged

fix: handle worker termination after worker exit #14

merged 1 commit into from
Jul 28, 2020

Conversation

r00b
Copy link
Contributor

@r00b r00b commented Jul 28, 2020

Setting closeWorkerAfterMs causes for the worker to be terminated after the specified time. However, if the worker completes and exits normally before closeWorkerAfterMs milliseconds, the timeout created upon worker init is not cleared. This causes a TypeError where this.workers[name] is undefined and thus this.workers[name].terminate cannot be read or called. This PR adds a check upon timeout to ensure that this.workers[name] exists before calling terminate. This PR also fixes the test case run > job terminates after set time, which was not actually a proper test since the Promise in the test case was not awaited. The test failed to ensure that a worker that was properly terminated when termination was disabled.

I realized during the fix that there is a generic stop function that performs several cleanup tasks, one of which is clearing the timout set by closeWorkerAfterMs. I wasn't sure if it was appropriate to call stop during the exit stage of the worker, since the worker would technically already be stopped, so I leave it up to the maintainers to advise.

@niftylettuce niftylettuce merged commit b9202a2 into breejs:master Jul 28, 2020
@r00b r00b deleted the b/exit-before-set-time branch July 28, 2020 23:14
@niftylettuce
Copy link
Contributor

Excellent catch, thank you @robertsteilberg, appreciate the PR as well - it truly helps! 👏 🙇

Published v1.1.21 to npm and published deprecation notice for all versions prior.

https://github.com/breejs/bree/releases/tag/v1.1.21

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants