refactor: improve ExtensionWorkers API (#1952)

* refactor: improve ExtensionWorkers API

* Update workerextension.go

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update workerextension.go

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update caddy/app.go

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Apply suggestion from @Copilot

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* review

* fix tests

* docs

* errors

* improved error handling

* fix race

* add missing return

* use %q in Errorf

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Author: dunglas

caddy/app.go

@@ -7,6 +7,7 @@ import (
 	"path/filepath"
 	"strconv"
 	"strings"
+	"sync"
 	"time"
 
 	"github.com/caddyserver/caddy/v2"
@@ -18,6 +19,22 @@ import (
 	"github.com/dunglas/frankenphp/internal/fastabs"
 )
 
+var (
+	options   []frankenphp.Option
+	optionsMU sync.RWMutex
+)
+
+// EXPERIMENTAL: RegisterWorkers provides a way for extensions to register frankenphp.Workers
+func RegisterWorkers(name, fileName string, num int, wo ...frankenphp.WorkerOption) frankenphp.Workers {
+	w, opt := frankenphp.WithExtensionWorkers(name, fileName, num, wo...)
+
+	optionsMU.Lock()
+	options = append(options, opt)
+	optionsMU.Unlock()
+
+	return w
+}
+
 // FrankenPHPApp represents the global "frankenphp" directive in the Caddyfile
 // it's responsible for starting up the global PHP instance and all threads
 //
@@ -118,6 +135,11 @@ func (f *FrankenPHPApp) Start() error {
 		frankenphp.WithPhpIni(f.PhpIni),
 		frankenphp.WithMaxWaitTime(f.MaxWaitTime),
 	}
+
+	optionsMU.RLock()
+	opts = append(opts, options...)
+	optionsMU.RUnlock()
+
 	for _, w := range append(f.Workers) {
 		workerOpts := []frankenphp.WorkerOption{
 			frankenphp.WithWorkerEnv(w.Env),
@@ -151,6 +173,10 @@ func (f *FrankenPHPApp) Stop() error {
 	f.NumThreads = 0
 	f.MaxWaitTime = 0
 
+	optionsMU.Lock()
+	options = nil
+	optionsMU.Unlock()
+
 	return nil
 }
 

caddy/caddy_test.go

@@ -965,15 +965,15 @@ func TestMaxWaitTime(t *testing.T) {
 	for range 10 {
 		go func() {
 			statusCode := getStatusCode("http://localhost:"+testPort+"/sleep.php?sleep=10", t)
-			if statusCode == http.StatusGatewayTimeout {
+			if statusCode == http.StatusServiceUnavailable {
 				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")
+	require.True(t, success.Load(), "At least one request should have failed with a 503 Service Unavailable status")
 }
 
 func TestMaxWaitTimeWorker(t *testing.T) {
@@ -1012,23 +1012,26 @@ func TestMaxWaitTimeWorker(t *testing.T) {
 	for range 10 {
 		go func() {
 			statusCode := getStatusCode("http://localhost:"+testPort+"/sleep.php?sleep=10&iteration=1", t)
-			if statusCode == http.StatusGatewayTimeout {
+			if statusCode == http.StatusServiceUnavailable {
 				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")
+	require.True(t, success.Load(), "At least one request should have failed with a 503 Service Unavailable status")
 
 	// Fetch metrics
 	resp, err := http.Get("http://localhost:2999/metrics")
 	require.NoError(t, err, "failed to fetch metrics")
-	defer resp.Body.Close()
+	t.Cleanup(func() {
+		require.NoError(t, resp.Body.Close())
+	})
 
 	// Read and parse metrics
 	metrics := new(bytes.Buffer)
 	_, err = metrics.ReadFrom(resp.Body)
+	require.NoError(t, err)
 
 	expectedMetrics := `
 	# TYPE frankenphp_worker_queue_depth gauge

caddy/module.go

@@ -2,6 +2,7 @@ package caddy
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"log/slog"
 	"net/http"
@@ -192,8 +193,11 @@ func (f *FrankenPHPModule) ServeHTTP(w http.ResponseWriter, r *http.Request, _ c
 		frankenphp.WithOriginalRequest(&origReq),
 		frankenphp.WithWorkerName(workerName),
 	)
+	if err != nil {
+		return caddyhttp.Error(http.StatusInternalServerError, err)
+	}
 
-	if err = frankenphp.ServeHTTP(w, fr); err != nil {
+	if err = frankenphp.ServeHTTP(w, fr); err != nil && !errors.As(err, &frankenphp.ErrRejected{}) {
 		return caddyhttp.Error(http.StatusInternalServerError, err)
 	}
 

context.go

@@ -2,6 +2,8 @@ package frankenphp
 
 import (
 	"context"
+	"errors"
+	"fmt"
 	"log/slog"
 	"net/http"
 	"os"
@@ -117,23 +119,25 @@ func (fc *frankenPHPContext) closeContext() {
 }
 
 // validate checks if the request should be outright rejected
-func (fc *frankenPHPContext) validate() bool {
+func (fc *frankenPHPContext) validate() error {
 	if strings.Contains(fc.request.URL.Path, "\x00") {
-		fc.rejectBadRequest("Invalid request path")
+		fc.reject(ErrInvalidRequestPath)
 
-		return false
+		return ErrInvalidRequestPath
 	}
 
 	contentLengthStr := fc.request.Header.Get("Content-Length")
 	if contentLengthStr != "" {
 		if contentLength, err := strconv.Atoi(contentLengthStr); err != nil || contentLength < 0 {
-			fc.rejectBadRequest("invalid Content-Length header: " + contentLengthStr)
+			e := fmt.Errorf("%w: %q", ErrInvalidContentLengthHeader, contentLengthStr)
+
+			fc.reject(e)
 
-			return false
+			return e
 		}
 	}
 
-	return true
+	return nil
 }
 
 func (fc *frankenPHPContext) clientHasClosed() bool {
@@ -149,16 +153,22 @@ func (fc *frankenPHPContext) clientHasClosed() bool {
 	}
 }
 
-// reject sends a response with the given status code and message
-func (fc *frankenPHPContext) reject(statusCode int, message string) {
+// reject sends a response with the given status code and error
+func (fc *frankenPHPContext) reject(err error) {
 	if fc.isDone {
 		return
 	}
 
+	re := &ErrRejected{}
+	if !errors.As(err, re) {
+		// Should never happen
+		panic("only instance of ErrRejected can be passed to reject")
+	}
+
 	rw := fc.responseWriter
 	if rw != nil {
-		rw.WriteHeader(statusCode)
-		_, _ = rw.Write([]byte(message))
+		rw.WriteHeader(re.status)
+		_, _ = rw.Write([]byte(err.Error()))
 
 		if f, ok := rw.(http.Flusher); ok {
 			f.Flush()
@@ -167,7 +177,3 @@ func (fc *frankenPHPContext) reject(statusCode int, message string) {
 
 	fc.closeContext()
 }
-
-func (fc *frankenPHPContext) rejectBadRequest(message string) {
-	fc.reject(http.StatusBadRequest, message)
-}

frankenphp.go

@@ -52,6 +52,10 @@ var (
 	ErrScriptExecution        = errors.New("error during PHP script execution")
 	ErrNotRunning             = errors.New("FrankenPHP is not running. For proper configuration visit: https://frankenphp.dev/docs/config/#caddyfile-config")
 
+	ErrInvalidRequestPath         = ErrRejected{"invalid request path", http.StatusBadRequest}
+	ErrInvalidContentLengthHeader = ErrRejected{"invalid Content-Length header", http.StatusBadRequest}
+	ErrMaxWaitTimeExceeded        = ErrRejected{"maximum request handling time exceeded", http.StatusServiceUnavailable}
+
 	isRunning        bool
 	onServerShutdown []func()
 
@@ -63,34 +67,43 @@ var (
 	maxWaitTime time.Duration
 )
 
+type ErrRejected struct {
+	message string
+	status  int
+}
+
+func (e ErrRejected) Error() string {
+	return e.message
+}
+
 type syslogLevel int
 
 const (
-	emerg   syslogLevel = iota // system is unusable
-	alert                      // action must be taken immediately
-	crit                       // critical conditions
-	err                        // error conditions
-	warning                    // warning conditions
-	notice                     // normal but significant condition
-	info                       // informational
-	debug                      // debug-level messages
+	syslogLevelEmerg  syslogLevel = iota // system is unusable
+	syslogLevelAlert                     // action must be taken immediately
+	syslogLevelCrit                      // critical conditions
+	syslogLevelErr                       // error conditions
+	syslogLevelWarn                      // warning conditions
+	syslogLevelNotice                    // normal but significant condition
+	syslogLevelInfo                      // informational
+	syslogLevelDebug                     // debug-level messages
 )
 
 func (l syslogLevel) String() string {
 	switch l {
-	case emerg:
+	case syslogLevelEmerg:
 		return "emerg"
-	case alert:
+	case syslogLevelAlert:
 		return "alert"
-	case crit:
+	case syslogLevelCrit:
 		return "crit"
-	case err:
+	case syslogLevelErr:
 		return "err"
-	case warning:
+	case syslogLevelWarn:
 		return "warning"
-	case notice:
+	case syslogLevelNotice:
 		return "notice"
-	case debug:
+	case syslogLevelDebug:
 		return "debug"
 	default:
 		return "info"
@@ -210,11 +223,6 @@ func Init(options ...Option) error {
 
 	registerExtensions()
 
-	// add registered external workers
-	for _, ew := range extensionWorkers {
-		options = append(options, WithWorkers(ew.name, ew.fileName, ew.num, ew.options...))
-	}
-
 	opt := &opt{}
 	for _, o := range options {
 		if err := o(opt); err != nil {
@@ -336,20 +344,17 @@ func ServeHTTP(responseWriter http.ResponseWriter, request *http.Request) error
 
 	fc.responseWriter = responseWriter
 
-	if !fc.validate() {
-		return nil
+	if err := fc.validate(); err != nil {
+		return err
 	}
 
 	// Detect if a worker is available to handle this request
 	if fc.worker != nil {
-		fc.worker.handleRequest(fc)
-
-		return nil
+		return fc.worker.handleRequest(fc)
 	}
 
 	// If no worker was available, send the request to non-worker threads
-	handleRequestWithRegularPHPThreads(fc)
-	return nil
+	return handleRequestWithRegularPHPThreads(fc)
 }
 
 //export go_ub_write
@@ -566,19 +571,19 @@ func go_log(message *C.char, level C.int) {
 	m := C.GoString(message)
 
 	var le syslogLevel
-	if level < C.int(emerg) || level > C.int(debug) {
-		le = info
+	if level < C.int(syslogLevelEmerg) || level > C.int(syslogLevelDebug) {
+		le = syslogLevelInfo
 	} else {
 		le = syslogLevel(level)
 	}
 
 	switch le {
-	case emerg, alert, crit, err:
+	case syslogLevelEmerg, syslogLevelAlert, syslogLevelCrit, syslogLevelErr:
 		logger.LogAttrs(context.Background(), slog.LevelError, m, slog.String("syslog_level", syslogLevel(level).String()))
 
-	case warning:
+	case syslogLevelWarn:
 		logger.LogAttrs(context.Background(), slog.LevelWarn, m, slog.String("syslog_level", syslogLevel(level).String()))
-	case debug:
+	case syslogLevelDebug:
 		logger.LogAttrs(context.Background(), slog.LevelDebug, m, slog.String("syslog_level", syslogLevel(level).String()))
 
 	default:

frankenphp_test.go

@@ -78,15 +78,17 @@ func runTest(t *testing.T, test func(func(http.ResponseWriter, *http.Request), *
 	}
 
 	err := frankenphp.Init(initOpts...)
-	require.Nil(t, err)
+	require.NoError(t, err)
 	defer frankenphp.Shutdown()
 
 	handler := func(w http.ResponseWriter, r *http.Request) {
 		req, err := frankenphp.NewRequestWithContext(r, frankenphp.WithRequestDocumentRoot(testDataDir, false))
 		assert.NoError(t, err)
 
 		err = frankenphp.ServeHTTP(w, req)
-		assert.NoError(t, err)
+		if err != nil && !errors.As(err, &frankenphp.ErrRejected{}) {
+			assert.Fail(t, fmt.Sprintf("Received unexpected error:\n%+v", err))
+		}
 	}
 
 	var ts *httptest.Server
@@ -109,6 +111,7 @@ func runTest(t *testing.T, test func(func(http.ResponseWriter, *http.Request), *
 
 func testRequest(req *http.Request, handler func(http.ResponseWriter, *http.Request), t *testing.T) (string, *http.Response) {
 	t.Helper()
+
 	w := httptest.NewRecorder()
 	handler(w, req)
 	resp := w.Result()
@@ -988,7 +991,7 @@ func FuzzRequest(f *testing.F) {
 			// The response status must be 400 if the request path contains null bytes
 			if strings.Contains(req.URL.Path, "\x00") {
 				assert.Equal(t, 400, resp.StatusCode)
-				assert.Contains(t, body, "Invalid request path")
+				assert.Contains(t, body, "invalid request path")
 				return
 			}