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

Add Graceful Shutdown Hook #251

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

luke-rucker
Copy link

Since the worker traps shutdown signals to the process so it can gracefully handle in-progress jobs, this makes it impossible for consumers of the library to perform any kind of graceful shutdown of their own.

This PR adds the ability for a cleanup function to be passed to the worker to be called after in-progress jobs are completed.

Let me know what you think!

@jbielick
Copy link
Owner

This looks pretty good. I'm curious, would a worker.emit('stop') or something do the trick? Any reason we need to await the shutdown task?

@luke-rucker
Copy link
Author

My thought with awaiting the shutdown task was so that the task would respect the shutdownTimeout of the worker. Although, one could argue that this is mixing concerns and the shutdownTimeout should only affect in-progress jobs, not the graceful shutdown of the whole process.

I'm more than happy to modify the PR to whatever you think is best!

@luke-rucker
Copy link
Author

Bump! It would be great if we could get this merged or if you could provide more feedback. Would really like to use this feature at work.

@jbielick
Copy link
Owner

jbielick commented Feb 7, 2022

I think I prefer the event emitter approach. I agree that does preclude the ability to block the shutdown and be subject to the hard shutdown timeout. If this is critical, maybe we can await all of the stop listeners? e.g. await Promise.all(worker.listeners('exit').map(f => f())))

I think the appropriate event name for after the workers are stopped is exit

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