From 1957da42cb898f37281faaead6020b77d3bcb0df Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Sat, 15 Mar 2025 16:41:07 +0100 Subject: [PATCH 1/3] Refines thread calculation. --- frankenphp.go | 52 +++++++++++++++++++++++++++++++++++++------ frankenphp_test.go | 5 +++-- phpmainthread_test.go | 44 ++++++++++++++++++++++++++++++++++++ 3 files changed, 92 insertions(+), 9 deletions(-) diff --git a/frankenphp.go b/frankenphp.go index 48f71be381..12d25d879a 100644 --- a/frankenphp.go +++ b/frankenphp.go @@ -57,7 +57,6 @@ var ( InvalidRequestError = errors.New("not a FrankenPHP request") AlreadyStartedError = errors.New("FrankenPHP is already started") InvalidPHPVersionError = errors.New("FrankenPHP is only compatible with PHP 8.2+") - NotEnoughThreads = errors.New("the number of threads must be superior to the number of workers") MainThreadCreationError = errors.New("error creating the main thread") RequestContextCreationError = errors.New("error during request context creation") ScriptExecutionError = errors.New("error during PHP script execution") @@ -163,6 +162,33 @@ func calculateMaxThreads(opt *opt) (int, int, int, error) { numWorkers += opt.workers[i].num } + // max_threads is set, num_threads is not set + if opt.maxThreads != 0 && opt.numThreads <= 0 { + opt.numThreads = numWorkers + 1 + if opt.maxThreads > 0 && opt.numThreads > opt.maxThreads { + err := fmt.Errorf("max_threads (%d) must be greater than the number of worker threads (%d)", opt.maxThreads, numWorkers) + return 0, 0, 0, err + } + + return opt.numThreads, numWorkers, opt.maxThreads, nil + } + + // num_threads is set, max_threads is not set + if opt.numThreads > 0 && opt.maxThreads <= 0 { + if opt.numThreads <= numWorkers { + err := fmt.Errorf("num_threads (%d) must be greater than the number of worker threads (%d)", opt.numThreads, numWorkers) + return 0, 0, 0, err + } + + // -1 means signifies automatic max_threads calculation + if opt.maxThreads == 0 { + opt.maxThreads = opt.numThreads + } + + return opt.numThreads, numWorkers, opt.maxThreads, nil + } + + // neither num_threads nor max_threads are set if opt.numThreads <= 0 { if numWorkers >= maxProcs { // Start at least as many threads as workers, and keep a free thread to handle requests in non-worker mode @@ -170,16 +196,25 @@ func calculateMaxThreads(opt *opt) (int, int, int, error) { } else { opt.numThreads = maxProcs } - } else if opt.numThreads <= numWorkers { - return opt.numThreads, numWorkers, opt.maxThreads, NotEnoughThreads + + // -1 means signifies automatic max_threads calculation + if opt.maxThreads == 0 { + opt.maxThreads = opt.numThreads + } + + return opt.numThreads, numWorkers, opt.maxThreads, nil } - if opt.maxThreads < opt.numThreads && opt.maxThreads > 0 { - opt.maxThreads = opt.numThreads + // both num_threads and max_threads are set + if opt.numThreads <= numWorkers { + err := fmt.Errorf("num_threads (%d) must be greater than the number of worker threads (%d)", opt.numThreads, numWorkers) + return 0, 0, 0, err } - metrics.TotalThreads(opt.numThreads) - MaxThreads = opt.numThreads + if opt.maxThreads < opt.numThreads { + err := fmt.Errorf("max_threads (%d) must be greater than or equal to num_threads (%d)", opt.maxThreads, opt.numThreads) + return 0, 0, 0, err + } return opt.numThreads, numWorkers, opt.maxThreads, nil } @@ -226,6 +261,9 @@ func Init(options ...Option) error { return err } + metrics.TotalThreads(totalThreadCount) + MaxThreads = totalThreadCount + config := Config() if config.Version.MajorVersion < 8 || (config.Version.MajorVersion == 8 && config.Version.MinorVersion < 2) { diff --git a/frankenphp_test.go b/frankenphp_test.go index 731fa32e85..30fc8323d2 100644 --- a/frankenphp_test.go +++ b/frankenphp_test.go @@ -672,11 +672,12 @@ func TestFailingWorker(t *testing.T) { } func TestEnv(t *testing.T) { - testEnv(t, &testOptions{nbParallelRequests:1}) + testEnv(t, &testOptions{nbParallelRequests: 1}) } func TestEnvWorker(t *testing.T) { - testEnv(t, &testOptions{nbParallelRequests:1, workerScript: "env/test-env.php"}) + testEnv(t, &testOptions{nbParallelRequests: 1, workerScript: "env/test-env.php"}) } + // testEnv cannot be run in parallel due to https://github.com/golang/go/issues/63567 func testEnv(t *testing.T, opts *testOptions) { assert.NoError(t, os.Setenv("EMPTY", "")) diff --git a/phpmainthread_test.go b/phpmainthread_test.go index 71f4667d37..8379dc7b4c 100644 --- a/phpmainthread_test.go +++ b/phpmainthread_test.go @@ -5,6 +5,7 @@ import ( "math/rand/v2" "net/http/httptest" "path/filepath" + "runtime" "sync" "sync/atomic" "testing" @@ -214,3 +215,46 @@ func allPossibleTransitions(worker1Path string, worker2Path string) []func(*phpT convertToInactiveThread, } } + +func TestCorrectThreadCalculation(t *testing.T) { + maxProcs := runtime.GOMAXPROCS(0) * 2 + oneWorkerThread := []workerOpt{workerOpt{num: 1}} + + // default values + testThreadCalculation(t, maxProcs, maxProcs, &opt{}) + testThreadCalculation(t, maxProcs, maxProcs, &opt{workers: oneWorkerThread}) + + // num_threads is set + testThreadCalculation(t, 1, 1, &opt{numThreads: 1}) + testThreadCalculation(t, 2, 2, &opt{numThreads: 2, workers: oneWorkerThread}) + + // max_threads is set + testThreadCalculation(t, 1, 10, &opt{maxThreads: 10}) + testThreadCalculation(t, 2, 10, &opt{maxThreads: 10, workers: oneWorkerThread}) + testThreadCalculation(t, 5, 10, &opt{numThreads: 5, maxThreads: 10, workers: oneWorkerThread}) + + // automatic max_threads + testThreadCalculation(t, 1, -1, &opt{maxThreads: -1}) + testThreadCalculation(t, 2, -1, &opt{maxThreads: -1, workers: oneWorkerThread}) + testThreadCalculation(t, 2, -1, &opt{numThreads: 2, maxThreads: -1}) + + // not enough num threads + testThreadCalculationError(t, &opt{numThreads: 1, workers: oneWorkerThread}) + testThreadCalculationError(t, &opt{numThreads: 1, maxThreads: 1, workers: oneWorkerThread}) + + // not enough max_threads + testThreadCalculationError(t, &opt{numThreads: 2, maxThreads: 1}) + testThreadCalculationError(t, &opt{maxThreads: 1, workers: oneWorkerThread}) +} + +func testThreadCalculation(t *testing.T, expectedNumThreads int, expectedMaxThreads int, o *opt) { + totalThreadCount, _, maxThreadCount, err := calculateMaxThreads(o) + assert.NoError(t, err, "no error should be returned") + assert.Equal(t, expectedNumThreads, totalThreadCount, "num_threads must be correct") + assert.Equal(t, expectedMaxThreads, maxThreadCount, "max_threads must be correct") +} + +func testThreadCalculationError(t *testing.T, o *opt) { + _, _, _, err := calculateMaxThreads(o) + assert.Error(t, err, "configuration must error") +} From e0f8a6bd27419f247e938ef3200347289d0f8ceb Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Sat, 15 Mar 2025 17:01:39 +0100 Subject: [PATCH 2/3] Makes logic clearer. --- frankenphp.go | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/frankenphp.go b/frankenphp.go index 12d25d879a..8f9eee9776 100644 --- a/frankenphp.go +++ b/frankenphp.go @@ -162,8 +162,11 @@ func calculateMaxThreads(opt *opt) (int, int, int, error) { numWorkers += opt.workers[i].num } - // max_threads is set, num_threads is not set - if opt.maxThreads != 0 && opt.numThreads <= 0 { + numThreadsIsSet := opt.numThreads > 0 + maxThreadsIsSet := opt.maxThreads != 0 + maxThreadsIsAuto := opt.maxThreads < 0 // maxthreads < 0 signifies auto mode (see phpmaintread.go) + + if maxThreadsIsSet && !numThreadsIsSet { opt.numThreads = numWorkers + 1 if opt.maxThreads > 0 && opt.numThreads > opt.maxThreads { err := fmt.Errorf("max_threads (%d) must be greater than the number of worker threads (%d)", opt.maxThreads, numWorkers) @@ -173,34 +176,25 @@ func calculateMaxThreads(opt *opt) (int, int, int, error) { return opt.numThreads, numWorkers, opt.maxThreads, nil } - // num_threads is set, max_threads is not set - if opt.numThreads > 0 && opt.maxThreads <= 0 { + if numThreadsIsSet && !maxThreadsIsSet { + opt.maxThreads = opt.numThreads if opt.numThreads <= numWorkers { err := fmt.Errorf("num_threads (%d) must be greater than the number of worker threads (%d)", opt.numThreads, numWorkers) return 0, 0, 0, err } - // -1 means signifies automatic max_threads calculation - if opt.maxThreads == 0 { - opt.maxThreads = opt.numThreads - } - return opt.numThreads, numWorkers, opt.maxThreads, nil } - // neither num_threads nor max_threads are set - if opt.numThreads <= 0 { + // both num_thread and max_threads are not set + if !numThreadsIsSet { if numWorkers >= maxProcs { // Start at least as many threads as workers, and keep a free thread to handle requests in non-worker mode opt.numThreads = numWorkers + 1 } else { opt.numThreads = maxProcs } - - // -1 means signifies automatic max_threads calculation - if opt.maxThreads == 0 { - opt.maxThreads = opt.numThreads - } + opt.maxThreads = opt.numThreads return opt.numThreads, numWorkers, opt.maxThreads, nil } @@ -211,7 +205,7 @@ func calculateMaxThreads(opt *opt) (int, int, int, error) { return 0, 0, 0, err } - if opt.maxThreads < opt.numThreads { + if !maxThreadsIsAuto && opt.maxThreads < opt.numThreads { err := fmt.Errorf("max_threads (%d) must be greater than or equal to num_threads (%d)", opt.maxThreads, opt.numThreads) return 0, 0, 0, err } From 1f8e0cf7350614b8b7c40c9a380b8ac5e3ebadee Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Sat, 15 Mar 2025 17:15:19 +0100 Subject: [PATCH 3/3] Makes logic clearer. --- frankenphp.go | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/frankenphp.go b/frankenphp.go index 8f9eee9776..0d790708a9 100644 --- a/frankenphp.go +++ b/frankenphp.go @@ -166,28 +166,27 @@ func calculateMaxThreads(opt *opt) (int, int, int, error) { maxThreadsIsSet := opt.maxThreads != 0 maxThreadsIsAuto := opt.maxThreads < 0 // maxthreads < 0 signifies auto mode (see phpmaintread.go) - if maxThreadsIsSet && !numThreadsIsSet { - opt.numThreads = numWorkers + 1 - if opt.maxThreads > 0 && opt.numThreads > opt.maxThreads { - err := fmt.Errorf("max_threads (%d) must be greater than the number of worker threads (%d)", opt.maxThreads, numWorkers) + if numThreadsIsSet && !maxThreadsIsSet { + opt.maxThreads = opt.numThreads + if opt.numThreads <= numWorkers { + err := fmt.Errorf("num_threads (%d) must be greater than the number of worker threads (%d)", opt.numThreads, numWorkers) return 0, 0, 0, err } return opt.numThreads, numWorkers, opt.maxThreads, nil } - if numThreadsIsSet && !maxThreadsIsSet { - opt.maxThreads = opt.numThreads - if opt.numThreads <= numWorkers { - err := fmt.Errorf("num_threads (%d) must be greater than the number of worker threads (%d)", opt.numThreads, numWorkers) + if maxThreadsIsSet && !numThreadsIsSet { + opt.numThreads = numWorkers + 1 + if !maxThreadsIsAuto && opt.numThreads > opt.maxThreads { + err := fmt.Errorf("max_threads (%d) must be greater than the number of worker threads (%d)", opt.maxThreads, numWorkers) return 0, 0, 0, err } return opt.numThreads, numWorkers, opt.maxThreads, nil } - // both num_thread and max_threads are not set - if !numThreadsIsSet { + if !numThreadsIsSet && !maxThreadsIsSet { if numWorkers >= maxProcs { // Start at least as many threads as workers, and keep a free thread to handle requests in non-worker mode opt.numThreads = numWorkers + 1