feat: unify php_server logic

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")
+}

caddy/app.go

@@ -7,7 +7,6 @@ import (
 	"log/slog"
 	"path/filepath"
 	"strconv"
-	"strings"
 	"sync"
 	"time"
 
@@ -17,7 +16,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 +62,7 @@ type FrankenPHPApp struct {
 	metrics frankenphp.Metrics
 	ctx     context.Context
 	logger  *slog.Logger
+	modules []*FrankenPHPModule
 }
 
 var errIni = errors.New(`"php_ini" must be in the format: php_ini "<key>" "<value>"`)
@@ -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()
 
@@ -188,19 +119,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()...))
+	}
+
+	if err := f.registerModules(repl); err != nil {
+		return err
 	}
 
+	// If FrankenPHP is currently running, shut it down first
+	// this will happen in admin API reloads (like in the caddy tests)
+	// make sure the app instance is reset after startup since it persists across reloads
 	frankenphp.Shutdown()
+	defer f.reset()
+
 	if err := frankenphp.Init(f.opts...); err != nil {
 		return err
 	}
@@ -220,16 +154,68 @@ 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()
+}
+
+// register all modules for Init()
+func (f *FrankenPHPApp) registerModules(repl *caddy.Replacer) error {
+	modulesByIndex := make(map[int]*FrankenPHPModule)
+	for _, module := range f.modules {
+		if module.ServerIdx == 0 {
+			// module has no dedicated index, this can happen if registered via json route config
+			if err := f.registerModule(repl, module); err != nil {
+				return err
+			}
+			continue
+		}
+
+		// ignore modules with a duplicate index
+		// this can happen if multiple "php" modules are defined within the same caddy subroute
+		if existingModule, ok := modulesByIndex[module.ServerIdx]; ok {
+			module.server = existingModule.server
+			continue
+		}
+
+		modulesByIndex[module.ServerIdx] = module
+		if err := f.registerModule(repl, module); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
+// register module server and workers for Init()
+func (f *FrankenPHPApp) registerModule(repl *caddy.Replacer, module *FrankenPHPModule) error {
+	server, err := frankenphp.NewServer(module.resolvedDocumentRoot, module.SplitPath, module.resolvedEnv)
+	if err != nil {
+		return err
+	}
+
+	module.server = server
+	f.opts = append(f.opts, frankenphp.WithServer(server))
+
+	for _, w := range module.Workers {
+		w.FileName = repl.ReplaceKnown(w.FileName, "")
+		workerOptions := append(w.toWorkerOptions(), frankenphp.WithWorkerServerScope(server))
+		f.opts = append(f.opts, frankenphp.WithWorkers(w.Name, w.FileName, w.Num, workerOptions...))
+	}
 
 	return nil
 }
@@ -344,8 +330,8 @@ func (f *FrankenPHPApp) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
 				if frankenphp.EmbeddedAppPath != "" && filepath.IsLocal(wc.FileName) {
 					wc.FileName = filepath.Join(frankenphp.EmbeddedAppPath, wc.FileName)
 				}
-				if strings.HasPrefix(wc.Name, "m#") {
-					return d.Errf(`global worker names must not start with "m#": %q`, wc.Name)
+				if len(wc.Name) >= 3 && wc.Name[0] == 'm' && wc.Name[2] == '#' {
+					return d.Errf(`global worker names must not start with "m<num>#": %q`, wc.Name)
 				}
 				// check for duplicate workers
 				for _, existingWorker := range f.Workers {