feat: unify php_server logic

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")
 
@@ -79,6 +76,7 @@ func TestShowTheCorrectThreadDebugStatus(t *testing.T) {
 	debugState := getDebugState(t, tester)
 
 	// assert that the correct threads are present in the thread info
+	assert.Len(t, debugState.ThreadDebugStates, 3)
 	assert.Equal(t, debugState.ThreadDebugStates[0].State, "ready")
 	assert.Contains(t, debugState.ThreadDebugStates[1].Name, "worker-with-counter.php")
 	assert.Contains(t, debugState.ThreadDebugStates[2].Name, "index.php")
@@ -325,7 +323,7 @@ func TestAddModuleWorkerViaAdminApi(t *testing.T) {
 	assert.NoError(t, err)
 	r.Header.Set("Content-Type", "text/caddyfile")
 	resp := tester.AssertResponseCode(r, http.StatusOK)
-	defer func() { require.NoError(t, resp.Body.Close()) }()
+	require.NoError(t, resp.Body.Close())
 
 	// Get the updated debug state to check if the worker was added
 	updatedDebugState := getDebugState(t, tester)
@@ -348,3 +346,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")
+}

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 "<key>" "<value>"`)
@@ -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,23 +120,33 @@ 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
+	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()
 	if err := frankenphp.Init(f.opts...); err != nil {
 		return err
 	}
 
+	// after startup, reset all configuration for future reloads or tests
+	// it is necessary to do this here since caddy will re-use the app instance
+	f.reset()
+
 	return nil
 }
 
@@ -220,18 +162,35 @@ 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
+func (f *FrankenPHPApp) registerModule(m *FrankenPHPModule, serverOpt frankenphp.Option) {
+	if f.modules == nil {
+		f.modules = make(map[int]*FrankenPHPModule)
+	}
+
+	if _, ok := f.modules[m.ServerIdx]; !ok {
+		// only register the module if it's not already registered
+		f.modules[m.ServerIdx] = m
+		f.opts = append(f.opts, serverOpt)
+	}
 }
 
 // UnmarshalCaddyfile implements caddyfile.Unmarshaler.

caddy/caddy_test.go

@@ -442,7 +442,7 @@ func TestCustomCaddyVariablesInEnv(t *testing.T) {
 	tester.AssertGetResponse("http://localhost:"+testPort+"/worker-env.php", http.StatusOK, "hello world")
 }
 
-func TestPHPServerDirective(t *testing.T) {
+func TestServerDirective(t *testing.T) {
 	tester := caddytest.NewTester(t)
 	initServer(t, tester, `
 		{
@@ -463,7 +463,7 @@ func TestPHPServerDirective(t *testing.T) {
 	tester.AssertGetResponse("http://localhost:"+testPort+"/not-found.txt", http.StatusOK, "I am by birth a Genevese (i not set)")
 }
 
-func TestPHPServerDirectiveDisableFileServer(t *testing.T) {
+func TestServerDirectiveDisableFileServer(t *testing.T) {
 	tester := caddytest.NewTester(t)
 	initServer(t, tester, `
 		{
@@ -487,7 +487,7 @@ func TestPHPServerDirectiveDisableFileServer(t *testing.T) {
 	tester.AssertGetResponse("http://localhost:"+testPort+"/not-found.txt", http.StatusOK, "I am by birth a Genevese (i not set)")
 }
 
-func TestPHPServerGlobals(t *testing.T) {
+func TestServerGlobals(t *testing.T) {
 	documentRoot, _ := filepath.Abs("../testdata")
 	scriptFilename := filepath.Join(documentRoot, "server-globals.php")
 
@@ -539,7 +539,7 @@ REQUEST_URI: /server-globals.php/en
 	)
 }
 
-func TestWorkerPHPServerGlobals(t *testing.T) {
+func TestWorkerServerGlobals(t *testing.T) {
 	documentRoot, _ := filepath.Abs("../testdata")
 	documentRoot2, _ := filepath.Abs("../caddy")
 	scriptFilename := documentRoot + string(filepath.Separator) + "server-globals.php"
@@ -859,7 +859,7 @@ func TestWorkerMetrics(t *testing.T) {
 }
 
 // #2477: verify one pool per worker, no "_0" duplicates.
-func TestPhpServerWorkerMatchPoolCount(t *testing.T) {
+func TestServerWorkerMatchPoolCount(t *testing.T) {
 	tester := caddytest.NewTester(t)
 	initServer(t, tester, `
 	{