From 30c9df69ae90cc768e0f92e1ed402538f8dfabec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Mon, 27 Jul 2026 23:41:49 +0200 Subject: [PATCH 1/2] fix: don't treat a finished request's write as an aborted connection go_ub_write() reported any write after fastcgi_finish_request() as an aborted connection (0 bytes, closed=true). frankenphp_ub_write() then calls php_handle_aborted_connection(), which marks connection_status() as aborted and, when ignore_user_abort is off (the default outside worker mode), bails out the rest of the script at that statement. Worker mode forces ignore_user_abort=1, which is why this only silently truncates connection_status() there instead of visibly bailing out, but the underlying request never actually aborted in either case. Fixes https://github.com/php/frankenphp/issues/2548. --- frankenphp.go | 10 +++++++++- frankenphp_test.go | 11 ++++++++++- testdata/finish-request.php | 5 +++++ 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/frankenphp.go b/frankenphp.go index ad2dedc42a..f423ff1a29 100644 --- a/frankenphp.go +++ b/frankenphp.go @@ -433,7 +433,15 @@ func go_ub_write(threadIndex C.uintptr_t, cBuf *C.char, length C.size_t) (C.size fc := thread.frankenPHPContext() if fc.isDone { - return 0, C.bool(true) + // The request already finished (e.g. via fastcgi_finish_request()), so + // the responseWriter may no longer be safe to write to (see #2535): + // discard the write, but still report a genuine client disconnect via + // fc.clientHasClosed() rather than hardcoding "aborted". Otherwise + // frankenphp_ub_write() calls php_handle_aborted_connection() for a + // request that finished normally, which marks connection_status() as + // aborted and, when ignore_user_abort is off (the default outside + // worker mode), bails out the rest of the script. + return C.size_t(length), C.bool(fc.clientHasClosed()) } var writer io.Writer diff --git a/frankenphp_test.go b/frankenphp_test.go index 409e644634..e8d957e3fd 100644 --- a/frankenphp_test.go +++ b/frankenphp_test.go @@ -173,14 +173,23 @@ func TestEnvVarsInPhpIni(t *testing.T) { }) } -func TestFinishRequest_module(t *testing.T) { testFinishRequest(t, nil) } +func TestFinishRequest_module(t *testing.T) { testFinishRequest(t, &testOptions{}) } func TestFinishRequest_worker(t *testing.T) { testFinishRequest(t, &testOptions{workerScript: "finish-request.php"}) } func testFinishRequest(t *testing.T, opts *testOptions) { + var buf fmt.Stringer + opts.logger, buf = newTestLogger(t) + runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, i int) { body, _ := testGet(fmt.Sprintf("http://example.com/finish-request.php?i=%d", i), handler, t) assert.Equal(t, fmt.Sprintf("This is output %d\n", i), body) + + // The write after frankenphp_finish_request() must be silently + // discarded, not treated as an aborted connection that bails out the + // script: the log line after it must still be reached. + for !strings.Contains(buf.String(), fmt.Sprintf("reached after finish_request %d", i)) { + } }, opts) } diff --git a/testdata/finish-request.php b/testdata/finish-request.php index 2862f81388..4ae5f2d4c1 100644 --- a/testdata/finish-request.php +++ b/testdata/finish-request.php @@ -8,4 +8,9 @@ frankenphp_finish_request(); echo 'This is not'; + + // A write after the request is finished must not be treated as an + // aborted connection: with the default ignore_user_abort=Off, that + // would bail out the script here and skip this log line. + error_log('reached after finish_request '.($_GET['i'] ?? '')); }; From 727d6854941a698b1b35dca1b59c3f553d0409ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Wed, 29 Jul 2026 00:16:37 +0200 Subject: [PATCH 2/2] fix: snapshot client connection state before it's contaminated by finish_request @AlliBalliBaba caught this in review: request.Context() is canceled the instant the handler returns (standard net/http behavior, independent of whether the client is still connected), and frankenphp_finish_request() is exactly what lets ServeHTTP return. Checking fc.clientHasClosed() again after isDone was therefore reading true for virtually any write following a normal finish_request, not just a real disconnect - functionally the same bug this was meant to fix. Verified with a real net/http.Server (httptest.NewRecorder(), used by the rest of this suite, never exercises Go's cancel-on-return behavior at all, which is how the original version passed its own tests). Fix: capture clientHasClosed() once in closeContext(), before close(fc.done) - which is what triggers the eventual handler return - and use that cached value in go_ub_write instead of re-checking a context that's since been cancelled for an unrelated reason. --- context.go | 12 ++++++++ finishrequest_realserver_test.go | 50 ++++++++++++++++++++++++++++++++ frankenphp.go | 12 ++++++-- 3 files changed, 72 insertions(+), 2 deletions(-) create mode 100644 finishrequest_realserver_test.go diff --git a/context.go b/context.go index c582c6e453..86b611b8aa 100644 --- a/context.go +++ b/context.go @@ -35,6 +35,14 @@ type frankenPHPContext struct { // Whether the request is already closed by us isDone bool + // The client's connection state as of the moment isDone was set. Captured + // once in closeContext, before close(fc.done): that unblocks the request + // dispatcher, which lets the handler return, which is itself what cancels + // request.Context() (see the net/http docs - this happens whether or not + // the client is actually still connected). Checking clientHasClosed() + // again after isDone would therefore read true for virtually any write + // following a normal fastcgi_finish_request(), not just a real abort. + clientHadClosed bool responseWriter http.ResponseWriter responseController *http.ResponseController @@ -130,6 +138,10 @@ func (fc *frankenPHPContext) closeContext() { return } + // Snapshot before close(fc.done): that call is what eventually lets the + // handler return and cancels request.Context(), so clientHasClosed() + // must be read before it, not after. + fc.clientHadClosed = fc.clientHasClosed() close(fc.done) fc.isDone = true } diff --git a/finishrequest_realserver_test.go b/finishrequest_realserver_test.go new file mode 100644 index 0000000000..17747fea6a --- /dev/null +++ b/finishrequest_realserver_test.go @@ -0,0 +1,50 @@ +package frankenphp_test + +import ( + "io" + "net/http" + "net/http/httptest" + "os" + "strings" + "testing" + "time" + + "github.com/dunglas/frankenphp" + "github.com/stretchr/testify/require" +) + +// TestFinishRequestRealHTTPServerDoesNotAbort proves the frankenphp_finish_request() +// fix holds against a real net/http.Server, not just the httptest.NewRecorder() +// harness the rest of this suite uses. A recorder-driven test never exercises +// Go's automatic cancellation of request.Context() on handler return, which +// happens the instant frankenphp_finish_request() lets ServeHTTP return - +// independent of whether the client is still connected (see the discussion on +// php/frankenphp#2569). Checking clientHasClosed() again after that point, +// instead of the state captured before the handler returned, would read true +// for this reason alone and incorrectly abort the script. +func TestFinishRequestRealHTTPServerDoesNotAbort(t *testing.T) { + logger, buf := newTestLogger(t) + require.NoError(t, frankenphp.Init(frankenphp.WithLogger(logger))) + defer frankenphp.Shutdown() + + cwd, _ := os.Getwd() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + fr, err := frankenphp.NewRequestWithContext(r, frankenphp.WithRequestDocumentRoot(cwd+"/testdata/", false)) + require.NoError(t, err) + require.NoError(t, frankenphp.ServeHTTP(w, fr)) + })) + defer srv.Close() + + resp, err := http.Get(srv.URL + "/finish-request.php?i=1") + require.NoError(t, err) + defer func() { _ = resp.Body.Close() }() + + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.Equal(t, "This is output 1\n", string(body)) + + require.Eventually(t, func() bool { + return strings.Contains(buf.String(), "reached after finish_request 1") + }, 2*time.Second, 10*time.Millisecond, + "a write after a normal fastcgi_finish_request() must not be treated as an aborted connection") +} diff --git a/frankenphp.go b/frankenphp.go index f423ff1a29..8927a9037b 100644 --- a/frankenphp.go +++ b/frankenphp.go @@ -436,12 +436,20 @@ func go_ub_write(threadIndex C.uintptr_t, cBuf *C.char, length C.size_t) (C.size // The request already finished (e.g. via fastcgi_finish_request()), so // the responseWriter may no longer be safe to write to (see #2535): // discard the write, but still report a genuine client disconnect via - // fc.clientHasClosed() rather than hardcoding "aborted". Otherwise + // fc.clientHadClosed rather than hardcoding "aborted". Otherwise // frankenphp_ub_write() calls php_handle_aborted_connection() for a // request that finished normally, which marks connection_status() as // aborted and, when ignore_user_abort is off (the default outside // worker mode), bails out the rest of the script. - return C.size_t(length), C.bool(fc.clientHasClosed()) + // + // This must be the cached fc.clientHadClosed, not a fresh + // fc.clientHasClosed() call: request.Context() gets canceled as soon + // as the handler returns (standard net/http behavior, independent of + // whether the client is still connected), and the handler returns as + // a direct consequence of isDone becoming true. A fresh check here + // would read true for virtually every write after a normal + // fastcgi_finish_request(), reintroducing the original bug. + return C.size_t(length), C.bool(fc.clientHadClosed) } var writer io.Writer