From d709aa7bb3108e55b2048fa992deb943796bbb59 Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Mon, 10 Mar 2025 22:30:56 +0100 Subject: [PATCH 1/5] Adds option to timeout when busy. --- caddy/caddy.go | 17 ++++++++++++++- caddy/caddy_test.go | 51 +++++++++++++++++++++++++++++++++++++++++++++ frankenphp.go | 13 ++++++++++++ options.go | 24 +++++++++++++++------ threadregular.go | 4 ++++ worker.go | 4 ++++ 6 files changed, 106 insertions(+), 7 deletions(-) diff --git a/caddy/caddy.go b/caddy/caddy.go index 1e5cc8da27..ffc562e1f7 100644 --- a/caddy/caddy.go +++ b/caddy/caddy.go @@ -11,6 +11,7 @@ import ( "path/filepath" "strconv" "strings" + "time" "github.com/dunglas/frankenphp/internal/fastabs" @@ -63,6 +64,8 @@ type FrankenPHPApp struct { Workers []workerConfig `json:"workers,omitempty"` // Overwrites the default php ini configuration PhpIni map[string]string `json:"php_ini,omitempty"` + // The maximum amount of time a request may be stalled wainting for threads + BusyTimeout time.Duration `json:"busy_timeout,omitempty"` metrics frankenphp.Metrics logger *zap.Logger @@ -93,6 +96,7 @@ func (f *FrankenPHPApp) Start() error { frankenphp.WithLogger(f.logger), frankenphp.WithMetrics(f.metrics), frankenphp.WithPhpIni(f.PhpIni), + frankenphp.WithBusyTimeout(f.BusyTimeout), } for _, w := range f.Workers { opts = append(opts, frankenphp.WithWorkers(repl.ReplaceKnown(w.FileName, ""), w.Num, w.Env, w.Watch)) @@ -156,6 +160,17 @@ func (f *FrankenPHPApp) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { } f.MaxThreads = int(v) + case "busy_timeout": + if !d.NextArg() { + return d.ArgErr() + } + + v, err := time.ParseDuration(d.Val()) + if err != nil { + return errors.New("busy_timeout must be a valid duration (example: 10s)") + } + + f.BusyTimeout = v case "php_ini": parseIniLine := func(d *caddyfile.Dispenser) error { key := d.Val() @@ -266,7 +281,7 @@ func (f *FrankenPHPApp) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { f.Workers = append(f.Workers, wc) default: - allowedDirectives := "num_threads, max_threads, php_ini, worker" + allowedDirectives := "num_threads, max_threads, php_ini, worker, busy_timeout" return wrongSubDirectiveError("frankenphp", allowedDirectives, d.Val()) } } diff --git a/caddy/caddy_test.go b/caddy/caddy_test.go index 0d374f47fc..118610f384 100644 --- a/caddy/caddy_test.go +++ b/caddy/caddy_test.go @@ -8,6 +8,7 @@ import ( "path/filepath" "strings" "sync" + "sync/atomic" "testing" "github.com/dunglas/frankenphp" @@ -748,3 +749,53 @@ func TestOsEnv(t *testing.T) { "ENV1=value1,ENV2=value2", ) } + +func TestGatewayTimeout(t *testing.T) { + tester := caddytest.NewTester(t) + tester.InitServer(` + { + skip_install_trust + admin localhost:2999 + http_port `+testPort+` + + frankenphp { + num_threads 1 + busy_timeout 1ns + } + } + + localhost:`+testPort+` { + route { + root ../testdata + php + } + } + `, "caddyfile") + + // send 10 requests simultaneously, at least one request should be stalled longer than 1ns + // since we only have 1 thread, this will cause a 504 Gateway Timeout + wg := sync.WaitGroup{} + success := atomic.Bool{} + wg.Add(10) + for i := 0; i < 10; i++ { + go func() { + statusCode := getStatusCode("http://localhost:"+testPort+"/sleep.php?sleep=10", t) + if statusCode == http.StatusGatewayTimeout { + success.Store(true) + } + wg.Done() + }() + } + wg.Wait() + + require.True(t, success.Load(), "At least one request should have failed with a 504 Gateway Timeout status") +} + +func getStatusCode(url string, t *testing.T) int { + req, err := http.NewRequest("GET", url, nil) + require.NoError(t, err) + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + defer resp.Body.Close() + return resp.StatusCode +} diff --git a/frankenphp.go b/frankenphp.go index f8012730a0..4c21afb8f6 100644 --- a/frankenphp.go +++ b/frankenphp.go @@ -41,6 +41,7 @@ import ( "strings" "sync" "syscall" + "time" "unsafe" "go.uber.org/zap" @@ -69,6 +70,8 @@ var ( logger *zap.Logger metrics Metrics = nullMetrics{} + + busyTimeout time.Duration ) type syslogLevel int @@ -221,6 +224,8 @@ func Init(options ...Option) error { metrics = opt.metrics } + busyTimeout = opt.busyTimeout + totalThreadCount, workerThreadCount, maxThreadCount, err := calculateMaxThreads(opt) if err != nil { return err @@ -617,3 +622,11 @@ func executePHPFunction(functionName string) bool { return C.frankenphp_execute_php_function(cFunctionName) == 1 } + +func timeOutIfBusy() <-chan time.Time { + if busyTimeout == 0 { + return nil + } + + return time.After(busyTimeout) +} diff --git a/options.go b/options.go index 6d1a5ebd21..c26c23c9eb 100644 --- a/options.go +++ b/options.go @@ -1,6 +1,8 @@ package frankenphp import ( + "time" + "go.uber.org/zap" ) @@ -11,12 +13,13 @@ type Option func(h *opt) error // // If you change this, also update the Caddy module and the documentation. type opt struct { - numThreads int - maxThreads int - workers []workerOpt - logger *zap.Logger - metrics Metrics - phpIni map[string]string + numThreads int + maxThreads int + workers []workerOpt + logger *zap.Logger + metrics Metrics + phpIni map[string]string + busyTimeout time.Duration } type workerOpt struct { @@ -76,3 +79,12 @@ func WithPhpIni(overrides map[string]string) Option { return nil } } + +// WithLogger configures the global logger to use. +func WithBusyTimeout(stallTime time.Duration) Option { + return func(o *opt) error { + o.busyTimeout = stallTime + + return nil + } +} diff --git a/threadregular.go b/threadregular.go index 19588d1a7c..a0a52e68cd 100644 --- a/threadregular.go +++ b/threadregular.go @@ -116,6 +116,10 @@ func handleRequestWithRegularPHPThreads(fc *frankenPHPContext) { return case scaleChan <- fc: // the request has triggered scaling, continue to wait for a thread + case <-timeOutIfBusy(): + // the request has timed out stalling + fc.reject(504, "Gateway Timeout") + return } } } diff --git a/worker.go b/worker.go index 598be8c630..aa90da0b3f 100644 --- a/worker.go +++ b/worker.go @@ -198,6 +198,10 @@ func (worker *worker) handleRequest(fc *frankenPHPContext) { return case scaleChan <- fc: // the request has triggered scaling, continue to wait for a thread + case <-timeOutIfBusy(): + // the request has timed out stalling + fc.reject(504, "Gateway Timeout") + return } } } From 7b9b512a951aad691577fd5460e9346985fa62f2 Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Mon, 10 Mar 2025 22:39:32 +0100 Subject: [PATCH 2/5] Renames option. --- caddy/caddy.go | 13 +++++++------ caddy/caddy_test.go | 2 +- frankenphp.go | 8 ++++---- options.go | 6 +++--- 4 files changed, 15 insertions(+), 14 deletions(-) diff --git a/caddy/caddy.go b/caddy/caddy.go index ffc562e1f7..fa070f483d 100644 --- a/caddy/caddy.go +++ b/caddy/caddy.go @@ -65,7 +65,7 @@ type FrankenPHPApp struct { // Overwrites the default php ini configuration PhpIni map[string]string `json:"php_ini,omitempty"` // The maximum amount of time a request may be stalled wainting for threads - BusyTimeout time.Duration `json:"busy_timeout,omitempty"` + MaxWaitTime time.Duration `json:"max_wait_time,omitempty"` metrics frankenphp.Metrics logger *zap.Logger @@ -96,7 +96,7 @@ func (f *FrankenPHPApp) Start() error { frankenphp.WithLogger(f.logger), frankenphp.WithMetrics(f.metrics), frankenphp.WithPhpIni(f.PhpIni), - frankenphp.WithBusyTimeout(f.BusyTimeout), + frankenphp.WithMaxWaitTime(f.MaxWaitTime), } for _, w := range f.Workers { opts = append(opts, frankenphp.WithWorkers(repl.ReplaceKnown(w.FileName, ""), w.Num, w.Env, w.Watch)) @@ -123,6 +123,7 @@ func (f *FrankenPHPApp) Stop() error { // reset configuration so it doesn't bleed into later tests f.Workers = nil f.NumThreads = 0 + f.MaxWaitTime = 0 return nil } @@ -160,17 +161,17 @@ func (f *FrankenPHPApp) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { } f.MaxThreads = int(v) - case "busy_timeout": + case "max_wait_time": if !d.NextArg() { return d.ArgErr() } v, err := time.ParseDuration(d.Val()) if err != nil { - return errors.New("busy_timeout must be a valid duration (example: 10s)") + return errors.New("max_wait_time must be a valid duration (example: 10s)") } - f.BusyTimeout = v + f.MaxWaitTime = v case "php_ini": parseIniLine := func(d *caddyfile.Dispenser) error { key := d.Val() @@ -281,7 +282,7 @@ func (f *FrankenPHPApp) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { f.Workers = append(f.Workers, wc) default: - allowedDirectives := "num_threads, max_threads, php_ini, worker, busy_timeout" + allowedDirectives := "num_threads, max_threads, php_ini, worker, max_wait_time" return wrongSubDirectiveError("frankenphp", allowedDirectives, d.Val()) } } diff --git a/caddy/caddy_test.go b/caddy/caddy_test.go index 118610f384..dba5c2be76 100644 --- a/caddy/caddy_test.go +++ b/caddy/caddy_test.go @@ -760,7 +760,7 @@ func TestGatewayTimeout(t *testing.T) { frankenphp { num_threads 1 - busy_timeout 1ns + max_wait_time 1ns } } diff --git a/frankenphp.go b/frankenphp.go index 4c21afb8f6..c6ea2915d7 100644 --- a/frankenphp.go +++ b/frankenphp.go @@ -71,7 +71,7 @@ var ( metrics Metrics = nullMetrics{} - busyTimeout time.Duration + maxWaitTime time.Duration ) type syslogLevel int @@ -224,7 +224,7 @@ func Init(options ...Option) error { metrics = opt.metrics } - busyTimeout = opt.busyTimeout + maxWaitTime = opt.maxWaitTime totalThreadCount, workerThreadCount, maxThreadCount, err := calculateMaxThreads(opt) if err != nil { @@ -624,9 +624,9 @@ func executePHPFunction(functionName string) bool { } func timeOutIfBusy() <-chan time.Time { - if busyTimeout == 0 { + if maxWaitTime == 0 { return nil } - return time.After(busyTimeout) + return time.After(maxWaitTime) } diff --git a/options.go b/options.go index c26c23c9eb..aa096073e8 100644 --- a/options.go +++ b/options.go @@ -19,7 +19,7 @@ type opt struct { logger *zap.Logger metrics Metrics phpIni map[string]string - busyTimeout time.Duration + maxWaitTime time.Duration } type workerOpt struct { @@ -81,9 +81,9 @@ func WithPhpIni(overrides map[string]string) Option { } // WithLogger configures the global logger to use. -func WithBusyTimeout(stallTime time.Duration) Option { +func WithMaxWaitTime(stallTime time.Duration) Option { return func(o *opt) error { - o.busyTimeout = stallTime + o.maxWaitTime = stallTime return nil } From 8db80d44ff3acceeb30d2068655b9fba5de3c520 Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Sat, 15 Mar 2025 14:42:20 +0100 Subject: [PATCH 3/5] Naming. --- caddy/caddy_test.go | 2 +- frankenphp.go | 2 +- frankenphp_test.go | 5 +++-- threadregular.go | 2 +- worker.go | 2 +- 5 files changed, 7 insertions(+), 6 deletions(-) diff --git a/caddy/caddy_test.go b/caddy/caddy_test.go index 10e9febe79..efc4241dcb 100644 --- a/caddy/caddy_test.go +++ b/caddy/caddy_test.go @@ -750,7 +750,7 @@ func TestOsEnv(t *testing.T) { ) } -func TestGatewayTimeout(t *testing.T) { +func TestMaxWaitTime(t *testing.T) { tester := caddytest.NewTester(t) tester.InitServer(` { diff --git a/frankenphp.go b/frankenphp.go index c9adea00d4..fbf700458c 100644 --- a/frankenphp.go +++ b/frankenphp.go @@ -629,7 +629,7 @@ func executePHPFunction(functionName string) bool { return C.frankenphp_execute_php_function(cFunctionName) == 1 } -func timeOutIfBusy() <-chan time.Time { +func timeoutIfBusy() <-chan time.Time { if maxWaitTime == 0 { return nil } 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/threadregular.go b/threadregular.go index a0a52e68cd..97200c9285 100644 --- a/threadregular.go +++ b/threadregular.go @@ -116,7 +116,7 @@ func handleRequestWithRegularPHPThreads(fc *frankenPHPContext) { return case scaleChan <- fc: // the request has triggered scaling, continue to wait for a thread - case <-timeOutIfBusy(): + case <-timeoutIfBusy(): // the request has timed out stalling fc.reject(504, "Gateway Timeout") return diff --git a/worker.go b/worker.go index aa90da0b3f..b3c9504aa6 100644 --- a/worker.go +++ b/worker.go @@ -198,7 +198,7 @@ func (worker *worker) handleRequest(fc *frankenPHPContext) { return case scaleChan <- fc: // the request has triggered scaling, continue to wait for a thread - case <-timeOutIfBusy(): + case <-timeoutIfBusy(): // the request has timed out stalling fc.reject(504, "Gateway Timeout") return From 9279e3c88f8cd0ec55f8d121dd9b7e6da9bb4b43 Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Sat, 15 Mar 2025 15:12:50 +0100 Subject: [PATCH 4/5] Naming. --- options.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/options.go b/options.go index aa096073e8..e166a5f294 100644 --- a/options.go +++ b/options.go @@ -81,9 +81,9 @@ func WithPhpIni(overrides map[string]string) Option { } // WithLogger configures the global logger to use. -func WithMaxWaitTime(stallTime time.Duration) Option { +func WithMaxWaitTime(maxWaitTime time.Duration) Option { return func(o *opt) error { - o.maxWaitTime = stallTime + o.maxWaitTime = maxWaitTime return nil } From 8b97c1403ff7b3d6eeb397b47267e8a17ed29704 Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Sat, 15 Mar 2025 23:04:17 +0100 Subject: [PATCH 5/5] Adjusts wording. --- caddy/caddy.go | 2 +- frankenphp.go | 6 +++--- options.go | 2 +- threadregular.go | 2 +- worker.go | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/caddy/caddy.go b/caddy/caddy.go index fa070f483d..ebcde3fcaf 100644 --- a/caddy/caddy.go +++ b/caddy/caddy.go @@ -64,7 +64,7 @@ type FrankenPHPApp struct { Workers []workerConfig `json:"workers,omitempty"` // Overwrites the default php ini configuration PhpIni map[string]string `json:"php_ini,omitempty"` - // The maximum amount of time a request may be stalled wainting for threads + // The maximum amount of time a request may be stalled waiting for a thread MaxWaitTime time.Duration `json:"max_wait_time,omitempty"` metrics frankenphp.Metrics diff --git a/frankenphp.go b/frankenphp.go index fbf700458c..a2e7b18e8e 100644 --- a/frankenphp.go +++ b/frankenphp.go @@ -629,10 +629,10 @@ func executePHPFunction(functionName string) bool { return C.frankenphp_execute_php_function(cFunctionName) == 1 } -func timeoutIfBusy() <-chan time.Time { - if maxWaitTime == 0 { +func timeoutChan(timeout time.Duration) <-chan time.Time { + if timeout == 0 { return nil } - return time.After(maxWaitTime) + return time.After(timeout) } diff --git a/options.go b/options.go index e166a5f294..c3821205f1 100644 --- a/options.go +++ b/options.go @@ -80,7 +80,7 @@ func WithPhpIni(overrides map[string]string) Option { } } -// WithLogger configures the global logger to use. +// WithMaxWaitTime configures the max time a request may be stalled waiting for a thread. func WithMaxWaitTime(maxWaitTime time.Duration) Option { return func(o *opt) error { o.maxWaitTime = maxWaitTime diff --git a/threadregular.go b/threadregular.go index 97200c9285..ac119208a3 100644 --- a/threadregular.go +++ b/threadregular.go @@ -116,7 +116,7 @@ func handleRequestWithRegularPHPThreads(fc *frankenPHPContext) { return case scaleChan <- fc: // the request has triggered scaling, continue to wait for a thread - case <-timeoutIfBusy(): + case <-timeoutChan(maxWaitTime): // the request has timed out stalling fc.reject(504, "Gateway Timeout") return diff --git a/worker.go b/worker.go index b3c9504aa6..aff577c34c 100644 --- a/worker.go +++ b/worker.go @@ -198,7 +198,7 @@ func (worker *worker) handleRequest(fc *frankenPHPContext) { return case scaleChan <- fc: // the request has triggered scaling, continue to wait for a thread - case <-timeoutIfBusy(): + case <-timeoutChan(maxWaitTime): // the request has timed out stalling fc.reject(504, "Gateway Timeout") return