improved error handling

Author: dunglas

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"
@@ -150,6 +151,8 @@ func needReplacement(s string) bool {
 	return strings.ContainsAny(s, "{}")
 }
 
+var errRejected = &frankenphp.ErrRejected{}
+
 // ServeHTTP implements caddyhttp.MiddlewareHandler.
 func (f *FrankenPHPModule) ServeHTTP(w http.ResponseWriter, r *http.Request, _ caddyhttp.Handler) error {
 	origReq := r.Context().Value(caddyhttp.OriginalRequestCtxKey).(http.Request)
@@ -192,8 +195,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, 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,23 @@ 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: %s", ErrInvalidContentLengthHeader, contentLengthStr)
 
-			return false
+			fc.reject(e)
 		}
 	}
 
-	return true
+	return nil
 }
 
 func (fc *frankenPHPContext) clientHasClosed() bool {
@@ -149,27 +151,27 @@ func (fc *frankenPHPContext) clientHasClosed() bool {
 	}
 }
 
-// reject sends a response with the given status code and message
-func (fc *frankenPHPContext) reject(statusCode int, message string) error {
+// reject sends a response with the given status code and error
+func (fc *frankenPHPContext) reject(err error) {
 	if fc.isDone {
-		return nil
+		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()
 		}
 	}
 
 	fc.closeContext()
-
-	return ErrMaxTimeExceeded
-}
-
-func (fc *frankenPHPContext) rejectBadRequest(message string) {
-	fc.reject(http.StatusBadRequest, message)
 }

frankenphp.go

@@ -51,7 +51,10 @@ var (
 	ErrRequestContextCreation = errors.New("error during request context creation")
 	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")
-	ErrMaxTimeExceeded        = errors.New("max request handling time exceeded")
+
+	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()
@@ -64,6 +67,15 @@ var (
 	maxWaitTime time.Duration
 )
 
+type ErrRejected struct {
+	message string
+	status  int
+}
+
+func (e ErrRejected) Error() string {
+	return e.message
+}
+
 type syslogLevel int
 
 const (
@@ -332,8 +344,8 @@ 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

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
 			}
 

threadregular.go

@@ -113,7 +113,9 @@ func handleRequestWithRegularPHPThreads(fc *frankenPHPContext) error {
 			// the request has timed out stalling
 			metrics.DequeuedRequest()
 
-			return fc.reject(504, "Gateway Timeout")
+			fc.reject(ErrMaxWaitTimeExceeded)
+
+			return ErrMaxWaitTimeExceeded
 		}
 	}
 }

worker.go

@@ -258,7 +258,9 @@ func (worker *worker) handleRequest(fc *frankenPHPContext) error {
 			// the request has timed out stalling
 			metrics.DequeuedWorkerRequest(worker.name)
 
-			return fc.reject(504, "Gateway Timeout")
+			fc.reject(ErrMaxWaitTimeExceeded)
+
+			return ErrMaxWaitTimeExceeded
 		}
 	}
 }