fix: don't treat a finished request's write as an aborted connection - #2569
fix: don't treat a finished request's write as an aborted connection#2569dunglas wants to merge 2 commits into
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.
| // 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()) |
There was a problem hiding this comment.
Wouldn't you then always return false here for clientHasClosed? As the client might close the connection anytime after flushing
There was a problem hiding this comment.
You're right, and it's a real bug, not just a theoretical concern. Verified it: r.Context() gets canceled the instant the handler returns — standard net/http behavior, independent of whether the client is still connected. frankenphp_finish_request() is exactly what lets ServeHTTP return, so checking clientHasClosed() again afterward was reading true for virtually any write following a normal finish, not just a real disconnect — functionally the same bug this PR was meant to fix. It slipped past the existing tests because they drive requests through httptest.NewRecorder(), which never exercises Go's cancel-on-handler-return behavior at all (no real http.Server involved).
Pushed a fix: closeContext() now snapshots clientHasClosed() once, before close(fc.done) (the call that eventually lets the handler return), and go_ub_write uses that cached value instead of re-checking a context that's since been canceled for an unrelated reason. Added TestFinishRequestRealHTTPServerDoesNotAbort, which drives the request through a real httptest.NewServer — confirmed it fails against the old code and passes with the fix.
…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.
2 participants
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.