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 - graceful shutdown #1242

Merged
merged 4 commits into from
Dec 17, 2024
Merged

fix - graceful shutdown #1242

merged 4 commits into from
Dec 17, 2024

Conversation

Alliballibaba2
Copy link
Collaborator

This PR fixes #1226 by calling frankenphp.Shutdown() on Caddy's Shutdown().

I also went ahead and replaced the CaddyUsagePool by a global boolean. It's probably simpler to just have Shutdown() do nothing if FrankenPHP is not running (unless I'm missing something).

@AlliBalliBaba AlliBalliBaba changed the title Shuts caddy down gracefully. fix - graceful shutdown Dec 12, 2024
@AlliBalliBaba
Copy link
Collaborator

As a sidenote: I'm still not sure why calling frankenphp.Shutdown() inside Stop() will make Caddy's integration tests fail, but will work normally outside of tests. It seems like caddytest will call Stop() too early, preventing requests from being properly finished.


// attempt a graceful shutdown if we are not running tests
if flag.Lookup("test.v") == nil {
frankenphp.Shutdown()
Copy link
Owner

Choose a reason for hiding this comment

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

This cannot work because in case of reload, Caddy starts the new module before stopping the old one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I think I get it now. There's an experimental caddy.Exiting() api that we could maybe use instead.

Is there also a way to reload Caddy outside of tests? Running curl "http://localhost:2019/load" via the admin API also doesn't work on main.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nvm I realized you have to actually patch the whole config for a reload

curl "http://localhost:2019/config/apps/frankenphp" -X PATCH --header "Content-Type: application/json" -d '{"workers"
:[{"file_name":"/go/src/app/testdata/index.php","watch":["./**/*.{php,yaml,yml,twig,env}"]}]}' -H "Cache-Control: must-revalidate"

This works now with the Exiting() check if you're fine with using an experimental api. Otherwise I'll close this PR.

Copy link
Contributor

@francislavoie francislavoie Dec 14, 2024

Choose a reason for hiding this comment

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

You can use --force if you're using caddy reload or equivalent, which is the same as passing the Cache-Control: must-revalidate header in the API to reload even if the config text has not changed.

Copy link
Owner

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

LGTM!

@dunglas
Copy link
Owner

dunglas commented Dec 17, 2024

Could you please rebase?

@dunglas dunglas merged commit fbbc129 into main Dec 17, 2024
53 checks passed
@dunglas dunglas deleted the fix/graceful-shutdown branch December 17, 2024 17:10
@dunglas
Copy link
Owner

dunglas commented Dec 17, 2024

Thanks!

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.

Worker script itself is not shutdown gracefully, but hangs
5 participants