diff --git a/caddy/admin_test.go b/caddy/admin_test.go index bbac1188c2..7979025667 100644 --- a/caddy/admin_test.go +++ b/caddy/admin_test.go @@ -540,6 +540,89 @@ func TestRejectedReloadWithAModuleWorkerKeepsThePreviousSiteServing(t *testing.T require.Equal(t, servedBefore+1, countedRequests(t, workerURL)) } +// requests still routed by the previous configuration must reach the new workers once booted +func TestReloadKeepsServingWhileWorkersBoot(t *testing.T) { + config := ` + { + skip_install_trust + admin localhost:2999 + http_port ` + testPort + ` + } + + localhost:` + testPort + ` { + php_server { + root ../testdata + worker { + file worker-slow-boot.php + num 1 + } + } + } + ` + + tester := caddytest.NewTester(t) + initServer(t, tester, config, "caddyfile") + + workerURL := "http://localhost:" + testPort + "/worker-slow-boot.php" + client := &http.Client{Timeout: 3 * time.Second} + done := make(chan struct{}) + var ( + wg sync.WaitGroup + mu sync.Mutex + statuses = map[int]int{} + ) + stop := sync.OnceFunc(func() { + close(done) + wg.Wait() + }) + t.Cleanup(stop) + + // one request per tick: a request stuck on a draining worker must not stop the probing + wg.Go(func() { + ticker := time.NewTicker(5 * time.Millisecond) + defer ticker.Stop() + + for { + select { + case <-done: + return + case <-ticker.C: + } + + wg.Go(func() { + resp, err := client.Get(workerURL) + if err != nil { + return + } + _, _ = io.Copy(io.Discard, resp.Body) + _ = resp.Body.Close() + + mu.Lock() + statuses[resp.StatusCode]++ + mu.Unlock() + }) + } + }) + + for range 3 { + r, err := http.NewRequest("POST", "http://localhost:2999/load", strings.NewReader(config)) + require.NoError(t, err) + r.Header.Set("Content-Type", "text/caddyfile") + // reload even though the configuration did not change + r.Header.Set("Cache-Control", "must-revalidate") + resp, err := http.DefaultClient.Do(r) + require.NoError(t, err) + _, _ = io.Copy(io.Discard, resp.Body) + require.NoError(t, resp.Body.Close()) + require.Equal(t, http.StatusOK, resp.StatusCode) + } + + stop() + + require.NotZero(t, statuses[http.StatusOK]) + assert.Equal(t, map[int]int{http.StatusOK: statuses[http.StatusOK]}, statuses) +} + // the number of requests testdata/worker-with-counter.php has served func countedRequests(t *testing.T, workerURL string) int { t.Helper() diff --git a/caddy/app.go b/caddy/app.go index 8dd40490d6..4560494ae3 100644 --- a/caddy/app.go +++ b/caddy/app.go @@ -23,6 +23,9 @@ import ( var ( options []frankenphp.Option optionsMU sync.RWMutex + + // the app whose servers are registered, or about to be + activeApp atomic.Pointer[FrankenPHPApp] ) // EXPERIMENTAL: RegisterWorkers provides a way for extensions to register frankenphp.Workers @@ -180,10 +183,15 @@ func (f *FrankenPHPApp) Start() error { return err } + // requests still reaching the previous configuration are forwarded to this one + activeApp.Store(f) + // if FrankenPHP is currently running, shut it down first // this will happen in admin API reloads and caddy tests frankenphp.Shutdown() if err := frankenphp.Init(f.opts...); err != nil { + activeApp.CompareAndSwap(f, nil) + return err } @@ -193,6 +201,8 @@ func (f *FrankenPHPApp) Start() error { } func (f *FrankenPHPApp) Stop() error { + activeApp.CompareAndSwap(f, nil) + if f.logger.Enabled(f.ctx, slog.LevelInfo) { f.logger.LogAttrs(f.ctx, slog.LevelInfo, "FrankenPHP stopped 🐘") } diff --git a/caddy/module.go b/caddy/module.go index 20dcec9ee2..767d09e61f 100644 --- a/caddy/module.go +++ b/caddy/module.go @@ -185,14 +185,9 @@ func needReplacement(s string) bool { } // ServeHTTP implements caddyhttp.MiddlewareHandler. -func (f *FrankenPHPModule) ServeHTTP(w http.ResponseWriter, r *http.Request, _ caddyhttp.Handler) error { - if !f.app.hasStarted.Load() { - // stall any incoming request if FrankenPHP has not started yet, blocking for up to 10 seconds - select { - case <-f.app.started: - case <-time.After(10 * time.Second): - return caddyhttp.Error(http.StatusServiceUnavailable, frankenphp.ErrNotRunning) - } +func (f *FrankenPHPModule) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddyhttp.Handler) error { + if !waitForStart(f.app) { + return caddyhttp.Error(http.StatusServiceUnavailable, frankenphp.ErrNotRunning) } ctx := r.Context() @@ -226,6 +221,9 @@ func (f *FrankenPHPModule) ServeHTTP(w http.ResponseWriter, r *http.Request, _ c } err := f.server.ServeHTTP(w, r, opts...) + if errors.Is(err, frankenphp.ErrNotRunning) { + return f.serveWithReplacement(w, r, next) + } if _, rejected := errors.AsType[frankenphp.ErrRejected](err); err != nil && !rejected { return caddyhttp.Error(http.StatusInternalServerError, err) @@ -234,6 +232,49 @@ func (f *FrankenPHPModule) ServeHTTP(w http.ResponseWriter, r *http.Request, _ c return nil } +// waitForStart stalls the request for up to 10 seconds if FrankenPHP has not started yet +func waitForStart(app *FrankenPHPApp) bool { + if app.hasStarted.Load() { + return true + } + + select { + case <-app.started: + return true + case <-time.After(10 * time.Second): + return false + } +} + +// serveWithReplacement hands the request to the configuration Caddy started before this one stopped routing +func (f *FrankenPHPModule) serveWithReplacement(w http.ResponseWriter, r *http.Request, next caddyhttp.Handler) error { + notRunning := caddyhttp.Error(http.StatusServiceUnavailable, frankenphp.ErrNotRunning) + + app := activeApp.Load() + if app == nil || app == f.app || !waitForStart(app) { + return notRunning + } + + var target *FrankenPHPModule + for _, m := range app.modules { + if m.server == nil || m.server.Name() != f.server.Name() { + continue + } + + // modules sharing a server are interchangeable, distinct servers with the same name are not + if target != nil && target.server != m.server { + return notRunning + } + target = m + } + + if target == nil { + return notRunning + } + + return target.ServeHTTP(w, r, next) +} + // UnmarshalCaddyfile implements caddyfile.Unmarshaler. func (f *FrankenPHPModule) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { for d.Next() { diff --git a/testdata/worker-slow-boot.php b/testdata/worker-slow-boot.php new file mode 100644 index 0000000000..be217b495b --- /dev/null +++ b/testdata/worker-slow-boot.php @@ -0,0 +1,9 @@ +