diff --git a/caddy/app.go b/caddy/app.go index 59c2e5e233..a4c4ce2e37 100644 --- a/caddy/app.go +++ b/caddy/app.go @@ -83,6 +83,10 @@ func (f *FrankenPHPApp) Provision(ctx caddy.Context) error { f.ctx = ctx f.logger = ctx.Slogger() + // drop the modules of a config that failed to provision, reset() only runs in Start() + f.modules = nil + f.usedWorkerNames = nil + // We have at least 7 hardcoded options f.opts = make([]frankenphp.Option, 0, 7+len(options)) @@ -371,6 +375,10 @@ func (f *FrankenPHPApp) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { if frankenphp.EmbeddedAppPath != "" && filepath.IsLocal(wc.FileName) { wc.FileName = filepath.Join(frankenphp.EmbeddedAppPath, wc.FileName) } + // a global worker has no php_server, so no set of requests to match against + if len(wc.MatchPath) != 0 { + return d.Errf(`"match" can only be used in a php_server worker block, not in a global one: %q`, wc.FileName) + } // check for duplicate workers for _, existingWorker := range f.Workers { if existingWorker.FileName == wc.FileName { diff --git a/caddy/config_test.go b/caddy/config_test.go index 04498ecf05..f5012003ca 100644 --- a/caddy/config_test.go +++ b/caddy/config_test.go @@ -7,9 +7,20 @@ import ( "github.com/caddyserver/caddy/v2" "github.com/caddyserver/caddy/v2/caddyconfig/caddyfile" + "github.com/dunglas/frankenphp" "github.com/stretchr/testify/require" ) +// options collected while provisioning a module, like the Mercure hub, must reach frankenphp +func TestWorkerOptionsKeepProvisionedOptions(t *testing.T) { + wc := workerConfig{FileName: "../testdata/worker-with-env.php"} + base := len(wc.toWorkerOptions()) + + wc.options = append(wc.options, frankenphp.WithWorkerMaxThreads(3)) + + require.Len(t, wc.toWorkerOptions(), base+1, "worker options set during provisioning must be forwarded") +} + func TestModuleRequestBodyTimeout(t *testing.T) { d := caddyfile.NewTestDispenser(` { diff --git a/caddy/module.go b/caddy/module.go index 52b895e838..5c25b76399 100644 --- a/caddy/module.go +++ b/caddy/module.go @@ -47,7 +47,7 @@ type FrankenPHPModule struct { Env map[string]string `json:"env,omitempty"` // Workers configures the worker scripts to start. Workers []workerConfig `json:"workers,omitempty"` - // ServerIdx is the idx of the php_server this module belongs to + // ServerIdx is set automatically to pair the route embeds of one php_server directive. Do not set it manually: modules sharing an index share one server, defined by the first of them. ServerIdx int `json:"server_idx,omitempty"` // RequestBodyTimeout is an idle timeout on request body reads: a stalled (slow POST) client is cut off while a steady upload of any size succeeds. Defaults to 60s when omitted; set to 0 to disable. RequestBodyTimeout *caddy.Duration `json:"request_body_timeout,omitempty"` @@ -188,6 +188,11 @@ func needReplacement(s string) bool { // ServeHTTP implements caddyhttp.MiddlewareHandler. func (f *FrankenPHPModule) ServeHTTP(w http.ResponseWriter, r *http.Request, _ caddyhttp.Handler) error { + // the server is assigned in FrankenPHPApp.Start(), which caddy may run after the http app started serving + if f.server == nil { + return caddyhttp.Error(http.StatusInternalServerError, frankenphp.ErrNotRunning) + } + ctx := r.Context() repl := ctx.Value(caddy.ReplacerCtxKey).(*caddy.Replacer) diff --git a/caddy/workerconfig.go b/caddy/workerconfig.go index 42e1fc2c18..2ce169c9dd 100644 --- a/caddy/workerconfig.go +++ b/caddy/workerconfig.go @@ -165,6 +165,9 @@ func (wc *workerConfig) toWorkerOptions() []frankenphp.WorkerOption { frankenphp.WithWorkerRequestOptions(wc.requestOptions...), } + // options collected while provisioning the module, e.g. the Mercure hub + opts = append(opts, wc.options...) + // copy the caddy match logic and create a unique matcher function for this worker // inject the matcher into frankenphp if len(wc.MatchPath) > 0 { diff --git a/cgi.go b/cgi.go index 582cdbb2a4..cfc8f6532b 100644 --- a/cgi.go +++ b/cgi.go @@ -171,11 +171,14 @@ func go_register_server_variables(threadIndex C.uintptr_t, trackVarsArray *C.zva thread := phpThreads[threadIndex] fc := thread.handler.frankenPHPContext() - if fc.request != nil { - addKnownVariablesToServer(fc, trackVarsArray) - addHeadersToServer(fc.ctx, fc.request, trackVarsArray) + if fc.request == nil { + // go_update_request_info() never ran, the thread-local prepared env still holds the previous request + return } + addKnownVariablesToServer(fc, trackVarsArray) + addHeadersToServer(fc.ctx, fc.request, trackVarsArray) + // The Prepared Environment is registered last and can overwrite any previous values if len(fc.env) != 0 || len(fc.server.env) != 0 { C.frankenphp_merge_with_prepared_env(trackVarsArray) diff --git a/context.go b/context.go index c5ed828a26..db05ff450c 100644 --- a/context.go +++ b/context.go @@ -122,14 +122,21 @@ func newWorkerDummyContext(w *worker) (*frankenPHPContext, error) { return nil, err } + server := w.server + if server == nil { + // global worker, not associated with a server + server = fallbackServer + } + fc := &frankenPHPContext{ done: make(chan any), ctx: r.Context(), - server: w.server, + server: server, request: r, startedAt: time.Now(), - logger: globalLogger, - worker: w, + // startup output of a scoped worker belongs to its server's logger + logger: server.logger.Load(), + worker: w, } for _, o := range w.requestOptions { @@ -138,11 +145,6 @@ func newWorkerDummyContext(w *worker) (*frankenPHPContext, error) { } } - if fc.server == nil { - // global worker, not associated with a server - fc.server = fallbackServer - } - splitCgiPath(fc) return fc, nil @@ -150,26 +152,25 @@ func newWorkerDummyContext(w *worker) (*frankenPHPContext, error) { // newContextFromMessage creates a context from a message (external workers) func newContextFromMessage(message any, rw http.ResponseWriter, ctx context.Context, w *worker) *frankenPHPContext { - fc := &frankenPHPContext{ + server := w.server + if server == nil { + server = fallbackServer + } + + if ctx == nil { + ctx = globalCtx + } + + return &frankenPHPContext{ done: make(chan any), startedAt: time.Now(), - server: w.server, + server: server, worker: w, - logger: globalLogger, + logger: server.logger.Load(), responseWriter: rw, handlerParameters: message, ctx: ctx, } - - if fc.server == nil { - fc.server = fallbackServer - } - - if fc.ctx == nil { - fc.ctx = globalCtx - } - - return fc } // closeContext sends the response to the client diff --git a/docs/library.md b/docs/library.md index a78b2ff8c9..1d4c82c1bd 100644 --- a/docs/library.md +++ b/docs/library.md @@ -46,7 +46,7 @@ func main() { `NewServer()` takes a human-readable name used to attribute workers, metrics and logs to the server (defaults to `server_` at registration when empty), the document root, the split path suffixes (defaults to `[".php"]`), environment variables made available to every request, and a `*slog.Logger` (defaults to the global logger). -`Init()` starts the PHP runtime and must be called exactly once before serving requests; `Shutdown()` stops it. Calling `Server.ServeHTTP()` before `Init()` or after `Shutdown()` returns `ErrNotRunning`. +`Init()` starts the PHP runtime and must be called exactly once before serving requests; `Shutdown()` stops it. Calling `Server.ServeHTTP()` before `Init()` or after `Shutdown()` returns `ErrNotRunning`. The same `*Server` may be passed to `Init()` again after a `Shutdown()`, for instance to reload the configuration. ## Multiple servers @@ -87,7 +87,7 @@ err := frankenphp.Init( ) ``` -Workers declared without a server scope are global: they match by file path on any server. +Workers declared without a server scope are global: they match by file path on any server. Since a global worker has no set of requests to match against, combining `WithWorkerMatcher()` with a global worker is a configuration error and `Init()` rejects it. ## Per-request options diff --git a/frankenphp.go b/frankenphp.go index 8b2a70efb8..6fa728e51e 100644 --- a/frankenphp.go +++ b/frankenphp.go @@ -342,6 +342,9 @@ func Init(options ...Option) error { initAutoScaling(mainThread) + // only now that the workers and threads are up may requests reach a server + activateServers() + if globalLogger.Enabled(globalCtx, slog.LevelInfo) { globalLogger.LogAttrs(globalCtx, slog.LevelInfo, "FrankenPHP started 🐘", slog.String("php_version", Version().Version), slog.Int("num_threads", mainThread.numThreads), slog.Int("max_threads", mainThread.maxThreads), slog.Int("max_requests", maxRequestsPerThread)) @@ -782,6 +785,8 @@ func resetGlobals() { workers = nil workersByName = nil globalWorkersByPath = nil + servers = nil + fallbackServer.logger.Store(globalLogger) watcherIsEnabled = false maxIdleTime = defaultMaxIdleTime maxRequestsPerThread = 0 diff --git a/server.go b/server.go index 84f7acb66f..675095a1be 100644 --- a/server.go +++ b/server.go @@ -13,7 +13,9 @@ import ( // Server represents a preconfigured server block // requests and workers can be scoped to a Server type Server struct { - idx int + idx int + // name passed to NewServer(), kept so re-registering resolves the default anew + configuredName string name string root string splitPath []string @@ -21,7 +23,6 @@ type Server struct { workers []*worker workersByPath map[string]*worker workersWithRequestMatcher []*worker - workerOpts []workerOpt // registered while FrankenPHP runs with this server; read by concurrent // ServeHTTP calls while Init()/Shutdown() flip it, hence atomic @@ -48,15 +49,29 @@ func newFallbackServer() *Server { return s } +// registerServers assigns the identity of every server and clears the workers of a previous run, +// so the same *Server can be passed to Init() again after a Shutdown() +// servers do not accept requests yet at this point, see activateServers() func registerServers(newServers []*Server) { servers = newServers fallbackServer.logger.Store(globalLogger) - fallbackServer.isRegistered.Store(true) + fallbackServer.resetWorkers() + for i, s := range servers { s.idx = i + s.name = s.configuredName if s.name == "" { s.name = "server_" + strconv.Itoa(i) } + s.resetWorkers() + } +} + +// activateServers lets registered servers accept requests +// it runs once workers and threads are up, so a request cannot reach a server before them +func activateServers() { + fallbackServer.isRegistered.Store(true) + for _, s := range servers { s.isRegistered.Store(true) } } @@ -66,6 +81,14 @@ func unregisterServers() { for _, server := range servers { server.isRegistered.Store(false) } + servers = nil +} + +// resetWorkers drops the workers of a previous run; initWorkers() adds them back +func (s *Server) resetWorkers() { + s.workers = nil + s.workersByPath = make(map[string]*worker) + s.workersWithRequestMatcher = nil } // NewServer creates a Server that can be registered via WithServer(). @@ -83,12 +106,12 @@ func NewServer(name, root string, splitPath []string, env map[string]string, log } s := &Server{ - name: name, - root: root, - splitPath: splitPath, - env: PrepareEnv(env), - workersByPath: make(map[string]*worker), - workerOpts: make([]workerOpt, 0), + configuredName: name, + name: name, + root: root, + splitPath: splitPath, + env: PrepareEnv(env), + workersByPath: make(map[string]*worker), } if logger == nil { diff --git a/server_test.go b/server_test.go index 00db5ea199..bef6149cf8 100644 --- a/server_test.go +++ b/server_test.go @@ -172,6 +172,42 @@ func TestServer(t *testing.T) { assert.Contains(t, err.Error(), "two workers in a server cannot have the same filename") }) + t.Run("error_on_request_matcher_without_server_scope", func(t *testing.T) { + t.Cleanup(frankenphp.Shutdown) + + err := frankenphp.Init( + frankenphp.WithWorkers("global", testDataDir+"worker-with-counter.php", 1, + frankenphp.WithWorkerMatcher(func(*http.Request) bool { return true }), + ), + ) + + assert.Error(t, err) + assert.Contains(t, err.Error(), "no server scope") + }) + + // re-registering a Server must not trip the duplicate filename check on its own workers + t.Run("reregistration_after_shutdown", func(t *testing.T) { + server, err := frankenphp.NewServer("", testDataDir, nil, nil, nil) + require.NoError(t, err) + + opts := []frankenphp.Option{ + frankenphp.WithServer(server), + frankenphp.WithWorkers("counter", testDataDir+"worker-with-counter.php", 1, + frankenphp.WithWorkerServerScope(server), + ), + } + + initServers(t, opts...) + assert.Equal(t, "requests:1", serverGet(t, server, "http://example.com/worker-with-counter.php")) + assert.Equal(t, "server_0", server.Name()) + + frankenphp.Shutdown() + + initServers(t, opts...) + assert.Equal(t, "requests:1", serverGet(t, server, "http://example.com/worker-with-counter.php")) + assert.Equal(t, "server_0", server.Name()) + }) + t.Run("error_on_missing_registration", func(t *testing.T) { server, _ := frankenphp.NewServer("", testDataDir, nil, nil, nil) diff --git a/worker.go b/worker.go index cf684e8f7c..ecbed9e392 100644 --- a/worker.go +++ b/worker.go @@ -123,12 +123,19 @@ func newWorker(o workerOpt) (*worker, error) { o.name = absFileName } - if o.server == nil && globalWorkersByPath[absFileName] != nil { - return nil, fmt.Errorf("two global workers cannot have the same filename: %q", absFileName) + if o.server == nil { + if globalWorkersByPath[absFileName] != nil { + return nil, fmt.Errorf("two global workers cannot have the same filename: %q", absFileName) + } + + // no server means no set of requests to match against, the matcher would never run + if o.matchRequest != nil { + return nil, fmt.Errorf("worker %q has a request matcher but no server scope, use WithWorkerServerScope()", o.name) + } } - if w := workersByName[o.name]; w != nil { - return w, fmt.Errorf("two workers cannot have the same name: %q", o.name) + if workersByName[o.name] != nil { + return nil, fmt.Errorf("two workers cannot have the same name: %q", o.name) } // env should always contain FRANKENPHP_WORKER and the parent php_server env