fix: better max_threads calculation (#1446)

Author: AlliBalliBaba

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,23 +162,52 @@ func calculateMaxThreads(opt *opt) (int, int, int, error) {
 		numWorkers += opt.workers[i].num
 	}
 
-	if opt.numThreads <= 0 {
+	numThreadsIsSet := opt.numThreads > 0
+	maxThreadsIsSet := opt.maxThreads != 0
+	maxThreadsIsAuto := opt.maxThreads < 0 // maxthreads < 0 signifies auto mode (see phpmaintread.go)
+
+	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 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
+	}
+
+	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
 		} else {
 			opt.numThreads = maxProcs
 		}
-	} else if opt.numThreads <= numWorkers {
-		return opt.numThreads, numWorkers, opt.maxThreads, NotEnoughThreads
+		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 !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
+	}
 
 	return opt.numThreads, numWorkers, opt.maxThreads, nil
 }
@@ -226,6 +254,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) {

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