diff --git a/caddy/admin_test.go b/caddy/admin_test.go index 27ece031f3..5ec45fa7c4 100644 --- a/caddy/admin_test.go +++ b/caddy/admin_test.go @@ -348,3 +348,49 @@ func TestAddModuleWorkerViaAdminApi(t *testing.T) { // Make a request to the worker to verify it's working tester.AssertGetResponse("http://localhost:"+testPort+"/worker-with-counter.php", http.StatusOK, "requests:1") } + +func TestRegisteredModuleWorkerPoolsMustBeCorrect(t *testing.T) { + tester := caddytest.NewTester(t) + initServer(t, tester, ` + { + skip_install_trust + admin localhost:2999 + + frankenphp { + num_threads 4 + worker ../testdata/worker-with-env.php 1 + } + } + + http://localhost:`+testPort+` { + route { + php { + root ../testdata + worker worker-with-counter.php 1 { + match /matched* + } + } + php { + root ../testdata + worker worker.php 1 + } + } + } + `, "caddyfile") + + debugState := getDebugState(t, tester) + + worker1Path, _ := fastabs.FastAbs("../testdata/worker-with-env.php") + worker2Path, _ := fastabs.FastAbs("../testdata/worker-with-counter.php") + worker3Path, _ := fastabs.FastAbs("../testdata/worker.php") + receivedThreadNames := make([]string, 0) + for _, thread := range debugState.ThreadDebugStates { + receivedThreadNames = append(receivedThreadNames, thread.Name) + } + + assert.Len(t, receivedThreadNames, 4, "expected 4 threads to be present") + assert.Contains(t, receivedThreadNames, "Regular PHP Thread", "expected a regular thread to be present") + assert.Contains(t, receivedThreadNames, "Worker PHP Thread - "+worker1Path, "expected global worker to be present") + assert.Contains(t, receivedThreadNames, "Worker PHP Thread - "+worker2Path, "expected module worker with \"match\" directive to be present") + assert.Contains(t, receivedThreadNames, "Worker PHP Thread - "+worker3Path, "expected module worker without \"match\" directive to be present") +} diff --git a/caddy/app.go b/caddy/app.go index e4593acc49..8a0f968824 100644 --- a/caddy/app.go +++ b/caddy/app.go @@ -17,7 +17,6 @@ import ( "github.com/caddyserver/caddy/v2/caddyconfig/httpcaddyfile" "github.com/caddyserver/caddy/v2/modules/caddyhttp" "github.com/dunglas/frankenphp" - "github.com/dunglas/frankenphp/internal/fastabs" ) var ( @@ -101,74 +100,6 @@ func (f *FrankenPHPApp) Provision(ctx caddy.Context) error { return nil } -func (f *FrankenPHPApp) generateUniqueModuleWorkerName(filepath string) string { - var i uint - filepath, _ = fastabs.FastAbs(filepath) - name := "m#" + filepath - -retry: - for _, wc := range f.Workers { - if wc.Name == name { - name = fmt.Sprintf("m#%s_%d", filepath, i) - i++ - - goto retry - } - } - - return name -} - -func (f *FrankenPHPApp) addModuleWorkers(workers ...workerConfig) ([]workerConfig, error) { - for i := range workers { - w := &workers[i] - - if frankenphp.EmbeddedAppPath != "" && filepath.IsLocal(w.FileName) { - w.FileName = filepath.Join(frankenphp.EmbeddedAppPath, w.FileName) - } - } - - // A php_server directive is provisioned once per route it's embedded in. Only the first embed - // registers its pools; later embeds reuse them by position, never touching other directives (#2477). - var registered []workerConfig - if len(workers) > 0 && workers[0].routeGroup != "" { - registered = f.moduleWorkersInRouteGroup(workers[0].routeGroup) - } - - for i := range workers { - if i < len(registered) { - workers[i].Name = registered[i].Name - continue - } - - f.registerModuleWorker(&workers[i]) - } - - return workers, nil -} - -func (f *FrankenPHPApp) registerModuleWorker(w *workerConfig) { - if w.Name == "" { - w.Name = f.generateUniqueModuleWorkerName(w.FileName) - } else if !strings.HasPrefix(w.Name, "m#") { - w.Name = "m#" + w.Name - } - - f.Workers = append(f.Workers, *w) -} - -// moduleWorkersInRouteGroup returns the registered workers of one directive, in registration order. -func (f *FrankenPHPApp) moduleWorkersInRouteGroup(routeGroup string) []workerConfig { - var group []workerConfig - for _, w := range f.Workers { - if w.routeGroup == routeGroup { - group = append(group, w) - } - } - - return group -} - func (f *FrankenPHPApp) Start() error { repl := caddy.NewReplacer() @@ -189,15 +120,8 @@ func (f *FrankenPHPApp) Start() error { ) for _, w := range f.Workers { - w.options = append(w.options, - frankenphp.WithWorkerEnv(w.Env), - frankenphp.WithWorkerWatchMode(w.Watch), - frankenphp.WithWorkerMaxFailures(w.MaxConsecutiveFailures), - frankenphp.WithWorkerMaxThreads(w.MaxThreads), - frankenphp.WithWorkerRequestOptions(w.requestOptions...), - ) - - f.opts = append(f.opts, frankenphp.WithWorkers(w.Name, repl.ReplaceKnown(w.FileName, ""), w.Num, w.options...)) + w.FileName = repl.ReplaceKnown(w.FileName, "") + f.opts = append(f.opts, frankenphp.WithWorkers(w.Name, w.FileName, w.Num, w.toWorkerOptions()...)) } frankenphp.Shutdown() diff --git a/caddy/config_test.go b/caddy/config_test.go index c0da09c565..49a6739e8c 100644 --- a/caddy/config_test.go +++ b/caddy/config_test.go @@ -1,104 +1,12 @@ package caddy import ( - "strings" "testing" "github.com/caddyserver/caddy/v2/caddyconfig/caddyfile" "github.com/stretchr/testify/require" ) -func TestPhpServerWorkerMatchNoDuplicatePools(t *testing.T) { - const config = ` - { - php { - worker { - file ../testdata/worker-with-env.php - num 2 - match /index.php/* - } - worker { - file ../testdata/worker-with-counter.php - num 1 - } - } - }` - - // a fresh copy per call mirrors the two module instances Caddy provisions for one directive - parseWorkers := func(routeGroup string) []workerConfig { - d := caddyfile.NewTestDispenser(config) - module := &FrankenPHPModule{} - require.NoError(t, module.UnmarshalCaddyfile(d)) - require.Len(t, module.Workers, 2, "block should parse to two workers") - for i := range module.Workers { - module.Workers[i].routeGroup = routeGroup - } - - return module.Workers - } - - app := &FrankenPHPApp{} - - first, err := app.addModuleWorkers(parseWorkers("g1")...) - require.NoError(t, err) - second, err := app.addModuleWorkers(parseWorkers("g1")...) - require.NoError(t, err) - - require.Len(t, app.Workers, 2, "each worker must be registered exactly once") - - for _, w := range app.Workers { - require.False(t, strings.HasSuffix(w.Name, "_0"), - "no _0-suffixed duplicate pool may exist, got %q", w.Name) - } - - // both embeds must resolve to the same pools for serve-time matching - require.Len(t, first, 2) - require.Len(t, second, 2) - for i := range first { - require.Equal(t, first[i].Name, second[i].Name, - "both routes must reference the same pool name for worker %d", i) - } -} - -func TestPhpServerSeparateDirectivesKeepDistinctPools(t *testing.T) { - worker := func(routeGroup string) workerConfig { - return workerConfig{FileName: "../testdata/worker-with-env.php", Num: 2, routeGroup: routeGroup} - } - - app := &FrankenPHPApp{} - site1, err := app.addModuleWorkers(worker("g1")) - require.NoError(t, err) - site2, err := app.addModuleWorkers(worker("g2")) - require.NoError(t, err) - - require.Len(t, app.Workers, 2, "identical workers from separate directives must stay separate pools") - require.NotEqual(t, site1[0].Name, site2[0].Name, "separate directives must not share a pool name") - require.True(t, strings.HasSuffix(site2[0].Name, "_0"), - "the second directive's pool must take a unique _0 name, got %q", site2[0].Name) -} - -func TestPhpServerEmbedReuseIsPositional(t *testing.T) { - embed := func() []workerConfig { - return []workerConfig{ - {FileName: "../testdata/worker-with-env.php", Num: 1, MatchPath: []string{"/x/*"}, routeGroup: "g1"}, - {FileName: "../testdata/worker-with-env.php", Num: 1, MatchPath: []string{"/x/*"}, routeGroup: "g1"}, - } - } - - app := &FrankenPHPApp{} - first, err := app.addModuleWorkers(embed()...) - require.NoError(t, err) - second, err := app.addModuleWorkers(embed()...) - require.NoError(t, err) - - require.Len(t, app.Workers, 2, "two identical workers in one block stay two pools, just as without the duplicate embed") - require.NotEqual(t, first[0].Name, first[1].Name, "the second identical worker must take its own _0 pool") - require.True(t, strings.HasSuffix(first[1].Name, "_0"), "got %q", first[1].Name) - for i := range first { - require.Equal(t, first[i].Name, second[i].Name, "the duplicate embed must reuse pools by position for worker %d", i) - } -} - func TestModuleWorkerDuplicateFilenamesFail(t *testing.T) { // Create a test configuration with duplicate worker filenames configWithDuplicateFilenames := ` @@ -168,7 +76,6 @@ func TestModuleWorkersDifferentNamesSucceed(t *testing.T) { // Parse the first configuration d1 := caddyfile.NewTestDispenser(configWithWorkerName1) - app := &FrankenPHPApp{} module1 := &FrankenPHPModule{} // Unmarshal the first configuration @@ -196,16 +103,6 @@ func TestModuleWorkersDifferentNamesSucceed(t *testing.T) { // Verify that no error was returned require.NoError(t, err, "Expected no error when two workers have different names") - - _, err = app.addModuleWorkers(module1.Workers...) - require.NoError(t, err, "Expected no error when adding the first module workers") - _, err = app.addModuleWorkers(module2.Workers...) - require.NoError(t, err, "Expected no error when adding the second module workers") - - // Verify that both workers were added - require.Len(t, app.Workers, 2, "Expected two workers in the app") - require.Equal(t, "m#test-worker-1", app.Workers[0].Name, "First worker should have the correct name") - require.Equal(t, "m#test-worker-2", app.Workers[1].Name, "Second worker should have the correct name") } func TestModuleWorkerWithEnvironmentVariables(t *testing.T) { @@ -294,7 +191,6 @@ func TestModuleWorkerWithCustomName(t *testing.T) { // Parse the configuration d := caddyfile.NewTestDispenser(configWithCustomName) module := &FrankenPHPModule{} - app := &FrankenPHPApp{} // Unmarshal the configuration err := module.UnmarshalCaddyfile(d) @@ -305,10 +201,4 @@ func TestModuleWorkerWithCustomName(t *testing.T) { // Verify that the worker was added to the module require.Len(t, module.Workers, 1, "Expected one worker to be added to the module") require.Equal(t, "../testdata/worker-with-env.php", module.Workers[0].FileName, "Worker should have the correct filename") - - // Verify that the worker was added to app.Workers with the m# prefix - module.Workers, err = app.addModuleWorkers(module.Workers...) - require.NoError(t, err, "Expected no error when adding the worker to the app") - require.Equal(t, "m#custom-worker-name", module.Workers[0].Name, "Worker should have the custom name, prefixed with m#") - require.Equal(t, "m#custom-worker-name", app.Workers[0].Name, "Worker should have the custom name, prefixed with m#") } diff --git a/caddy/hotreload.go b/caddy/hotreload.go index 4b9c0ebe69..bcab369372 100644 --- a/caddy/hotreload.go +++ b/caddy/hotreload.go @@ -26,6 +26,7 @@ type hotReloadConfig struct { Watch []string `json:"watch"` } +// TODO: this should be scoped to the php_server to avoid duplicate hot reloads func (f *FrankenPHPModule) configureHotReload(app *FrankenPHPApp) error { if f.HotReload == nil { return nil @@ -49,7 +50,12 @@ func (f *FrankenPHPModule) configureHotReload(app *FrankenPHPApp) error { } app.opts = append(app.opts, frankenphp.WithHotReload(f.HotReload.Topic, f.mercureHub, f.HotReload.Watch)) - f.preparedEnv["FRANKENPHP_HOT_RELOAD\x00"] = "/.well-known/mercure?topic=" + url.QueryEscape(f.HotReload.Topic) + + // add the hot reload to the env variables + if f.Env == nil { + f.Env = make(map[string]string) + } + f.Env["FRANKENPHP_HOT_RELOAD"] = "/.well-known/mercure?topic=" + url.QueryEscape(f.HotReload.Topic) return nil } diff --git a/caddy/module.go b/caddy/module.go index bf57c0efa0..763d5476d3 100644 --- a/caddy/module.go +++ b/caddy/module.go @@ -4,7 +4,6 @@ import ( "encoding/json" "errors" "fmt" - "log/slog" "net/http" "path/filepath" "runtime" @@ -45,14 +44,12 @@ type FrankenPHPModule struct { Env map[string]string `json:"env,omitempty"` // Workers configures the worker scripts to start. Workers []workerConfig `json:"workers,omitempty"` - // RouteGroup is set automatically to pair the route embeds of one php_server directive (#2477). Do not set it manually. - RouteGroup string `json:"route_group,omitempty"` - - resolvedDocumentRoot string - preparedEnv frankenphp.PreparedEnv - preparedEnvNeedsReplacement bool - logger *slog.Logger - requestOptions []frankenphp.RequestOption + // PhpServerIdx is the idx of the php_server this module belongs to + PhpServerIdx int `json:"php_server_idx,omitempty"` + + resolvedDocumentRoot string + preparedEnv frankenphp.PreparedEnv + requestOptions []frankenphp.RequestOption } // CaddyModule returns the Caddy module information. @@ -65,7 +62,6 @@ func (FrankenPHPModule) CaddyModule() caddy.ModuleInfo { // Provision sets up the module. func (f *FrankenPHPModule) Provision(ctx caddy.Context) error { - f.logger = ctx.Slogger() app, err := ctx.App("frankenphp") if err != nil { return err @@ -80,7 +76,6 @@ func (f *FrankenPHPModule) Provision(ctx caddy.Context) error { f.assignMercureHub(ctx) - loggerOpt := frankenphp.WithRequestLogger(f.logger) for i, wc := range f.Workers { // make the file path absolute from the public directory // this can only be done if the root is defined inside php_server @@ -88,21 +83,23 @@ func (f *FrankenPHPModule) Provision(ctx caddy.Context) error { wc.FileName = filepath.Join(f.Root, wc.FileName) } - // Inherit environment variables from the parent php_server directive - if f.Env != nil { - wc.inheritEnv(f.Env) - } - - wc.requestOptions = append(wc.requestOptions, loggerOpt) - wc.routeGroup = f.RouteGroup f.Workers[i] = wc } - workers, err := fapp.addModuleWorkers(f.Workers...) - if err != nil { - return err + for i, wc := range f.Workers { + if frankenphp.EmbeddedAppPath != "" && filepath.IsLocal(wc.FileName) { + wc.FileName = filepath.Join(frankenphp.EmbeddedAppPath, wc.FileName) + } + + phpServerPrefix := fmt.Sprintf("m%d#", f.PhpServerIdx) + if wc.Name == "" { + wc.Name = f.generateUniqueModuleWorkerName(wc.FileName, phpServerPrefix) + } else if !strings.HasPrefix(wc.Name, phpServerPrefix) { + wc.Name = phpServerPrefix + wc.Name + } + + f.Workers[i] = wc } - f.Workers = workers if f.Root == "" { if frankenphp.EmbeddedAppPath == "" { @@ -116,25 +113,10 @@ func (f *FrankenPHPModule) Provision(ctx caddy.Context) error { f.Root = filepath.Join(frankenphp.EmbeddedAppPath, f.Root) } - if len(f.SplitPath) == 0 { - f.SplitPath = []string{".php"} - } - - opt, err := frankenphp.WithRequestSplitPath(f.SplitPath) - if err != nil { - return fmt.Errorf("invalid split_path: %w", err) - } - f.requestOptions = append(f.requestOptions, opt) - if f.ResolveRootSymlink == nil { f.ResolveRootSymlink = new(true) } - // Always pre-compute absolute file names for fallback matching - for i := range f.Workers { - f.Workers[i].absFileName, _ = fastabs.FastAbs(f.Workers[i].FileName) - } - if !needReplacement(f.Root) { root, err := fastabs.FastAbs(f.Root) if err != nil { @@ -155,45 +137,65 @@ func (f *FrankenPHPModule) Provision(ctx caddy.Context) error { if filepath.IsAbs(wc.FileName) { resolvedPath, _ := filepath.EvalSymlinks(wc.FileName) f.Workers[i].FileName = resolvedPath - f.Workers[i].absFileName = resolvedPath } } } - // Pre-compute relative match paths for all workers (requires resolved document root) - docRootWithSep := f.resolvedDocumentRoot + string(filepath.Separator) - for i := range f.Workers { - if strings.HasPrefix(f.Workers[i].absFileName, docRootWithSep) { - f.Workers[i].matchRelPath = filepath.ToSlash(f.Workers[i].absFileName[len(f.resolvedDocumentRoot):]) - } - } - - f.requestOptions = append(f.requestOptions, frankenphp.WithRequestResolvedDocumentRoot(f.resolvedDocumentRoot)) } - if f.preparedEnv == nil { - f.preparedEnv = frankenphp.PrepareEnv(f.Env) + if err := f.configureHotReload(fapp); err != nil { + return err + } - for _, e := range f.preparedEnv { - if needReplacement(e) { - f.preparedEnvNeedsReplacement = true + unchangingEnv := make(map[string]string) // env variables that do not need replacement + requestEnv := make(map[string]string) // env variables that need replacement, e.g. {http.vars.root} - break - } + for k, e := range f.Env { + if needReplacement(e) { + requestEnv[k] = e + } else { + unchangingEnv[k] = e } } - if !f.preparedEnvNeedsReplacement { - f.requestOptions = append(f.requestOptions, frankenphp.WithRequestPreparedEnv(f.preparedEnv)) + f.preparedEnv = frankenphp.PrepareEnv(requestEnv) + + // note: duplicate PhpServerIdx registration will be ignored, only the first one will be used + // this is necessary since caddy drops the module instance between parsing and provisioning + phpServerOptions := []frankenphp.PhpServerOption{ + frankenphp.WithPhpServerRoot(f.resolvedDocumentRoot, *f.ResolveRootSymlink), + frankenphp.WithPhpServerEnv(unchangingEnv), + frankenphp.WithPHPServerLogger(ctx.Slogger()), + frankenphp.WithPhpServerSplitPath(f.SplitPath), } - if err := f.configureHotReload(fapp); err != nil { - return err + for _, w := range f.Workers { + phpServerOptions = append(phpServerOptions, frankenphp.WithPhpServerWorker(w.Name, w.FileName, w.Num, w.toWorkerOptions()...)) } + fapp.opts = append(fapp.opts, frankenphp.WithPhpServer(f.PhpServerIdx, phpServerOptions...)) + return nil } +func (f *FrankenPHPModule) generateUniqueModuleWorkerName(filepath string, phpServerPrefix string) string { + var i uint + filepath, _ = fastabs.FastAbs(filepath) + name := phpServerPrefix + filepath + +retry: + for _, wc := range f.Workers { + if wc.Name == name { + name = phpServerPrefix + filepath + fmt.Sprintf("_%d", i) + i++ + + goto retry + } + } + + return name +} + // needReplacement checks if a string contains placeholders. func needReplacement(s string) bool { return strings.ContainsAny(s, "{}") @@ -208,6 +210,7 @@ func (f *FrankenPHPModule) ServeHTTP(w http.ResponseWriter, r *http.Request, _ c opts := make([]frankenphp.RequestOption, 0, len(f.requestOptions)+4) opts = append(opts, f.requestOptions...) + opts = append(opts, frankenphp.WithOriginalRequest(new(ctx.Value(caddyhttp.OriginalRequestCtxKey).(http.Request)))) if documentRoot == "" { documentRoot = repl.ReplaceKnown(f.Root, "") @@ -221,8 +224,8 @@ func (f *FrankenPHPModule) ServeHTTP(w http.ResponseWriter, r *http.Request, _ c opts = append(opts, frankenphp.WithRequestDocumentRoot(documentRoot, false)) } - if f.preparedEnvNeedsReplacement { - env := make(frankenphp.PreparedEnv, len(f.Env)) + if len(f.preparedEnv) > 0 { + env := make(frankenphp.PreparedEnv, len(f.preparedEnv)) for k, v := range f.preparedEnv { env[k] = repl.ReplaceKnown(v, "") } @@ -230,28 +233,14 @@ func (f *FrankenPHPModule) ServeHTTP(w http.ResponseWriter, r *http.Request, _ c opts = append(opts, frankenphp.WithRequestPreparedEnv(env)) } - workerName := "" - for _, w := range f.Workers { - if w.matchesPath(r, documentRoot) { - workerName = w.Name - break - } + phpServer := frankenphp.PhpServers[f.PhpServerIdx] + if phpServer == nil { + return fmt.Errorf("php server with idx %d was not correctly provisioned", f.PhpServerIdx) } - fr, err := frankenphp.NewRequestWithContext( - r, - append( - opts, - frankenphp.WithOriginalRequest(new(ctx.Value(caddyhttp.OriginalRequestCtxKey).(http.Request))), - frankenphp.WithWorkerName(workerName), - )..., - ) - - if err != nil { - return caddyhttp.Error(http.StatusInternalServerError, err) - } + err := phpServer.ServeHTTP(w, r, opts...) - if err = frankenphp.ServeHTTP(w, fr); err != nil && !errors.As(err, &frankenphp.ErrRejected{}) { + if err != nil && !errors.As(err, &frankenphp.ErrRejected{}) { return caddyhttp.Error(http.StatusInternalServerError, err) } @@ -282,10 +271,8 @@ func (f *FrankenPHPModule) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { } if f.Env == nil { f.Env = make(map[string]string) - f.preparedEnv = make(frankenphp.PreparedEnv) } f.Env[args[0]] = args[1] - f.preparedEnv[args[0]+"\x00"] = args[1] case "resolve_root_symlink": if !d.NextArg() { @@ -334,16 +321,22 @@ func (f *FrankenPHPModule) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { return nil } +func (f *FrankenPHPModule) assignPhpServerIdx(h httpcaddyfile.Helper) { + counter, _ := h.State["php_server_count"].(int) + f.PhpServerIdx = counter + h.State["php_server_count"] = counter + 1 +} + // parseCaddyfile unmarshals tokens from h into a new Middleware. func parseCaddyfile(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, error) { m := &FrankenPHPModule{} err := m.UnmarshalCaddyfile(h.Dispenser) + m.assignPhpServerIdx(h) + return m, err } -const routeGroupStateKey = "frankenphp.worker_route_group_seq" - // parsePhpServer parses the php_server directive, which has a similar syntax // to the php_fastcgi directive. A line such as this: // @@ -376,12 +369,6 @@ func parsePhpServer(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error) return nil, h.ArgErr() } - // per-adaptation counter: identical for both embeds of this directive, distinct for every other - // (including separate snippet imports), and stable across re-adaptation since State resets each time - seq, _ := h.State[routeGroupStateKey].(int) - h.State[routeGroupStateKey] = seq + 1 - routeGroup := strconv.Itoa(seq) - // set up FrankenPHP phpsrv := FrankenPHPModule{} @@ -481,6 +468,9 @@ func parsePhpServer(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error) return nil, err } + // assign a unique index to the php server + phpsrv.assignPhpServerIdx(h) + if frankenphp.EmbeddedAppPath != "" { if phpsrv.Root == "" { phpsrv.Root = filepath.Join(frankenphp.EmbeddedAppPath, defaultDocumentRoot) @@ -492,8 +482,6 @@ func parsePhpServer(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error) } } - phpsrv.RouteGroup = routeGroup - // set up a route list that we'll append to routes := caddyhttp.RouteList{} diff --git a/caddy/workerconfig.go b/caddy/workerconfig.go index 0ade09fee3..2464306a17 100644 --- a/caddy/workerconfig.go +++ b/caddy/workerconfig.go @@ -1,8 +1,6 @@ package caddy import ( - "net/http" - "path" "path/filepath" "strconv" @@ -10,7 +8,6 @@ import ( "github.com/caddyserver/caddy/v2/caddyconfig/caddyfile" "github.com/caddyserver/caddy/v2/modules/caddyhttp" "github.com/dunglas/frankenphp" - "github.com/dunglas/frankenphp/internal/fastabs" ) // workerConfig represents the "worker" directive in the Caddyfile @@ -44,9 +41,6 @@ type workerConfig struct { options []frankenphp.WorkerOption requestOptions []frankenphp.RequestOption - absFileName string - matchRelPath string // pre-computed relative URL path for fast matching - routeGroup string // identifies the php_server directive whose route embeds share this pool } func unmarshalWorker(d *caddyfile.Dispenser) (workerConfig, error) { @@ -162,41 +156,21 @@ func unmarshalWorker(d *caddyfile.Dispenser) (workerConfig, error) { return wc, nil } -func (wc *workerConfig) inheritEnv(env map[string]string) { - if wc.Env == nil { - wc.Env = make(map[string]string, len(env)) +func (wc *workerConfig) toWorkerOptions() []frankenphp.WorkerOption { + opts := []frankenphp.WorkerOption{ + frankenphp.WithWorkerEnv(wc.Env), + frankenphp.WithWorkerWatchMode(wc.Watch), + frankenphp.WithWorkerMaxFailures(wc.MaxConsecutiveFailures), + frankenphp.WithWorkerMaxThreads(wc.MaxThreads), + frankenphp.WithWorkerRequestOptions(wc.requestOptions...), } - for k, v := range env { - // do not overwrite existing environment variables - if _, exists := wc.Env[k]; !exists { - wc.Env[k] = v - } - } -} -func (wc *workerConfig) matchesPath(r *http.Request, documentRoot string) bool { - // try to match against a pattern if one is assigned - if len(wc.MatchPath) != 0 { - return (caddyhttp.MatchPath)(wc.MatchPath).Match(r) + // copy the caddy match logic and create a unique matcher function for this worker + // inject the matcher into frankenphp + if len(wc.MatchPath) > 0 { + matchFunc := caddyhttp.MatchPath(append([]string(nil), wc.MatchPath...)) + _ = matchFunc.Provision(caddy.Context{}) + opts = append(opts, frankenphp.WithWorkerMatchOn(matchFunc.Match)) } - - // fast path: compare the request URL path against the pre-computed relative path - if wc.matchRelPath != "" { - reqPath := r.URL.Path - if reqPath == wc.matchRelPath { - return true - } - - // ensure leading slash for relative paths (see #2166) - if reqPath == "" || reqPath[0] != '/' { - reqPath = "/" + reqPath - } - - return path.Clean(reqPath) == wc.matchRelPath - } - - // fallback when documentRoot is dynamic (contains placeholders) - fullPath, _ := fastabs.FastAbs(filepath.Join(documentRoot, r.URL.Path)) - - return fullPath == wc.absFileName + return opts } diff --git a/cgi.go b/cgi.go index 26312b7d15..92b8e36440 100644 --- a/cgi.go +++ b/cgi.go @@ -163,6 +163,10 @@ func addHeadersToServer(ctx context.Context, request *http.Request, trackVarsArr } func addPreparedEnvToServer(fc *frankenPHPContext, trackVarsArray *C.zval) { + for k, v := range fc.phpServer.env { + C.frankenphp_register_variable_safe(toUnsafeChar(k), toUnsafeChar(v), C.size_t(len(v)), trackVarsArray) + } + for k, v := range fc.env { C.frankenphp_register_variable_safe(toUnsafeChar(k), toUnsafeChar(v), C.size_t(len(v)), trackVarsArray) } @@ -218,7 +222,13 @@ func splitCgiPath(fc *frankenPHPContext) { // TODO: is it possible to delay this and avoid saving everything in the context? // SCRIPT_FILENAME is the absolute path of SCRIPT_NAME fc.scriptFilename = sanitizedPathJoin(fc.documentRoot, fc.scriptName) - fc.worker = workersByPath[fc.scriptFilename] + + // see if a php_server worker or global worker matches the request path + // aka: root + request path == worker.filename + fc.worker = fc.phpServer.workersByPath[fc.scriptFilename] + if fc.worker == nil { + fc.worker = globalWorkersByPath[fc.scriptFilename] + } } // splitPos returns the index where path should be split based on splitPath. diff --git a/context.go b/context.go index c8f75b98a4..9327b6c7e9 100644 --- a/context.go +++ b/context.go @@ -7,6 +7,7 @@ import ( "log/slog" "net/http" "os" + "path/filepath" "strconv" "strings" "time" @@ -16,6 +17,7 @@ import ( type frankenPHPContext struct { mercureContext + ctx context.Context documentRoot string splitPath []string env PreparedEnv @@ -29,6 +31,7 @@ type frankenPHPContext struct { scriptName string scriptFilename string requestURI string + phpServer *PhpServer // Whether the request is already closed by us isDone bool @@ -42,24 +45,6 @@ type frankenPHPContext struct { startedAt time.Time } -type contextHolder struct { - ctx context.Context - frankenPHPContext *frankenPHPContext -} - -// fromContext extracts the frankenPHPContext from a context. -func fromContext(ctx context.Context) (fctx *frankenPHPContext, ok bool) { - fctx, ok = ctx.Value(contextKey).(*frankenPHPContext) - return -} - -func newFrankenPHPContext() *frankenPHPContext { - return &frankenPHPContext{ - done: make(chan any), - startedAt: time.Now(), - } -} - // NewRequestWithContext creates a new FrankenPHP request context. // // FrankenPHP does not strip request headers whose name contains an underscore. @@ -71,8 +56,24 @@ func newFrankenPHPContext() *frankenPHPContext { // you explicitly need (and whitelist) them. The Caddy-based server and reverse // proxies such as nginx (underscores_in_headers off) already do this. func NewRequestWithContext(r *http.Request, opts ...RequestOption) (*http.Request, error) { - fc := newFrankenPHPContext() - fc.request = r + c := context.WithValue(r.Context(), contextKey, opts) + + return r.WithContext(c), nil +} + +func newContextFromRequest(request *http.Request, responseWriter http.ResponseWriter, s *PhpServer, opts ...RequestOption) (*frankenPHPContext, error) { + fc := &frankenPHPContext{ + ctx: request.Context(), + done: make(chan any), + startedAt: time.Now(), + phpServer: s, + splitPath: s.splitPath, + logger: s.logger, + request: request, + documentRoot: s.root, + responseWriter: responseWriter, + requestURI: request.URL.RequestURI(), + } for _, o := range opts { if err := o(fc); err != nil { @@ -80,6 +81,16 @@ func NewRequestWithContext(r *http.Request, opts ...RequestOption) (*http.Reques } } + // see if a worker matches the request + if fc.worker == nil { + for _, w := range s.workersWithRequestMatcher { + if w.matchRequest(request) { + fc.worker = w + break + } + } + } + if fc.logger == nil { fc.logger = globalLogger } @@ -97,30 +108,60 @@ func NewRequestWithContext(r *http.Request, opts ...RequestOption) (*http.Reques splitCgiPath(fc) - fc.requestURI = r.URL.RequestURI() - - c := context.WithValue(r.Context(), contextKey, fc) - - return r.WithContext(c), nil + return fc, nil } -// newDummyContext creates a fake context from a request path -func newDummyContext(requestPath string, opts ...RequestOption) (*frankenPHPContext, error) { - r, err := http.NewRequestWithContext(globalCtx, http.MethodGet, requestPath, nil) +// newWorkerDummyContext creates a context for worker startup +func newWorkerDummyContext(w *worker) (*frankenPHPContext, error) { + r, err := http.NewRequestWithContext(globalCtx, http.MethodGet, filepath.Base(w.fileName), nil) if err != nil { return nil, err } - fr, err := NewRequestWithContext(r, opts...) - if err != nil { - return nil, err + fc := &frankenPHPContext{ + ctx: r.Context(), + phpServer: w.phpServer, + request: r, + startedAt: time.Now(), + logger: globalLogger, + worker: w, } - fc, _ := fromContext(fr.Context()) + for _, o := range w.requestOptions { + if err := o(fc); err != nil { + return nil, err + } + } + + if fc.phpServer == nil { + fc.phpServer = newDummyPhpServer() + } + + splitCgiPath(fc) return fc, nil } +// newContextFromMessage creates a context from a message (external workers) +func newContextFromMessage(message any, rw http.ResponseWriter, ctx context.Context, w *worker) *frankenPHPContext { + fc := &frankenPHPContext{ + done: make(chan any), + startedAt: time.Now(), + phpServer: w.phpServer, + worker: w, + logger: globalLogger, + responseWriter: rw, + handlerParameters: message, + ctx: ctx, + } + + if fc.phpServer == nil { + fc.phpServer = newDummyPhpServer() + } + + return fc +} + // closeContext sends the response to the client func (fc *frankenPHPContext) closeContext() { if fc.isDone { @@ -154,12 +195,12 @@ func (fc *frankenPHPContext) validate() error { } func (fc *frankenPHPContext) clientHasClosed() bool { - if fc.request == nil { + if fc.ctx == nil { return false } select { - case <-fc.request.Context().Done(): + case <-fc.ctx.Done(): return true default: return false diff --git a/frankenphp.go b/frankenphp.go index 319f00a30c..90fcca3e67 100644 --- a/frankenphp.go +++ b/frankenphp.go @@ -284,6 +284,11 @@ func Init(options ...Option) error { maxIdleTime = opt.maxIdleTime } + // append all module workers to the global workers for registration + for _, phpServer := range PhpServers { + opt.workers = append(opt.workers, phpServer.workerOpts...) + } + workerThreadCount, err := calculateMaxThreads(opt) if err != nil { Shutdown() @@ -321,7 +326,7 @@ func Init(options ...Option) error { // reused across reloads so queued requests aren't orphaned on a stale channel if regularRequestChan == nil { - regularRequestChan = make(chan contextHolder) + regularRequestChan = make(chan *frankenPHPContext) } regularThreads = make([]*phpThread, 0, opt.numThreads-workerThreadCount) for i := 0; i < opt.numThreads-workerThreadCount; i++ { @@ -376,6 +381,7 @@ func Shutdown() { drainWatchers() drainPHPThreads() + resetPhpServers() metrics.Shutdown() @@ -394,37 +400,16 @@ func Shutdown() { // ServeHTTP executes a PHP script according to the given context. func ServeHTTP(responseWriter http.ResponseWriter, request *http.Request) error { - h := responseWriter.Header() - if h["Server"] == nil { - h["Server"] = serverHeader - } - - if !isRunning { - return ErrNotRunning - } - ctx := request.Context() - fc, ok := fromContext(ctx) - - ch := contextHolder{ctx, fc} + opts, ok := ctx.Value(contextKey).([]RequestOption) if !ok { return ErrInvalidRequest } - fc.responseWriter = responseWriter - - if err := fc.validate(); err != nil { - return err - } - - // Detect if a worker is available to handle this request - if fc.worker != nil { - return fc.worker.handleRequest(ch) - } + phpServer := newDummyPhpServer() - // If no worker was available, send the request to non-worker threads - return handleRequestWithRegularPHPThreads(ch) + return phpServer.ServeHTTP(responseWriter, request, opts...) } //export go_ub_write @@ -796,7 +781,7 @@ func resetGlobals() { globalLogger = slog.Default() workers = nil workersByName = nil - workersByPath = nil + globalWorkersByPath = nil watcherIsEnabled = false maxIdleTime = defaultMaxIdleTime maxRequestsPerThread = 0 diff --git a/frankenphp_test.go b/frankenphp_test.go index 4872893cf2..b7a7ff1f91 100644 --- a/frankenphp_test.go +++ b/frankenphp_test.go @@ -37,6 +37,8 @@ import ( "github.com/stretchr/testify/require" ) +var testDataDir = "" + type testOptions struct { workerScript string watch []string @@ -148,6 +150,9 @@ func TestMain(m *testing.M) { os.Exit(1) } + cwd, _ := os.Getwd() + testDataDir = cwd + strings.Clone("/testdata/") + os.Exit(m.Run()) } @@ -230,8 +235,6 @@ func TestPathInfo_worker(t *testing.T) { testPathInfo(t, &testOptions{workerScript: "server-variable.php"}) } func testPathInfo(t *testing.T, opts *testOptions) { - cwd, _ := os.Getwd() - testDataDir := cwd + strings.Clone("/testdata/") path := strings.Clone("/server-variable.php/pathinfo") runTest(t, func(_ func(http.ResponseWriter, *http.Request), _ *httptest.Server, i int) { diff --git a/options.go b/options.go index a9cd2a2630..9ad9351b2d 100644 --- a/options.go +++ b/options.go @@ -4,7 +4,11 @@ import ( "context" "fmt" "log/slog" + "net/http" + "path/filepath" "time" + + "github.com/dunglas/frankenphp/internal/fastabs" ) // defaultMaxConsecutiveFailures is the default maximum number of consecutive failures before panicking @@ -16,6 +20,9 @@ type Option func(h *opt) error // WorkerOption instances allow configuring FrankenPHP worker. type WorkerOption func(*workerOpt) error +// PhpServerOption instances allow to configure a PhpServer. +type PhpServerOption func(*PhpServer) error + // opt contains the available options. // // If you change this, also update the Caddy module and the documentation. @@ -44,8 +51,10 @@ type workerOpt struct { env PreparedEnv requestOptions []RequestOption watch []string + matchRequest func(*http.Request) bool maxConsecutiveFailures int extensionWorkers *extensionWorkers + phpServer *PhpServer onThreadReady func(int) onThreadShutdown func(int) onServerStartup func() @@ -149,6 +158,17 @@ func WithPhpIni(overrides map[string]string) Option { } } +// WithPhpServer configures a PHP server. +func WithPhpServer(idx int, opts ...PhpServerOption) Option { + return func(o *opt) error { + _, err := newPhpServer(idx, opts...) + if err != nil { + return err + } + return nil + } +} + // WithMaxWaitTime configures the max time a request may be stalled waiting for a thread. func WithMaxWaitTime(maxWaitTime time.Duration) Option { return func(o *opt) error { @@ -212,6 +232,16 @@ func WithWorkerWatchMode(watch []string) WorkerOption { } } +// WithWorkerMatchOn sets a request matcher for this worker +// if the matcher returns true, the worker will be used to handle the request +// if no request matcher is set, matching happens only by path (filename == root + request path) +func WithWorkerMatchOn(matcherFunc func(*http.Request) bool) WorkerOption { + return func(w *workerOpt) error { + w.matchRequest = matcherFunc + return nil + } +} + // WithWorkerMaxFailures sets the maximum number of consecutive failures before panicking func WithWorkerMaxFailures(maxFailures int) WorkerOption { return func(w *workerOpt) error { @@ -265,3 +295,73 @@ func withExtensionWorkers(w *extensionWorkers) WorkerOption { return nil } } + +func WithPhpServerRoot(root string, resolveSymlink bool) PhpServerOption { + return func(s *PhpServer) error { + root, err := fastabs.FastAbs(root) + if err != nil { + return err + } + + if resolveSymlink { + if root, err = filepath.EvalSymlinks(root); err != nil { + return err + } + } + + s.root = root + return nil + } +} + +func WithPhpServerSplitPath(splitPath []string) PhpServerOption { + return func(s *PhpServer) error { + if err := normalizeSplitPath(splitPath); err != nil { + return err + } + s.splitPath = splitPath + + return nil + } +} + +func WithPhpServerEnv(env map[string]string) PhpServerOption { + return func(s *PhpServer) error { + s.env = PrepareEnv(env) + + return nil + } +} + +// WithPhpServerWorker configures the PHP workers to start for a specific php server +func WithPhpServerWorker(name, fileName string, num int, options ...WorkerOption) PhpServerOption { + return func(s *PhpServer) error { + workerOpt := workerOpt{ + name: name, + fileName: fileName, + num: num, + env: PrepareEnv(nil), + watch: []string{}, + maxConsecutiveFailures: defaultMaxConsecutiveFailures, + phpServer: s, + } + + for _, option := range options { + if err := option(&workerOpt); err != nil { + return err + } + } + + s.workerOpts = append(s.workerOpts, workerOpt) + + return nil + } +} + +func WithPHPServerLogger(logger *slog.Logger) PhpServerOption { + return func(s *PhpServer) error { + s.logger = logger + + return nil + } +} diff --git a/phpmainthread_test.go b/phpmainthread_test.go index 8ee5746505..0158425e31 100644 --- a/phpmainthread_test.go +++ b/phpmainthread_test.go @@ -92,7 +92,7 @@ func TestTransitionAThreadBetween2DifferentWorkers(t *testing.T) { // try all possible handler transitions // takes around 200ms and is supposed to force race conditions func TestTransitionThreadsWhileDoingRequests(t *testing.T) { - t.Cleanup(Shutdown) + setupGlobals(t) var ( isDone atomic.Bool @@ -236,8 +236,10 @@ func TestFinishBootingAWorkerScript(t *testing.T) { convertToWorkerThread(phpThreads[0], worker) phpThreads[0].state.WaitFor(state.Ready) - assert.NotNil(t, phpThreads[0].handler.(*workerThread).dummyContext) - assert.Nil(t, phpThreads[0].handler.(*workerThread).workerContext) + dummyFC := phpThreads[0].handler.(*workerThread).dummyFrankenPHPContext + assert.NotNil(t, dummyFC) + assert.NotNil(t, dummyFC.ctx) + assert.Nil(t, phpThreads[0].handler.(*workerThread).workerFrankenPHPContext) assert.False( t, phpThreads[0].handler.(*workerThread).isBootingScript, @@ -249,27 +251,27 @@ func TestFinishBootingAWorkerScript(t *testing.T) { } func TestReturnAnErrorIf2WorkersHaveTheSameFileName(t *testing.T) { + resetGlobals() workers = []*worker{} workersByName = map[string]*worker{} - workersByPath = map[string]*worker{} + globalWorkersByPath = map[string]*worker{} w, err1 := newWorker(workerOpt{fileName: testDataPath + "/index.php"}) assert.NoError(t, err1) workers = append(workers, w) workersByName[w.name] = w - workersByPath[w.fileName] = w + globalWorkersByPath[w.fileName] = w _, err2 := newWorker(workerOpt{fileName: testDataPath + "/index.php"}) assert.Error(t, err2, "two workers cannot have the same filename") } func TestReturnAnErrorIf2ModuleWorkersHaveTheSameName(t *testing.T) { + resetGlobals() workers = []*worker{} workersByName = map[string]*worker{} - workersByPath = map[string]*worker{} w, err1 := newWorker(workerOpt{fileName: testDataPath + "/index.php", name: "workername"}) assert.NoError(t, err1) workers = append(workers, w) workersByName[w.name] = w - workersByPath[w.fileName] = w _, err2 := newWorker(workerOpt{fileName: testDataPath + "/hello.php", name: "workername"}) assert.Error(t, err2, "two workers cannot have the same name") } @@ -313,9 +315,9 @@ func allPossibleTransitions(worker1Path string, worker2Path string) []func(*phpT thread.boot() } }, - func(thread *phpThread) { convertToWorkerThread(thread, workersByPath[worker1Path]) }, + func(thread *phpThread) { convertToWorkerThread(thread, globalWorkersByPath[worker1Path]) }, convertToInactiveThread, - func(thread *phpThread) { convertToWorkerThread(thread, workersByPath[worker2Path]) }, + func(thread *phpThread) { convertToWorkerThread(thread, globalWorkersByPath[worker2Path]) }, convertToInactiveThread, } } diff --git a/phpserver.go b/phpserver.go new file mode 100644 index 0000000000..71d3a8215b --- /dev/null +++ b/phpserver.go @@ -0,0 +1,108 @@ +package frankenphp + +import ( + "fmt" + "log/slog" + "net/http" +) + +// PhpServer represents a php_server block in the caddyfile. +// can also be used to scope workers to a specific set of configurations. +type PhpServer struct { + idx int + root string + splitPath []string + env PreparedEnv + workers []*worker + workersByPath map[string]*worker + workersWithRequestMatcher []*worker + workerOpts []workerOpt + logger *slog.Logger +} + +// PhpServers is a map of all registered PhpServer instances. +// access this map only between frankenphp.Init() and frankenphp.Shutdown() calls (so it can be kept lock-free) +var PhpServers = make(map[int]*PhpServer) + +func resetPhpServers() { + PhpServers = make(map[int]*PhpServer) +} + +func newPhpServer(idx int, opts ...PhpServerOption) (*PhpServer, error) { + existingPhpServer, ok := PhpServers[idx] + if ok { + globalLogger.Debug("php server already registered, ignoring duplicate registration", "idx", idx) + return existingPhpServer, nil + } + + phpServer := &PhpServer{ + idx: idx, + env: make(map[string]string), + workersByPath: make(map[string]*worker), + workerOpts: make([]workerOpt, 0), + } + + for _, option := range opts { + if err := option(phpServer); err != nil { + return phpServer, err + } + } + + PhpServers[phpServer.idx] = phpServer + + return phpServer, nil +} + +// fallback PHP server if none could be associated with a request +func newDummyPhpServer() *PhpServer { + return &PhpServer{ + idx: -1, + workersByPath: make(map[string]*worker), + env: make(map[string]string), + } +} + +func (s *PhpServer) addWorker(w *worker) error { + w.phpServer.workers = append(w.phpServer.workers, w) + if w.matchRequest != nil { + w.phpServer.workersWithRequestMatcher = append(w.phpServer.workersWithRequestMatcher, w) + return nil + } + + if _, exists := w.phpServer.workersByPath[w.fileName]; exists { + return fmt.Errorf("two workers in a php_server cannot have the same filename: %q", w.fileName) + } + w.phpServer.workersByPath[w.fileName] = w + + return nil +} + +// ServeHTTP executes a PHP script according to the given context. +// the request will be scoped to the PhpServer instance. +func (s *PhpServer) ServeHTTP(responseWriter http.ResponseWriter, request *http.Request, opts ...RequestOption) error { + h := responseWriter.Header() + if h["Server"] == nil { + h["Server"] = serverHeader + } + + if !isRunning { + return ErrNotRunning + } + + fc, err := newContextFromRequest(request, responseWriter, s, opts...) + if err != nil { + return err + } + + if err := fc.validate(); err != nil { + return err + } + + // Detect if a worker is available to handle this request + if fc.worker != nil { + return fc.worker.handleRequest(fc) + } + + // If no worker was available, send the request to non-worker threads + return handleRequestWithRegularPHPThreads(fc) +} diff --git a/phpserver_test.go b/phpserver_test.go new file mode 100644 index 0000000000..ab089256a9 --- /dev/null +++ b/phpserver_test.go @@ -0,0 +1,220 @@ +package frankenphp_test + +import ( + "errors" + "io" + "net/http" + "net/http/httptest" + "path/filepath" + "strings" + "testing" + + "github.com/dunglas/frankenphp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func initPhpServer(t *testing.T, opts ...frankenphp.Option) { + t.Helper() + t.Cleanup(frankenphp.Shutdown) + require.NoError(t, frankenphp.Init(opts...)) +} + +func initPhpServerWithOptions(t *testing.T, idx int, serverOpts ...frankenphp.PhpServerOption) *frankenphp.PhpServer { + t.Helper() + + opts := append([]frankenphp.PhpServerOption{ + frankenphp.WithPhpServerRoot(testDataDir, false), + }, serverOpts...) + + initPhpServer(t, frankenphp.WithPhpServer(idx, opts...)) + + return frankenphp.PhpServers[idx] +} + +func phpServerRequest(t *testing.T, server *frankenphp.PhpServer, req *http.Request) (string, *http.Response) { + t.Helper() + + w := httptest.NewRecorder() + err := server.ServeHTTP(w, req) + if err != nil { + var rejected frankenphp.ErrRejected + if !errors.As(err, &rejected) { + require.NoError(t, err) + } + } + + resp := w.Result() + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + + return string(body), resp +} + +func phpServerGet(t *testing.T, server *frankenphp.PhpServer, url string) (string, *http.Response) { + t.Helper() + + return phpServerRequest(t, server, httptest.NewRequest(http.MethodGet, url, nil)) +} + +func TestPhpServer(t *testing.T) { + t.Run("idx", func(t *testing.T) { + initPhpServer(t, + frankenphp.WithPhpServer(1, + frankenphp.WithPhpServerRoot(testDataDir, false), + frankenphp.WithPhpServerEnv(map[string]string{"PHP_SERVER_IDX": "1"}), + ), + frankenphp.WithPhpServer(2, + frankenphp.WithPhpServerRoot(testDataDir, false), + frankenphp.WithPhpServerEnv(map[string]string{"PHP_SERVER_IDX": "2"}), + ), + ) + + body1, _ := phpServerGet(t, frankenphp.PhpServers[1], "http://example.com/server-variable.php") + body2, _ := phpServerGet(t, frankenphp.PhpServers[2], "http://example.com/server-variable.php") + + assert.Contains(t, body1, "[PHP_SERVER_IDX] => 1") + assert.Contains(t, body2, "[PHP_SERVER_IDX] => 2") + assert.NotContains(t, body1, "[PHP_SERVER_IDX] => 2") + assert.NotContains(t, body2, "[PHP_SERVER_IDX] => 1") + }) + + t.Run("root", func(t *testing.T) { + server := initPhpServerWithOptions(t, 1) + + body, _ := phpServerGet(t, server, "http://example.com/server-globals.php") + + expectedRoot := filepath.Clean(strings.TrimSuffix(testDataDir, string(filepath.Separator))) + assert.Contains(t, body, "DOCUMENT_ROOT: "+expectedRoot+"\n") + }) + + t.Run("env", func(t *testing.T) { + server := initPhpServerWithOptions(t, 1, frankenphp.WithPhpServerEnv(map[string]string{ + "PHP_SERVER_TEST_KEY": "from_php_server", + })) + + body, _ := phpServerGet(t, server, "http://example.com/server-variable.php") + + assert.Contains(t, body, "[PHP_SERVER_TEST_KEY] => from_php_server") + }) + + t.Run("split_path", func(t *testing.T) { + server := initPhpServerWithOptions(t, 1, frankenphp.WithPhpServerSplitPath([]string{".custom"})) + + body, _ := phpServerGet(t, server, "http://example.com/split-path.custom/pathinfo") + + assert.Contains(t, body, "PATH_INFO: /pathinfo\n") + assert.Contains(t, body, "SCRIPT_NAME: /split-path.custom\n") + assert.Contains(t, body, "PHP_SELF: /split-path.custom/pathinfo\n") + }) + + t.Run("logger", func(t *testing.T) { + logger, buf := newTestLogger(t) + + initPhpServer(t, frankenphp.WithPhpServer(1, + frankenphp.WithPhpServerRoot(testDataDir, false), + frankenphp.WithPHPServerLogger(logger), + frankenphp.WithPhpServerWorker("test", testDataDir+"/index.php", 1), + )) + + _, _ = phpServerGet(t, frankenphp.PhpServers[1], "http://example.com/index.php") + + assert.Contains(t, buf.String(), "request handling started", "should contain the debug message from worker start") + }) + + t.Run("workers_by_path_and_request_matcher", func(t *testing.T) { + initPhpServer( + t, + frankenphp.WithPhpServer(1, + frankenphp.WithPhpServerRoot(testDataDir, false), + frankenphp.WithPhpServerWorker("counter", testDataDir+"worker-with-counter.php", 1), + ), + frankenphp.WithPhpServer(2, + frankenphp.WithPhpServerRoot(testDataDir, false), + frankenphp.WithPhpServerWorker("match", testDataDir+"worker-with-counter.php", 1, + frankenphp.WithWorkerMatchOn(func(r *http.Request) bool { + return strings.HasPrefix(r.URL.Path, "/match/") + }), + ), + ), + ) + + body1, _ := phpServerGet(t, frankenphp.PhpServers[1], "http://example.com/worker-with-counter.php") + body2, _ := phpServerGet(t, frankenphp.PhpServers[1], "http://example.com/worker-with-counter.php") + body3, _ := phpServerGet(t, frankenphp.PhpServers[2], "http://example.com/match/anything") + body4, _ := phpServerGet(t, frankenphp.PhpServers[2], "http://example.com/match/anything") + body5, _ := phpServerGet(t, frankenphp.PhpServers[2], "http://example.com/index.php") + + assert.Equal(t, "requests:1", body1, "should contain the counter for the first worker") + assert.Equal(t, "requests:2", body2, "should contain the counter for the first worker") + assert.Equal(t, "requests:1", body3, "should contain the counter for the second worker") + assert.Equal(t, "requests:2", body4, "should contain the counter for the second worker") + assert.Contains(t, body5, "I am by birth a Genevese (i not set)", "should fall back to (non-worker) index.php") + }) + + t.Run("disallow_duplicate_worker_filenames_in_php_server", func(t *testing.T) { + initPhpServer(t, frankenphp.WithPhpServer(1, + frankenphp.WithPhpServerRoot(testDataDir, false), + frankenphp.WithPhpServerEnv(map[string]string{"APP_ENV": "staging"}), + frankenphp.WithPhpServerWorker("env", testDataDir+"worker-with-env.php", 1), + )) + + body, _ := phpServerGet(t, frankenphp.PhpServers[1], "http://example.com/worker-with-env.php") + + assert.Equal(t, "Worker has APP_ENV=staging", body) + }) + + t.Run("duplicate_worker_filenames_in_php_server", func(t *testing.T) { + t.Cleanup(frankenphp.Shutdown) + + err := frankenphp.Init( + frankenphp.WithPhpServer(1, + frankenphp.WithPhpServerRoot(testDataDir, false), + frankenphp.WithPhpServerWorker("worker1", testDataDir+"worker-with-counter.php", 1), + frankenphp.WithPhpServerWorker("worker2", testDataDir+"worker-with-counter.php", 1), + ), + ) + + assert.Error(t, err) + assert.Contains(t, err.Error(), "two workers in a php_server cannot have the same filename") + }) + + t.Run("duplicate_registration", func(t *testing.T) { + initPhpServer(t, + frankenphp.WithPhpServer(1, + frankenphp.WithPhpServerRoot(testDataDir, false), + frankenphp.WithPhpServerEnv(map[string]string{"PHP_SERVER_IDX": "first"}), + ), + frankenphp.WithPhpServer(1, + frankenphp.WithPhpServerRoot(testDataDir+"/other/", false), + frankenphp.WithPhpServerEnv(map[string]string{"PHP_SERVER_IDX": "second"}), + ), + ) + + body, _ := phpServerGet(t, frankenphp.PhpServers[1], "http://example.com/server-variable.php") + + assert.Contains(t, body, "[PHP_SERVER_IDX] => first") + assert.NotContains(t, body, "[PHP_SERVER_IDX] => second") + }) + + t.Run("serve_http_validation", func(t *testing.T) { + server := initPhpServerWithOptions(t, 1) + + req := httptest.NewRequest(http.MethodGet, "http://example.com/server-variable.php", nil) + req.Header.Add("Content-Length", "-1") + body, resp := phpServerRequest(t, server, req) + + assert.Equal(t, 400, resp.StatusCode) + assert.Contains(t, body, "invalid") + }) + + t.Run("not_running", func(t *testing.T) { + server := &frankenphp.PhpServer{} + req := httptest.NewRequest(http.MethodGet, "http://example.com/server-variable.php", nil) + w := httptest.NewRecorder() + + err := server.ServeHTTP(w, req) + + assert.ErrorIs(t, err, frankenphp.ErrNotRunning) + }) +} diff --git a/phpthread.go b/phpthread.go index 106ab2101b..2d053b6d89 100644 --- a/phpthread.go +++ b/phpthread.go @@ -19,7 +19,7 @@ import ( type phpThread struct { runtime.Pinner threadIndex int - requestChan chan contextHolder + requestChan chan *frankenPHPContext drainChan chan struct{} handlerMu sync.RWMutex handler threadHandler @@ -39,7 +39,6 @@ type threadHandler interface { name() string beforeScriptExecution() string afterScriptExecution(exitStatus int) - context() context.Context frankenPHPContext() *frankenPHPContext // drain is a hook called by drainWorkerThreads right before drainChan is // closed. Handlers that need to wake up a thread parked in a blocking C @@ -52,7 +51,7 @@ type threadHandler interface { func newPHPThread(threadIndex int) *phpThread { return &phpThread{ threadIndex: threadIndex, - requestChan: make(chan contextHolder), + requestChan: make(chan *frankenPHPContext), state: state.NewThreadState(), } } @@ -182,12 +181,11 @@ func (thread *phpThread) frankenPHPContext() *frankenPHPContext { } func (thread *phpThread) context() context.Context { - if thread.handler == nil { - // handler can be nil when using opcache.preload - return globalCtx + if fc := thread.frankenPHPContext(); fc != nil && fc.ctx != nil { + return fc.ctx } - return thread.handler.context() + return globalCtx } func (thread *phpThread) name() string { diff --git a/requestoptions.go b/requestoptions.go index 42cc3cf7c0..ea6863ada6 100644 --- a/requestoptions.go +++ b/requestoptions.go @@ -83,6 +83,19 @@ func WithRequestResolvedDocumentRoot(documentRoot string) RequestOption { // which can be mitigated with use of a try_files-like behavior // that 404s if the FastCGI path info is not found. func WithRequestSplitPath(splitPath []string) (RequestOption, error) { + if err := normalizeSplitPath(splitPath); err != nil { + return nil, err + } + + return func(o *frankenPHPContext) error { + o.splitPath = splitPath + + return nil + }, nil +} + +// normalize split path in-place to lowercase ASCII characters +func normalizeSplitPath(splitPath []string) error { var b strings.Builder for i, split := range splitPath { @@ -91,7 +104,7 @@ func WithRequestSplitPath(splitPath []string) (RequestOption, error) { for j := 0; j < len(split); j++ { c := split[j] if c >= utf8.RuneSelf { - return nil, ErrInvalidSplitPath + return ErrInvalidSplitPath } if 'A' <= c && c <= 'Z' { @@ -105,11 +118,7 @@ func WithRequestSplitPath(splitPath []string) (RequestOption, error) { b.Reset() } - return func(o *frankenPHPContext) error { - o.splitPath = splitPath - - return nil - }, nil + return nil } type PreparedEnv = map[string]string diff --git a/scaling.go b/scaling.go index 82f2b6c2d7..e4edc0c846 100644 --- a/scaling.go +++ b/scaling.go @@ -35,15 +35,15 @@ var ( ) func initAutoScaling(mainThread *phpMainThread) { + if mainThread.maxThreads <= mainThread.numThreads { + return + } + // reused across reloads so queued requests aren't orphaned on a stale channel if scaleChan == nil { scaleChan = make(chan *frankenPHPContext) } - if mainThread.maxThreads <= mainThread.numThreads { - return - } - done := mainThread.done mstate := mainThread.state diff --git a/scaling_test.go b/scaling_test.go index f899e7679e..d2784a8eeb 100644 --- a/scaling_test.go +++ b/scaling_test.go @@ -48,7 +48,7 @@ func TestScaleAWorkerThreadUpAndDown(t *testing.T) { autoScaledThread := phpThreads[2] // scale up - scaleWorkerThread(workersByPath[workerPath], mainThread.done, mainThread.state) + scaleWorkerThread(globalWorkersByPath[workerPath], mainThread.done, mainThread.state) assert.Equal(t, state.Ready, autoScaledThread.state.Get()) // on down-scale, the thread will be marked as inactive diff --git a/testdata/split-path.custom b/testdata/split-path.custom new file mode 100644 index 0000000000..836a9a4512 --- /dev/null +++ b/testdata/split-path.custom @@ -0,0 +1,26 @@ + 0 { + for k, v := range o.phpServer.env { + if _, exists := o.env[k]; !exists { + o.env[k] = v + } + } + } + w := &worker{ name: o.name, fileName: absFileName, + matchRequest: o.matchRequest, requestOptions: o.requestOptions, num: o.num, maxThreads: o.maxThreads, - requestChan: make(chan contextHolder), + requestChan: make(chan *frankenPHPContext), threads: make([]*phpThread, 0, o.num), - allowPathMatching: allowPathMatching, maxConsecutiveFailures: o.maxConsecutiveFailures, onThreadReady: o.onThreadReady, onThreadShutdown: o.onThreadShutdown, + phpServer: o.phpServer, } w.configureMercure(&o) @@ -222,7 +234,7 @@ func (worker *worker) isAtThreadLimit() bool { return atMaxThreads } -func (worker *worker) handleRequest(ch contextHolder) error { +func (worker *worker) handleRequest(fc *frankenPHPContext) error { metrics.StartWorkerRequest(worker.name) runtime.Gosched() @@ -232,10 +244,10 @@ func (worker *worker) handleRequest(ch contextHolder) error { worker.threadMutex.RLock() for _, thread := range worker.threads { select { - case thread.requestChan <- ch: + case thread.requestChan <- fc: worker.threadMutex.RUnlock() - <-ch.frankenPHPContext.done - metrics.StopWorkerRequest(worker.name, time.Since(ch.frankenPHPContext.startedAt)) + <-fc.done + metrics.StopWorkerRequest(worker.name, time.Since(fc.startedAt)) return nil default: @@ -256,22 +268,22 @@ func (worker *worker) handleRequest(ch contextHolder) error { } select { - case worker.requestChan <- ch: + case worker.requestChan <- fc: worker.queuedRequests.Add(-1) metrics.DequeuedWorkerRequest(worker.name) - <-ch.frankenPHPContext.done - metrics.StopWorkerRequest(worker.name, time.Since(ch.frankenPHPContext.startedAt)) + <-fc.done + metrics.StopWorkerRequest(worker.name, time.Since(fc.startedAt)) return nil - case workerScaleChan <- ch.frankenPHPContext: + case workerScaleChan <- fc: // the request has triggered scaling, continue to wait for a thread case <-timeoutChan(time.Duration(maxWaitTime.Load())): // the request has timed out stalling worker.queuedRequests.Add(-1) metrics.DequeuedWorkerRequest(worker.name) - metrics.StopWorkerRequest(worker.name, time.Since(ch.frankenPHPContext.startedAt)) + metrics.StopWorkerRequest(worker.name, time.Since(fc.startedAt)) - ch.frankenPHPContext.reject(ErrMaxWaitTimeExceeded) + fc.reject(ErrMaxWaitTimeExceeded) return ErrMaxWaitTimeExceeded } diff --git a/workerextension.go b/workerextension.go index 82b74631a7..814eea1565 100644 --- a/workerextension.go +++ b/workerextension.go @@ -45,13 +45,8 @@ func (w *extensionWorkers) NumThreads() int { // EXPERIMENTAL: SendMessage sends a message to the worker and waits for a response. func (w *extensionWorkers) SendMessage(ctx context.Context, message any, rw http.ResponseWriter) (any, error) { - fc := newFrankenPHPContext() - fc.logger = globalLogger - fc.worker = w.internalWorker - fc.responseWriter = rw - fc.handlerParameters = message - - err := w.internalWorker.handleRequest(contextHolder{context.WithValue(ctx, contextKey, fc), fc}) + fc := newContextFromMessage(message, rw, ctx, w.internalWorker) + err := w.internalWorker.handleRequest(fc) return fc.handlerReturn, err }