fix: disallow 2 workers with same filename (#1492)

* Disallows 2 workers with the same filename.

* Adds test.

* Prevent duplicate names.

---------

Co-authored-by: a.stecher <a.stecher@sportradar.com>
Co-authored-by: Alliballibaba <alliballibaba@gmail.com>

caddy/caddy.go

@@ -295,15 +295,6 @@ func (f *FrankenPHPApp) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
 					wc.FileName = filepath.Join(frankenphp.EmbeddedAppPath, wc.FileName)
 				}
 
-				if wc.Name == "" {
-					// let worker initialization validate if the FileName is valid or not
-					name, _ := fastabs.FastAbs(wc.FileName)
-					if name == "" {
-						name = wc.FileName
-					}
-					wc.Name = name
-				}
-
 				f.Workers = append(f.Workers, wc)
 			default:
 				allowedDirectives := "num_threads, max_threads, php_ini, worker, max_wait_time"

caddy/caddy_test.go

@@ -975,111 +975,6 @@ func TestMultiWorkersMetrics(t *testing.T) {
 		))
 }
 
-func TestMultiWorkersMetricsWithDuplicateName(t *testing.T) {
-	var wg sync.WaitGroup
-	tester := caddytest.NewTester(t)
-	tester.InitServer(`
-	{
-		skip_install_trust
-		admin localhost:2999
-		http_port `+testPort+`
-		https_port 9443
-		metrics
-
-		frankenphp {
-			worker {
-				name service1
-				file ../testdata/index.php
-				num 2
-			}
-			worker {
-				name service1
-				file ../testdata/ini.php
-				num 3
-			}
-		}
-	}
-
-	localhost:`+testPort+` {
-		route {
-			php {
-				root ../testdata
-			}
-		}
-	}
-
-	example.com:`+testPort+` {
-		route {
-			php {
-				root ../testdata
-			}
-		}
-	}
-	`, "caddyfile")
-
-	// Make some requests
-	for i := 0; i < 10; i++ {
-		wg.Add(1)
-		go func(i int) {
-			tester.AssertGetResponse(fmt.Sprintf("http://localhost:"+testPort+"/index.php?i=%d", i), http.StatusOK, fmt.Sprintf("I am by birth a Genevese (%d)", i))
-			wg.Done()
-		}(i)
-	}
-	wg.Wait()
-
-	// Fetch metrics
-	resp, err := http.Get("http://localhost:2999/metrics")
-	require.NoError(t, err, "failed to fetch metrics")
-	defer resp.Body.Close()
-
-	// Read and parse metrics
-	metrics := new(bytes.Buffer)
-	_, err = metrics.ReadFrom(resp.Body)
-	require.NoError(t, err, "failed to read metrics")
-
-	cpus := fmt.Sprintf("%d", frankenphp.MaxThreads)
-
-	// Check metrics
-	expectedMetrics := `
-	# HELP frankenphp_total_threads Total number of PHP threads
-	# TYPE frankenphp_total_threads counter
-	frankenphp_total_threads ` + cpus + `
-
-	# HELP frankenphp_busy_threads Number of busy PHP threads
-	# TYPE frankenphp_busy_threads gauge
-	frankenphp_busy_threads 5
-
-	# HELP frankenphp_busy_workers Number of busy PHP workers for this worker
-	# TYPE frankenphp_busy_workers gauge
-	frankenphp_busy_workers{worker="service1"} 0
-
-	# HELP frankenphp_total_workers Total number of PHP workers for this worker
-	# TYPE frankenphp_total_workers gauge
-	frankenphp_total_workers{worker="service1"} 5
-
-	# HELP frankenphp_worker_request_count
-	# TYPE frankenphp_worker_request_count counter
-	frankenphp_worker_request_count{worker="service1"} 10
-
-	# HELP frankenphp_ready_workers Running workers that have successfully called frankenphp_handle_request at least once
-	# TYPE frankenphp_ready_workers gauge
-	frankenphp_ready_workers{worker="service1"} 5
-	`
-
-	ctx := caddy.ActiveContext()
-	require.NoError(t,
-		testutil.GatherAndCompare(
-			ctx.GetMetricsRegistry(),
-			strings.NewReader(expectedMetrics),
-			"frankenphp_total_threads",
-			"frankenphp_busy_threads",
-			"frankenphp_busy_workers",
-			"frankenphp_total_workers",
-			"frankenphp_worker_request_count",
-			"frankenphp_ready_workers",
-		))
-}
-
 func TestDisabledMetrics(t *testing.T) {
 	var wg sync.WaitGroup
 	tester := caddytest.NewTester(t)
@@ -1097,7 +992,7 @@ func TestDisabledMetrics(t *testing.T) {
 				num 2
 			}
 			worker {
-				name service1
+				name service2
 				file ../testdata/ini.php
 				num 3
 			}

phpmainthread_test.go

@@ -177,6 +177,24 @@ func TestFinishBootingAWorkerScript(t *testing.T) {
 	assert.Nil(t, phpThreads)
 }
 
+func TestReturnAnErrorIf2WorkersHaveTheSameFileName(t *testing.T) {
+	workers = make(map[string]*worker)
+	_, err1 := newWorker(workerOpt{fileName: "filename.php"})
+	_, err2 := newWorker(workerOpt{fileName: "filename.php"})
+
+	assert.NoError(t, err1)
+	assert.Error(t, err2, "2 workers cannot have the same filename")
+}
+
+func TestReturnAnErrorIf2WorkersHaveTheSameName(t *testing.T) {
+	workers = make(map[string]*worker)
+	_, err1 := newWorker(workerOpt{fileName: "filename.php", name: "workername"})
+	_, err2 := newWorker(workerOpt{fileName: "filename2.php", name: "workername"})
+
+	assert.NoError(t, err1)
+	assert.Error(t, err2, "2 workers cannot have the same name")
+}
+
 func getDummyWorker(fileName string) *worker {
 	if workers == nil {
 		workers = make(map[string]*worker)

worker.go

@@ -35,11 +35,11 @@ func initWorkers(opt []workerOpt) error {
 
 	for _, o := range opt {
 		worker, err := newWorker(o)
-		worker.threads = make([]*phpThread, 0, o.num)
-		workersReady.Add(o.num)
 		if err != nil {
 			return err
 		}
+
+		workersReady.Add(o.num)
 		for i := 0; i < worker.num; i++ {
 			thread := getInactivePHPThread()
 			convertToWorkerThread(thread, worker)
@@ -74,6 +74,10 @@ func newWorker(o workerOpt) (*worker, error) {
 		o.env = make(PreparedEnv, 1)
 	}
 
+	if o.name == "" {
+		o.name = absFileName
+	}
+
 	o.env["FRANKENPHP_WORKER\x00"] = "1"
 	w := &worker{
 		name:        o.name,
@@ -81,7 +85,19 @@ func newWorker(o workerOpt) (*worker, error) {
 		num:         o.num,
 		env:         o.env,
 		requestChan: make(chan *frankenPHPContext),
+		threads:     make([]*phpThread, 0, o.num),
 	}
+
+	// ensure the filename or name are not already registered
+	for _, w := range workers {
+		if w.name == o.name {
+			return w, fmt.Errorf("2 workers cannot have the same name: %q.", o.name)
+		}
+		if w.fileName == absFileName {
+			return w, fmt.Errorf("2 workers cannot have the same filename: %q.", absFileName)
+		}
+	}
+
 	workers[absFileName] = w
 
 	return w, nil

worker_test.go

@@ -122,7 +122,7 @@ func TestWorkerGetOpt(t *testing.T) {
 func ExampleServeHTTP_workers() {
 	if err := frankenphp.Init(
 		frankenphp.WithWorkers("worker1", "worker1.php", 4, map[string]string{"ENV1": "foo"}, []string{}),
-		frankenphp.WithWorkers("worker1", "worker2.php", 2, map[string]string{"ENV2": "bar"}, []string{}),
+		frankenphp.WithWorkers("worker2", "worker2.php", 2, map[string]string{"ENV2": "bar"}, []string{}),
 	); err != nil {
 		panic(err)
 	}