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: only drain workers on graceful shutdown. #1405

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

Conversation

AlliBalliBaba
Copy link
Collaborator

@AlliBalliBaba AlliBalliBaba commented Feb 22, 2025

Should fix #1296

This PR changes the graceful shutdown to just drain all running worker scripts. It's faster and easier to just let the OS clean up all the remaining memory.

@AlliBalliBaba AlliBalliBaba changed the title Only drains workers on shutdown. fix: only drain workers on graceful shutdown. Feb 22, 2025
@AlliBalliBaba
Copy link
Collaborator Author

Graceful shutdown was originally introduces in #1242 to fix #1226

func drainWorkers() {
watcher.DrainWatcher()
// DrainWorkers finishes all worker scripts before a graceful shutdown
func DrainWorkers() {
Copy link
Owner

Choose a reason for hiding this comment

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

Is it necessary to make this function exported?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The alternative would be to pass the 'soft shutdown' as a shutdown option if you prefer that.

frankenphp.Shutdown(frankenphp.WithSoftShutdown())

or something like that.

Copy link
Owner

Choose a reason for hiding this comment

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

Hum not really, we'll also need a new symbol :D

Maybe could we make this function experimental at least?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm i guess you're right that would just be even more complicated 😅 . I'll mark the endpoint as 'experimental'

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.

Shutting down indefinitely when http extension is loaded
3 participants