fix: don't treat a finished request's write as an aborted connection - #2569
Merged
Conversation
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 #2548.
dunglas
marked this pull request as ready for review
July 28, 2026 07:55
…ish_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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
go_ub_write()reported any PHP output write that happens afterfastcgi_finish_request()/frankenphp_finish_request()as an aborted connection (0bytes written,closed=true), regardless of whether the client was actually still connected.frankenphp_ub_write()then callsphp_handle_aborted_connection()for that write, which:connection_status()as aborted even though the request finished normally, andignore_user_abortis off (PHP's default outside worker mode), callszend_bailout(), silently terminating the rest of the script right at that output statement.Worker mode forces
ignore_user_abort=1on every request, which is why this doesn't visibly crash a worker script — it only corruptsconnection_status()there. Outside worker mode, any code that echoes/outputs anything afterfastcgi_finish_request()gets silently truncated.Fix
Discard the write (the responseWriter may no longer be safe to touch, see #2535/#2538), but report the actual client-disconnect status via
fc.clientHasClosed()instead of hardcodingtrue.Reproducer
In classic (non-worker) mode with default
ignore_user_abort,echo "After"triggers the bailout and any code after it never runs.Tests
TestFinishRequest(testdata/finish-request.php) to log a line after the discarded write and assert it's reached — hangs on the unpatched code (module mode), passes with the fix.TestConnectionAbortstill passes, confirming a genuine client disconnect is still correctly reported afterfastcgi_finish_request().Fixes #2548.