diff --git a/caddy/admin_test.go b/caddy/admin_test.go index 27ece031f3..4353a556f7 100644 --- a/caddy/admin_test.go +++ b/caddy/admin_test.go @@ -40,9 +40,6 @@ func TestRestartWorkerViaAdminApi(t *testing.T) { } `, "caddyfile") - // make sure workers are not still running from any previous tests - assertAdminResponse(t, tester, "POST", "workers/restart", http.StatusOK, "workers restarted successfully\n") - tester.AssertGetResponse("http://localhost:"+testPort+"/", http.StatusOK, "requests:1") tester.AssertGetResponse("http://localhost:"+testPort+"/", http.StatusOK, "requests:2") @@ -348,3 +345,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..371e329e27 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 ( @@ -64,6 +63,7 @@ type FrankenPHPApp struct { metrics frankenphp.Metrics ctx context.Context logger *slog.Logger + modules map[int]*FrankenPHPModule } var errIni = errors.New(`"php_ini" must be in the format: php_ini "" ""`) @@ -101,74 +101,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() @@ -188,16 +120,22 @@ func (f *FrankenPHPApp) Start() error { frankenphp.WithMaxRequests(f.MaxRequests), ) + // register global workers 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()...)) + } + + // register module workers that are scoped to a php_server or php block + for _, module := range f.modules { + for _, w := range module.Workers { + w.FileName = repl.ReplaceKnown(w.FileName, "") + if module.server == nil { + return fmt.Errorf("module %d has no server", module.ServerIdx) + } + workerOptions := append(w.toWorkerOptions(), frankenphp.WithWorkerServerScope(module.server)) + f.opts = append(f.opts, frankenphp.WithWorkers(w.Name, w.FileName, w.Num, workerOptions...)) + } } frankenphp.Shutdown() @@ -205,6 +143,10 @@ func (f *FrankenPHPApp) Start() error { return err } + // after startup, reset all configuration on the app instance + // this must happen here since the instance is re-used across reloads and tests + f.reset() + return nil } @@ -220,18 +162,40 @@ func (f *FrankenPHPApp) Stop() error { frankenphp.Shutdown() } - // reset the configuration so it doesn't bleed into later tests + return nil +} + +func (f *FrankenPHPApp) reset() { f.Workers = nil f.NumThreads = 0 f.MaxWaitTime = 0 f.MaxIdleTime = 0 f.MaxRequests = 0 - + f.PhpIni = nil + f.modules = nil + f.opts = nil + f.ctx = nil + f.metrics = nil optionsMU.Lock() options = nil optionsMU.Unlock() +} - return nil +// register the php_server or php block to the app instance +func (f *FrankenPHPApp) registerServer(m *FrankenPHPModule) { + if f.modules == nil { + f.modules = make(map[int]*FrankenPHPModule) + } + + server, serverOpt := frankenphp.WithServer(m.ServerIdx, m.resolvedDocumentRoot, m.SplitPath, m.resolvedEnv) + m.server = server + + // only register the server module if it's not already registered to avoid duplicate modules + // this can happen if multiple "php" directives are used in the same caddy route block (like with worker matches) + if _, ok := f.modules[m.ServerIdx]; !ok { + f.modules[m.ServerIdx] = m + f.opts = append(f.opts, serverOpt) + } } // UnmarshalCaddyfile implements caddyfile.Unmarshaler. 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..bec8b16fbd 100644 --- a/caddy/hotreload.go +++ b/caddy/hotreload.go @@ -49,7 +49,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..f4f3647a19 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" @@ -25,6 +24,7 @@ import ( // FrankenPHPModule represents the "php_server" and "php" directives in the Caddyfile // they are responsible for forwarding requests to FrankenPHP via "ServeHTTP" +// keep in mind that the module reference gets lost between Caddy parsing and Caddy provisioning // // example.com { // php_server { @@ -45,14 +45,14 @@ 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 + // ServerIdx is the idx of the php_server this module belongs to + ServerIdx int `json:"server_idx,omitempty"` + + resolvedDocumentRoot string + resolvedEnv map[string]string + requestEnv frankenphp.PreparedEnv + requestOptions []frankenphp.RequestOption + server *frankenphp.Server } // CaddyModule returns the Caddy module information. @@ -65,7 +65,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 @@ -78,9 +77,15 @@ func (f *FrankenPHPModule) Provision(ctx caddy.Context) error { return fmt.Errorf(`expected ctx.App("frankenphp") to return *FrankenPHPApp, got nil`) } + if f.ServerIdx == 0 { + // if no idx was yet assigned to this module, assign one on-the-fly + // this is possible in JSON configuration when using "php" directives in route blocks + f.ServerIdx = len(fapp.modules) + 1 + caddy.Log().Warn("\"php\" in route block is missing a \"server_idx\", assigning one on-the-fly") + } + 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 +93,20 @@ 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) + if frankenphp.EmbeddedAppPath != "" && filepath.IsLocal(wc.FileName) { + wc.FileName = filepath.Join(frankenphp.EmbeddedAppPath, wc.FileName) } - wc.requestOptions = append(wc.requestOptions, loggerOpt) - wc.routeGroup = f.RouteGroup - f.Workers[i] = wc - } + // module worker names are prefixed with m#, to distinguish them from global workers + serverPrefix := fmt.Sprintf("m%d#", f.ServerIdx) + if wc.Name == "" { + wc.Name = f.generateUniqueModuleWorkerName(wc.FileName, serverPrefix) + } else if !strings.HasPrefix(wc.Name, serverPrefix) { + wc.Name = serverPrefix + wc.Name + } - workers, err := fapp.addModuleWorkers(f.Workers...) - if err != nil { - return err + f.Workers[i] = wc } - f.Workers = workers if f.Root == "" { if frankenphp.EmbeddedAppPath == "" { @@ -116,25 +120,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,43 +144,47 @@ 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 + f.resolvedEnv = make(map[string]string) // env variables that do not need replacement + f.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) { + f.requestEnv[k+"\x00"] = e // prepare the env with a null byte + } else { + f.resolvedEnv[k] = e } } - if !f.preparedEnvNeedsReplacement { - f.requestOptions = append(f.requestOptions, frankenphp.WithRequestPreparedEnv(f.preparedEnv)) - } + fapp.registerServer(f) - if err := f.configureHotReload(fapp); err != nil { - return err + 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 nil + return name } // needReplacement checks if a string contains placeholders. @@ -204,13 +197,13 @@ func (f *FrankenPHPModule) ServeHTTP(w http.ResponseWriter, r *http.Request, _ c ctx := r.Context() repl := ctx.Value(caddy.ReplacerCtxKey).(*caddy.Replacer) - documentRoot := f.resolvedDocumentRoot - 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, "") + // if the root contains a caddy placeholder, it needs to be resolved here in the hot path + if f.resolvedDocumentRoot == "" { + documentRoot := repl.ReplaceKnown(f.Root, "") if documentRoot == "" && frankenphp.EmbeddedAppPath != "" { documentRoot = frankenphp.EmbeddedAppPath } @@ -221,37 +214,19 @@ 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)) - for k, v := range f.preparedEnv { + // all env variables that contain caddy placeholders need to be resolved here in the hot path + if len(f.requestEnv) > 0 { + env := make(frankenphp.PreparedEnv, len(f.requestEnv)) + for k, v := range f.requestEnv { env[k] = repl.ReplaceKnown(v, "") } opts = append(opts, frankenphp.WithRequestPreparedEnv(env)) } - workerName := "" - for _, w := range f.Workers { - if w.matchesPath(r, documentRoot) { - workerName = w.Name - break - } - } + err := f.server.ServeHTTP(w, r, opts...) - 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) - } - - 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 +257,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 +307,22 @@ func (f *FrankenPHPModule) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { return nil } +func (f *FrankenPHPModule) assignServerIdx(h httpcaddyfile.Helper) { + counter, _ := h.State["php_server_count"].(int) + f.ServerIdx = counter + 1 + 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.assignServerIdx(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 +355,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 +454,9 @@ func parsePhpServer(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error) return nil, err } + // assign a unique index to the php server + phpsrv.assignServerIdx(h) + if frankenphp.EmbeddedAppPath != "" { if phpsrv.Root == "" { phpsrv.Root = filepath.Join(frankenphp.EmbeddedAppPath, defaultDocumentRoot) @@ -492,8 +468,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..6cb61e37bb 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.WithWorkerMatcher(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 0fe6f2e30e..c38ce51373 100644 --- a/cgi.go +++ b/cgi.go @@ -93,13 +93,6 @@ func addKnownVariablesToServer(fc *frankenPHPContext, trackVarsArray *C.zval) { serverPort := reqPort contentLength := request.Header.Get("Content-Length") - var requestURI string - if fc.originalRequest != nil { - requestURI = fc.originalRequest.URL.RequestURI() - } else { - requestURI = fc.requestURI - } - phpSelf := fc.scriptName + fc.pathInfo C.frankenphp_register_server_vars(trackVarsArray, C.frankenphp_server_vars{ @@ -136,8 +129,8 @@ func addKnownVariablesToServer(fc *frankenPHPContext, trackVarsArray *C.zval) { server_protocol_len: C.size_t(len(request.Proto)), http_host: toUnsafeChar(request.Host), http_host_len: C.size_t(len(request.Host)), - request_uri: toUnsafeChar(requestURI), - request_uri_len: C.size_t(len(requestURI)), + request_uri: toUnsafeChar(fc.requestURI), + request_uri_len: C.size_t(len(fc.requestURI)), ssl_cipher: toUnsafeChar(sslCipher), ssl_cipher_len: C.size_t(len(sslCipher)), @@ -164,26 +157,28 @@ func addHeadersToServer(ctx context.Context, request *http.Request, trackVarsArr } } -// registerPreparedEnv exposes fc.env to getenv() before any PHP code runs. -func registerPreparedEnv(env PreparedEnv) { - size := C.size_t(len(env)) - for k, v := range env { - C.frankenphp_add_to_prepared_env(toUnsafeChar(k), C.size_t(len(k)-1), toUnsafeChar(v), C.size_t(len(v)), size) +// registerPreparedEnv exposes fc.env and fc.server.env to getenv() before any PHP code runs. +func registerPreparedEnv(fc *frankenPHPContext, preparedEnvLen int) { + for k, v := range fc.server.env { + C.frankenphp_add_to_prepared_env(toUnsafeChar(k), C.size_t(len(k)-1), toUnsafeChar(v), C.size_t(len(v)), C.size_t(preparedEnvLen)) + } + for k, v := range fc.env { + C.frankenphp_add_to_prepared_env(toUnsafeChar(k), C.size_t(len(k)-1), toUnsafeChar(v), C.size_t(len(v)), C.size_t(preparedEnvLen)) } } //export go_register_server_variables func go_register_server_variables(threadIndex C.uintptr_t, trackVarsArray *C.zval) { thread := phpThreads[threadIndex] - fc := thread.frankenPHPContext() + fc := thread.handler.frankenPHPContext() if fc.request != nil { addKnownVariablesToServer(fc, trackVarsArray) - addHeadersToServer(thread.context(), fc.request, trackVarsArray) + addHeadersToServer(fc.ctx, fc.request, trackVarsArray) } // The Prepared Environment is registered last and can overwrite any previous values - if len(fc.env) != 0 { + if len(fc.env) != 0 || len(fc.server.env) != 0 { C.frankenphp_merge_with_prepared_env(trackVarsArray) } } @@ -223,7 +218,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.server.workersByPath[fc.scriptFilename] + if fc.worker == nil { + fc.worker = globalWorkersByPath[fc.scriptFilename] + } } // splitPos returns the index where path should be split based on splitPath. @@ -285,15 +286,16 @@ func splitPos(path string, splitPath []string) int { //export go_update_request_info func go_update_request_info(threadIndex C.uintptr_t, info *C.sapi_request_info) *C.char { thread := phpThreads[threadIndex] - fc := thread.frankenPHPContext() + fc := thread.handler.frankenPHPContext() request := fc.request if request == nil { return nil } - if len(fc.env) != 0 { - registerPreparedEnv(fc.env) + preparedEnvLen := len(fc.env) + len(fc.server.env) + if preparedEnvLen != 0 { + registerPreparedEnv(fc, preparedEnvLen) } if m, ok := cStringHTTPMethods[request.Method]; ok { diff --git a/context.go b/context.go index c8f75b98a4..c4ef37d9b0 100644 --- a/context.go +++ b/context.go @@ -7,6 +7,7 @@ import ( "log/slog" "net/http" "os" + "path/filepath" "strconv" "strings" "time" @@ -16,13 +17,14 @@ import ( type frankenPHPContext struct { mercureContext - documentRoot string - splitPath []string - env PreparedEnv - logger *slog.Logger - request *http.Request - originalRequest *http.Request - worker *worker + ctx context.Context + documentRoot string + splitPath []string + env PreparedEnv + logger *slog.Logger + request *http.Request + worker *worker + server *server docURI string pathInfo string @@ -42,24 +44,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 +55,23 @@ 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 *server, opts ...RequestOption) (*frankenPHPContext, error) { + fc := &frankenPHPContext{ + ctx: request.Context(), + done: make(chan any), + startedAt: time.Now(), + server: s, + splitPath: s.splitPath, + logger: globalLogger, + request: request, + documentRoot: s.root, + responseWriter: responseWriter, + } for _, o := range opts { if err := o(fc); err != nil { @@ -80,8 +79,14 @@ func NewRequestWithContext(r *http.Request, opts ...RequestOption) (*http.Reques } } - if fc.logger == nil { - fc.logger = globalLogger + // assign a worker directly if it has a request matcher + if fc.worker == nil { + for _, w := range s.workersWithRequestMatcher { + if w.matchRequest(request) { + fc.worker = w + break + } + } } if fc.documentRoot == "" { @@ -95,32 +100,74 @@ func NewRequestWithContext(r *http.Request, opts ...RequestOption) (*http.Reques } } - splitCgiPath(fc) - - fc.requestURI = r.URL.RequestURI() + // if no originalRequest was passed, use the URI from the actual request + // when using Caddy's http module, the original unchanged uri will be used here + // request.URL is often already rewritten to match a PHP script path + if fc.requestURI == "" { + fc.requestURI = fc.request.URL.RequestURI() + } - c := context.WithValue(r.Context(), contextKey, fc) + splitCgiPath(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(), + server: w.server, + request: r, + startedAt: time.Now(), + logger: globalLogger, + worker: w, + } + + for _, o := range w.requestOptions { + if err := o(fc); err != nil { + return nil, err + } + } + + if fc.server == nil { + // global worker, not associated with a server + fc.server = fallbackServer } - fc, _ := fromContext(fr.Context()) + 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(), + server: w.server, + worker: w, + logger: globalLogger, + 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 func (fc *frankenPHPContext) closeContext() { if fc.isDone { @@ -155,11 +202,11 @@ func (fc *frankenPHPContext) validate() error { func (fc *frankenPHPContext) clientHasClosed() bool { if fc.request == nil { - return false + return false // not in HTTP context } select { - case <-fc.request.Context().Done(): + case <-fc.ctx.Done(): return true default: return false diff --git a/debugstate.go b/debugstate.go index 62f3f9f7a0..ea9f3e364f 100644 --- a/debugstate.go +++ b/debugstate.go @@ -80,13 +80,8 @@ func threadDebugState(thread *phpThread) ThreadDebugState { return s } - if fc.originalRequest == nil { - s.CurrentURI = fc.requestURI - s.CurrentMethod = fc.request.Method - } else { - s.CurrentURI = fc.originalRequest.URL.RequestURI() - s.CurrentMethod = fc.originalRequest.Method - } + s.CurrentURI = fc.requestURI + s.CurrentMethod = fc.request.Method if !fc.startedAt.IsZero() { s.RequestStartedAt = fc.startedAt.UnixMilli() diff --git a/frankenphp.go b/frankenphp.go index 319f00a30c..ad3193f119 100644 --- a/frankenphp.go +++ b/frankenphp.go @@ -49,6 +49,8 @@ var ( ErrMainThreadCreation = errors.New("error creating the main thread") ErrScriptExecution = errors.New("error during PHP script execution") ErrNotRunning = errors.New("FrankenPHP is not running. For proper configuration visit: https://frankenphp.dev/docs/config/#caddyfile-config") + ErrServerNotFound = errors.New("server not found") + ErrAlreadyRegistered = errors.New("server already registered") ErrInvalidRequestPath = ErrRejected{"invalid request path", http.StatusBadRequest} ErrInvalidContentLengthHeader = ErrRejected{"invalid Content-Length header", http.StatusBadRequest} @@ -321,7 +323,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 +378,7 @@ func Shutdown() { drainWatchers() drainPHPThreads() + resetServers() metrics.Shutdown() @@ -394,43 +397,24 @@ 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) - } - - // If no worker was available, send the request to non-worker threads - return handleRequestWithRegularPHPThreads(ch) + return fallbackServer.serveHTTP(responseWriter, request, opts...) } //export go_ub_write func go_ub_write(threadIndex C.uintptr_t, cBuf *C.char, length C.size_t) (C.size_t, C.bool) { thread := phpThreads[threadIndex] - fc := thread.frankenPHPContext() + fc := thread.handler.frankenPHPContext() if fc.isDone { return 0, C.bool(true) @@ -445,27 +429,14 @@ func go_ub_write(threadIndex C.uintptr_t, cBuf *C.char, length C.size_t) (C.size writer = fc.responseWriter } - var ctx context.Context - i, e := writer.Write(unsafe.Slice((*byte)(unsafe.Pointer(cBuf)), length)) - if e != nil { - ctx = thread.context() - - if fc.logger.Enabled(ctx, slog.LevelWarn) { - fc.logger.LogAttrs(ctx, slog.LevelWarn, "write error", slog.Any("error", e)) - } + if e != nil && fc.logger.Enabled(fc.ctx, slog.LevelWarn) { + fc.logger.LogAttrs(fc.ctx, slog.LevelWarn, "write error", slog.Any("error", e)) } - if fc.responseWriter == nil { + if fc.responseWriter == nil && fc.logger.Enabled(fc.ctx, slog.LevelInfo) { // probably starting a worker script, log the output - - if ctx == nil { - ctx = thread.context() - } - - if fc.logger.Enabled(ctx, slog.LevelInfo) { - fc.logger.LogAttrs(ctx, slog.LevelInfo, writer.(*bytes.Buffer).String()) - } + fc.logger.LogAttrs(fc.ctx, slog.LevelInfo, writer.(*bytes.Buffer).String()) } return C.size_t(i), C.bool(fc.clientHasClosed()) @@ -474,14 +445,13 @@ func go_ub_write(threadIndex C.uintptr_t, cBuf *C.char, length C.size_t) (C.size //export go_apache_request_headers func go_apache_request_headers(threadIndex C.uintptr_t) (*C.go_string, C.size_t) { thread := phpThreads[threadIndex] - ctx := thread.context() - fc := thread.frankenPHPContext() + fc := thread.handler.frankenPHPContext() if fc.responseWriter == nil { // worker mode, not handling a request - if globalLogger.Enabled(ctx, slog.LevelDebug) { - globalLogger.LogAttrs(ctx, slog.LevelDebug, "apache_request_headers() called in non-HTTP context", slog.String("worker", fc.worker.name)) + if fc.logger.Enabled(fc.ctx, slog.LevelDebug) { + fc.logger.LogAttrs(fc.ctx, slog.LevelDebug, "apache_request_headers() called in non-HTTP context", slog.String("worker", fc.worker.name)) } return nil, 0 @@ -510,11 +480,11 @@ func go_apache_request_headers(threadIndex C.uintptr_t) (*C.go_string, C.size_t) return sd, C.size_t(len(fc.request.Header)) } -func addHeader(ctx context.Context, fc *frankenPHPContext, h *C.sapi_header_struct) { +func addHeader(fc *frankenPHPContext, h *C.sapi_header_struct) { key, val := splitRawHeader(h.header, int(h.header_len)) if key == "" { - if fc.logger.Enabled(ctx, slog.LevelDebug) { - fc.logger.LogAttrs(ctx, slog.LevelDebug, "invalid header", slog.String("header", C.GoStringN(h.header, C.int(h.header_len)))) + if fc.logger.Enabled(fc.ctx, slog.LevelDebug) { + fc.logger.LogAttrs(fc.ctx, slog.LevelDebug, "invalid header", slog.String("header", C.GoStringN(h.header, C.int(h.header_len)))) } return @@ -556,7 +526,7 @@ func splitRawHeader(rawHeader *C.char, length int) (string, string) { //export go_write_headers func go_write_headers(threadIndex C.uintptr_t, status C.int, headers *C.zend_llist) C.bool { thread := phpThreads[threadIndex] - fc := thread.frankenPHPContext() + fc := thread.handler.frankenPHPContext() if fc == nil { return C.bool(false) } @@ -574,7 +544,7 @@ func go_write_headers(threadIndex C.uintptr_t, status C.int, headers *C.zend_lli for current != nil { h := (*C.sapi_header_struct)(unsafe.Pointer(&(current.data))) - addHeader(thread.context(), fc, h) + addHeader(fc, h) current = current.next } @@ -583,10 +553,9 @@ func go_write_headers(threadIndex C.uintptr_t, status C.int, headers *C.zend_lli // go panics on invalid status code // https://github.com/golang/go/blob/9b8742f2e79438b9442afa4c0a0139d3937ea33f/src/net/http/server.go#L1162 if goStatus < 100 || goStatus > 999 { - ctx := thread.context() - if globalLogger.Enabled(ctx, slog.LevelWarn) { - globalLogger.LogAttrs(ctx, slog.LevelWarn, "Invalid response status code", slog.Int("status_code", goStatus)) + if fc.logger.Enabled(fc.ctx, slog.LevelWarn) { + fc.logger.LogAttrs(fc.ctx, slog.LevelWarn, "Invalid response status code", slog.Int("status_code", goStatus)) } goStatus = 500 @@ -608,7 +577,7 @@ func go_write_headers(threadIndex C.uintptr_t, status C.int, headers *C.zend_lli //export go_sapi_flush func go_sapi_flush(threadIndex C.uintptr_t) bool { thread := phpThreads[threadIndex] - fc := thread.frankenPHPContext() + fc := thread.handler.frankenPHPContext() if fc == nil { return false } @@ -625,10 +594,8 @@ func go_sapi_flush(threadIndex C.uintptr_t) bool { fc.responseController = http.NewResponseController(fc.responseWriter) } if err := fc.responseController.Flush(); err != nil { - ctx := thread.context() - - if globalLogger.Enabled(ctx, slog.LevelWarn) { - globalLogger.LogAttrs(ctx, slog.LevelWarn, "the current responseWriter is not a flusher, if you are not using a custom build, please report this issue", slog.Any("error", err)) + if fc.logger.Enabled(fc.ctx, slog.LevelWarn) { + fc.logger.LogAttrs(fc.ctx, slog.LevelWarn, "the current responseWriter is not a flusher, if you are not using a custom build, please report this issue", slog.Any("error", err)) } } @@ -637,7 +604,7 @@ func go_sapi_flush(threadIndex C.uintptr_t) bool { //export go_read_post func go_read_post(threadIndex C.uintptr_t, cBuf *C.char, countBytes C.size_t) (readBytes C.size_t) { - fc := phpThreads[threadIndex].frankenPHPContext() + fc := phpThreads[threadIndex].handler.frankenPHPContext() if fc.responseWriter == nil { return 0 @@ -656,7 +623,7 @@ func go_read_post(threadIndex C.uintptr_t, cBuf *C.char, countBytes C.size_t) (r //export go_read_cookies func go_read_cookies(threadIndex C.uintptr_t) *C.char { - request := phpThreads[threadIndex].frankenPHPContext().request + request := phpThreads[threadIndex].handler.frankenPHPContext().request if request == nil { return nil } @@ -673,23 +640,24 @@ func go_read_cookies(threadIndex C.uintptr_t) *C.char { return C.CString(cookie) } +// getLogger returns the logger and context savely even if phpThreads have not been created yet func getLogger(threadIndex C.uintptr_t) (*slog.Logger, context.Context) { - ctxHolder := phpThreads[threadIndex] - if ctxHolder == nil { + if threadIndex >= C.uintptr_t(len(phpThreads)) { return globalLogger, globalCtx } - ctx := ctxHolder.context() - if ctxHolder.handler == nil { - return globalLogger, ctx + thread := phpThreads[threadIndex] + if thread == nil || thread.handler == nil { + return globalLogger, globalCtx } - fCtx := ctxHolder.frankenPHPContext() - if fCtx == nil || fCtx.logger == nil { - return globalLogger, ctx + fc := thread.handler.frankenPHPContext() + if fc == nil { + return globalLogger, globalCtx } - return fCtx.logger, ctx + // logger and context must always be defined on fc + return fc.logger, fc.ctx } //export go_log @@ -757,7 +725,7 @@ func mapToAttr(input map[string]any) []slog.Attr { //export go_is_context_done func go_is_context_done(threadIndex C.uintptr_t) C.bool { - return C.bool(phpThreads[threadIndex].frankenPHPContext().isDone) + return C.bool(phpThreads[threadIndex].handler.frankenPHPContext().isDone) } //export go_schedule_opcache_reset @@ -796,7 +764,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 409e644634..f9bee9e94e 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/mercure.go b/mercure.go index d7cf33609e..821b057915 100644 --- a/mercure.go +++ b/mercure.go @@ -20,12 +20,11 @@ type mercureContext struct { //export go_mercure_publish func go_mercure_publish(threadIndex C.uintptr_t, topics *C.struct__zval_struct, data *C.zend_string, private bool, id, typ *C.zend_string, retry uint64) (generatedID *C.zend_string, error C.short) { thread := phpThreads[threadIndex] - ctx := thread.context() - fc := thread.frankenPHPContext() + fc := thread.handler.frankenPHPContext() if fc.mercureHub == nil { - if fc.logger.Enabled(ctx, slog.LevelError) { - fc.logger.LogAttrs(ctx, slog.LevelError, "No Mercure hub configured") + if fc.logger.Enabled(fc.ctx, slog.LevelError) { + fc.logger.LogAttrs(fc.ctx, slog.LevelError, "No Mercure hub configured") } return nil, 1 @@ -39,7 +38,7 @@ func go_mercure_publish(threadIndex C.uintptr_t, topics *C.struct__zval_struct, Type: GoString(unsafe.Pointer(typ)), }, Private: private, - Debug: fc.logger.Enabled(ctx, slog.LevelDebug), + Debug: fc.logger.Enabled(fc.ctx, slog.LevelDebug), } zvalType := C.zval_get_type(topics) @@ -49,8 +48,8 @@ func go_mercure_publish(threadIndex C.uintptr_t, topics *C.struct__zval_struct, case C.IS_ARRAY: ts, err := GoPackedArray[string](unsafe.Pointer(*(**C.zend_array)(unsafe.Pointer(&topics.value[0])))) if err != nil { - if fc.logger.Enabled(ctx, slog.LevelError) { - fc.logger.LogAttrs(ctx, slog.LevelError, "invalid topics type", slog.Any("error", err)) + if fc.logger.Enabled(fc.ctx, slog.LevelError) { + fc.logger.LogAttrs(fc.ctx, slog.LevelError, "invalid topics type", slog.Any("error", err)) } return nil, 1 @@ -62,9 +61,9 @@ func go_mercure_publish(threadIndex C.uintptr_t, topics *C.struct__zval_struct, panic("invalid topics type") } - if err := fc.mercureHub.Publish(ctx, u); err != nil { - if fc.logger.Enabled(ctx, slog.LevelError) { - fc.logger.LogAttrs(ctx, slog.LevelError, "Unable to publish Mercure update", slog.Any("error", err)) + if err := fc.mercureHub.Publish(fc.ctx, u); err != nil { + if fc.logger.Enabled(fc.ctx, slog.LevelError) { + fc.logger.LogAttrs(fc.ctx, slog.LevelError, "Unable to publish Mercure update", slog.Any("error", err)) } return nil, 2 diff --git a/options.go b/options.go index a9cd2a2630..1a869e0dae 100644 --- a/options.go +++ b/options.go @@ -4,7 +4,10 @@ import ( "context" "fmt" "log/slog" + "net/http" "time" + + "github.com/dunglas/frankenphp/internal/fastabs" ) // defaultMaxConsecutiveFailures is the default maximum number of consecutive failures before panicking @@ -44,12 +47,14 @@ type workerOpt struct { env PreparedEnv requestOptions []RequestOption watch []string + matchRequest func(*http.Request) bool maxConsecutiveFailures int extensionWorkers *extensionWorkers onThreadReady func(int) onThreadShutdown func(int) onServerStartup func() onServerShutdown func() + server *Server } // WithContext sets the main context to use. @@ -212,6 +217,26 @@ func WithWorkerWatchMode(watch []string) WorkerOption { } } +// WithWorkerMatcher 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 WithWorkerMatcher(matcherFunc func(*http.Request) bool) WorkerOption { + return func(w *workerOpt) error { + w.matchRequest = matcherFunc + return nil + } +} + +// WithWorkerServerScope scopes the worker to a server instance. +// Only requests that are handled by the server instance will reach the worker. +func WithWorkerServerScope(server *Server) WorkerOption { + return func(w *workerOpt) error { + w.server = server + + return nil + } +} + // WithWorkerMaxFailures sets the maximum number of consecutive failures before panicking func WithWorkerMaxFailures(maxFailures int) WorkerOption { return func(w *workerOpt) error { @@ -265,3 +290,35 @@ func withExtensionWorkers(w *extensionWorkers) WorkerOption { return nil } } + +// WithServer configures a server. +func WithServer( + idx int, + resolvedDocumentRoot string, + splitPath []string, + env map[string]string, +) (*Server, Option) { + return &Server{ + idx: idx, + }, func(o *opt) error { + if idx <= 0 { + return fmt.Errorf("server idx must be > 0, got %d", idx) + } + + root, err := fastabs.FastAbs(resolvedDocumentRoot) + if err != nil { + return err + } + + if err := normalizeSplitPath(splitPath); err != nil { + return err + } + + _, err = newServer(idx, root, splitPath, PrepareEnv(env)) + + if err != nil { + return err + } + return nil + } +} diff --git a/phpmainthread_test.go b/phpmainthread_test.go index 8ee5746505..428b02b0be 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, } } @@ -381,3 +383,22 @@ func testThreadCalculationError(t *testing.T, o *opt) { _, err := calculateMaxThreads(o) assert.Error(t, err, "configuration must error") } + +func TestContextAndLoggerMustNotBeNil(t *testing.T) { + log, ctx := getLogger(0) + assert.NotNil(t, log, "logger is defined if all threads are inactive") + assert.NotNil(t, ctx, "context is defined if all threads are inactive") + + fc := newContextFromMessage(nil, nil, nil, &worker{}) + assert.NotNil(t, fc.logger, "logger is defined for message context") + assert.NotNil(t, fc.ctx, "context is defined for message context") + + r := httptest.NewRequest("GET", "http://localhost/index.php", nil) + fc, _ = newContextFromRequest(r, nil, &server{}) + assert.NotNil(t, fc.logger, "logger is defined for request context") + assert.NotNil(t, fc.ctx, "context is defined for request context") + + fc, _ = newWorkerDummyContext(&worker{}) + assert.NotNil(t, fc.logger, "logger is defined for worker dummy context") + assert.NotNil(t, fc.ctx, "context is defined for worker dummy context") +} diff --git a/phpthread.go b/phpthread.go index 106ab2101b..39ee24b5d4 100644 --- a/phpthread.go +++ b/phpthread.go @@ -4,7 +4,6 @@ package frankenphp // #include "frankenphp.h" import "C" import ( - "context" "log/slog" "runtime" "sync" @@ -19,7 +18,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 +38,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 +50,7 @@ type threadHandler interface { func newPHPThread(threadIndex int) *phpThread { return &phpThread{ threadIndex: threadIndex, - requestChan: make(chan contextHolder), + requestChan: make(chan *frankenPHPContext), state: state.NewThreadState(), } } @@ -177,19 +175,6 @@ func (thread *phpThread) transitionToNewHandler() string { return thread.handler.beforeScriptExecution() } -func (thread *phpThread) frankenPHPContext() *frankenPHPContext { - return thread.handler.frankenPHPContext() -} - -func (thread *phpThread) context() context.Context { - if thread.handler == nil { - // handler can be nil when using opcache.preload - return globalCtx - } - - return thread.handler.context() -} - func (thread *phpThread) name() string { thread.handlerMu.RLock() defer thread.handlerMu.RUnlock() diff --git a/requestoptions.go b/requestoptions.go index 22efa5cc8d..e3dad1af95 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 @@ -164,7 +173,7 @@ func ensurePreparedEnv(env PreparedEnv) PreparedEnv { func WithOriginalRequest(r *http.Request) RequestOption { return func(o *frankenPHPContext) error { - o.originalRequest = r + o.requestURI = r.URL.RequestURI() return nil } @@ -175,6 +184,10 @@ func WithRequestLogger(logger *slog.Logger) RequestOption { return func(o *frankenPHPContext) error { o.logger = logger + if o.logger == nil { + o.logger = globalLogger // fall back to global logger + } + return nil } } 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/server.go b/server.go new file mode 100644 index 0000000000..05352f5897 --- /dev/null +++ b/server.go @@ -0,0 +1,114 @@ +package frankenphp + +import ( + "fmt" + "net/http" +) + +// server represents a server block in the caddyfile. +// can also be used to scope workers to a specific set of configurations. +type server struct { + idx int + root string + splitPath []string + env PreparedEnv + workers []*worker + workersByPath map[string]*worker + workersWithRequestMatcher []*worker + workerOpts []workerOpt +} + +type Server struct { + idx int +} + +var ( + servers = make(map[int]*server) + fallbackServer = &server{ + idx: -1, + workersByPath: make(map[string]*worker), + env: make(map[string]string), + } +) + +func resetServers() { + servers = make(map[int]*server) +} + +func newServer(idx int, root string, splitPath []string, env map[string]string) (*server, error) { + if _, ok := servers[idx]; ok { + return nil, fmt.Errorf("%w: duplicate registration of server with idx %d", ErrAlreadyRegistered, idx) + } + + server := &server{ + idx: idx, + root: root, + splitPath: splitPath, + env: env, + workersByPath: make(map[string]*worker), + workerOpts: make([]workerOpt, 0), + } + + if len(server.splitPath) == 0 { + server.splitPath = []string{".php"} + } + + if server.env == nil { + server.env = PrepareEnv(nil) + } + + servers[server.idx] = server + + return server, nil +} + +func (s *server) addWorker(w *worker) error { + s.workers = append(s.workers, w) + if w.matchRequest != nil { + s.workersWithRequestMatcher = append(s.workersWithRequestMatcher, w) + return nil + } + + if _, exists := s.workersByPath[w.fileName]; exists { + return fmt.Errorf("two workers in a server cannot have the same filename: %q", w.fileName) + } + s.workersByPath[w.fileName] = w + + return nil +} + +// ServeHTTP executes a PHP script on the registered server. +// The request will be scoped to the server instance that was registered via WithServer(). +// Otherwise, it is equivalent to calling ServeHTTP. +func (publicServer *Server) ServeHTTP(responseWriter http.ResponseWriter, request *http.Request, opts ...RequestOption) error { + s, ok := servers[publicServer.idx] + if !ok { + return fmt.Errorf("%w: server with idx %d was not initialized (%d servers initialized overall)", ErrServerNotFound, publicServer.idx, len(servers)) + } + + return s.serveHTTP(responseWriter, request, opts...) +} + +func (s *server) serveHTTP(responseWriter http.ResponseWriter, request *http.Request, opts ...RequestOption) error { + h := responseWriter.Header() + if h["Server"] == nil { + h["Server"] = serverHeader + } + + fc, err := newContextFromRequest(request, responseWriter, s, opts...) + if err != nil { + return err + } + + if err := fc.validate(); err != nil { + return err + } + + // Handle request with a worker if one is assigned + 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/server_test.go b/server_test.go new file mode 100644 index 0000000000..1466ca29ce --- /dev/null +++ b/server_test.go @@ -0,0 +1,181 @@ +package frankenphp_test + +import ( + "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 initServers(t *testing.T, opts ...frankenphp.Option) { + t.Helper() + t.Cleanup(frankenphp.Shutdown) + require.NoError(t, frankenphp.Init(opts...)) +} + +func serverRequest(t *testing.T, server *frankenphp.Server, req *http.Request) (string, *http.Response) { + t.Helper() + + w := httptest.NewRecorder() + require.NoError(t, server.ServeHTTP(w, req)) + + resp := w.Result() + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + + return string(body), resp +} + +func serverGet(t *testing.T, server *frankenphp.Server, url string) string { + t.Helper() + + body, _ := serverRequest(t, server, httptest.NewRequest(http.MethodGet, url, nil)) + + return body +} + +func TestServer(t *testing.T) { + t.Run("idx", func(t *testing.T) { + server1, opt1 := frankenphp.WithServer(1, testDataDir, nil, map[string]string{"PHP_SERVER_IDX_1": "1"}) + server2, opt2 := frankenphp.WithServer(2, testDataDir, nil, map[string]string{"PHP_SERVER_IDX_2": "2"}) + + initServers(t, opt1, opt2) + + body1 := serverGet(t, server1, "http://example.com/server-variable.php") + body2 := serverGet(t, server2, "http://example.com/server-variable.php") + + assert.Contains(t, body1, "[PHP_SERVER_IDX_1] => 1") + assert.Contains(t, body2, "[PHP_SERVER_IDX_2] => 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, opt := frankenphp.WithServer(1, testDataDir, nil, nil) + initServers(t, opt) + + body := serverGet(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, opt := frankenphp.WithServer(1, testDataDir, nil, map[string]string{"TEST_123": "123"}) + initServers(t, opt) + + body := serverGet(t, server, "http://example.com/server-variable.php") + + assert.Contains(t, body, "[TEST_123] => 123") + }) + + t.Run("split_path", func(t *testing.T) { + server, opt := frankenphp.WithServer(1, testDataDir, []string{".custom"}, nil) + initServers(t, opt) + + body := serverGet(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("workers_by_path_and_request_matcher", func(t *testing.T) { + server1, opt1 := frankenphp.WithServer(1, testDataDir, nil, nil) + server2, opt2 := frankenphp.WithServer(2, testDataDir, nil, nil) + initServers( + t, + opt1, + opt2, + frankenphp.WithWorkers("counter", testDataDir+"worker-with-counter.php", 1, frankenphp.WithWorkerServerScope(server1)), + frankenphp.WithWorkers("match", testDataDir+"worker-with-counter.php", 1, + frankenphp.WithWorkerServerScope(server2), + frankenphp.WithWorkerMatcher(func(r *http.Request) bool { + return strings.HasPrefix(r.URL.Path, "/match/") + }), + ), + ) + + body1 := serverGet(t, server1, "http://example.com/worker-with-counter.php") + body2 := serverGet(t, server1, "http://example.com/worker-with-counter.php") + body3 := serverGet(t, server2, "http://example.com/match/anything") + body4 := serverGet(t, server2, "http://example.com/match/anything") + body5 := serverGet(t, server2, "http://example.com/index.php") + body6 := serverGet(t, server1, "http://example.com/match/anything") + + assert.Equal(t, "requests:1", body1, "could not access the worker by path on server 1") + assert.Equal(t, "requests:2", body2, "could not access the worker by path on server 1") + assert.Equal(t, "requests:1", body3, "could not access the worker by matcher on server 2") + assert.Equal(t, "requests:2", body4, "could not access the worker by matcher on server 2") + assert.Contains(t, body5, "I am by birth a Genevese (i not set)", "could not access the worker by path on server 2") + assert.Contains(t, body6, "Failed opening required", "worker is scoped to server 1, so should not be found on server 2") + }) + + t.Run("worker_env_inheritance", func(t *testing.T) { + server, opt := frankenphp.WithServer(1, testDataDir, nil, map[string]string{ + "FROM_SERVER_ENV": "original", + "FROM_WORKER_ENV": "overridden", + }) + initServers( + t, + frankenphp.WithPhpIni(map[string]string{"variables_order": "EGPCS"}), + opt, + frankenphp.WithWorkers( + "env", + testDataDir+"env/env.php", + 1, + frankenphp.WithWorkerServerScope(server), + frankenphp.WithWorkerEnv(map[string]string{ + "FROM_WORKER_ENV": "original", + }), + ), + ) + + body := serverGet(t, server, "http://example.com/env/env.php?keys[]=FROM_SERVER_ENV&keys[]=FROM_WORKER_ENV") + + assert.Equal( + t, + "FROM_SERVER_ENV=original,FROM_WORKER_ENV=original", + body, + "should contain the server env and not override the worker env", + ) + }) + + t.Run("error_on_duplicate_worker_filenames", func(t *testing.T) { + t.Cleanup(frankenphp.Shutdown) + + server, opt := frankenphp.WithServer(1, testDataDir, nil, nil) + err := frankenphp.Init( + opt, + frankenphp.WithWorkers("worker1", testDataDir+"worker-with-counter.php", 1, frankenphp.WithWorkerServerScope(server)), + frankenphp.WithWorkers("worker2", testDataDir+"worker-with-counter.php", 1, frankenphp.WithWorkerServerScope(server)), + ) + + assert.Error(t, err) + assert.Contains(t, err.Error(), "two workers in a server cannot have the same filename") + }) + + t.Run("error_on_duplicate_registration", func(t *testing.T) { + _, opt1 := frankenphp.WithServer(1, testDataDir, nil, nil) + _, opt2 := frankenphp.WithServer(1, testDataDir, nil, nil) + err := frankenphp.Init(opt1, opt2) + + assert.ErrorIs(t, err, frankenphp.ErrAlreadyRegistered) + }) + + t.Run("error_on_missing_registration", func(t *testing.T) { + server, _ := frankenphp.WithServer(1, testDataDir, nil, nil) + 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.ErrServerNotFound) + }) +} 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 server.env { + if _, exists := o.env[k]; !exists { + o.env[k] = v + } + } + } + o.env["FRANKENPHP_WORKER\x00"] = "1" + 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, + server: server, } w.configureMercure(&o) @@ -222,7 +241,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 +251,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 +275,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 }