Repository navigation
Conversation
Caddy starts the new configuration before it stops the previous one. Since each php_server got its own frankenphp.Server, the new app's Start() unregisters the servers the previous HTTP server still routes to, so every request it handles until it stops fails with ErrNotRunning, answered with a 500. Such requests now wait for the new configuration to start and are handed to its module serving the same server name. Requests that cannot be forwarded get a 503 instead of a 500. Fixes #2698
Member
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2698, a regression in 1.13.0.
Caddy starts the new configuration before it stops the previous one. Since #2499, each
php_serverhas its ownfrankenphp.Server. The new app'sStart()callsfrankenphp.Shutdown(), which unregisters the servers the previous HTTP server still routes to. Until Caddy stops that HTTP server, every request it handles getsErrNotRunning, answered with a 500. The window covers the whole worker boot (Laravel Octane: about 20 failed requests per reload).How this fixes it:
activeAppbeforeShutdown().ErrNotRunning, it waits for that app to start, using the existing 10s stall. It then hands the request to the new module serving the same server name, as 1.12.7 effectively did by routing to the current workers.This is a smaller alternative to #2648. It doesn't replay the request through the Caddy handler chain and doesn't need to capture the body.
Not covered:
shutdown()drains PHP threads before it unregisters the servers. A request queued on a draining worker during that window can hang, as onmain. This is the side note in the issue, and a follow-up should reject those requests withErrNotRunningso they get forwarded as well.The new test sends requests in a loop during three reloads. It fails on
main(e.g. 186 脳 500, 5 脳 200) and passes with the fix.