fix: don't treat a finished request's write as an aborted connection

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
 }

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

frankenphp.go

@@ -433,7 +433,23 @@ 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.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.
+		//
+		// 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

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

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'] ?? ''));
 };