diff --git a/caddy/caddy.go b/caddy/caddy.go index 501af3ed2e..eada38f73e 100644 --- a/caddy/caddy.go +++ b/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" diff --git a/caddy/caddy_test.go b/caddy/caddy_test.go index 76282fc2bd..5bbf96bf7f 100644 --- a/caddy/caddy_test.go +++ b/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 } diff --git a/phpmainthread_test.go b/phpmainthread_test.go index f75e72b134..97eb653aa7 100644 --- a/phpmainthread_test.go +++ b/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) diff --git a/worker.go b/worker.go index b0880ebbb1..60a2c534a0 100644 --- a/worker.go +++ b/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 diff --git a/worker_test.go b/worker_test.go index be404c2f8d..c08ea71554 100644 --- a/worker_test.go +++ b/worker_test.go @@ -119,7 +119,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) }