fix: bound response writes so a stalled client can't wedge a thread forever - #2574
Closed
dunglas wants to merge 2 commits into
Closed
fix: bound response writes so a stalled client can't wedge a thread forever#2574dunglas wants to merge 2 commits into
dunglas wants to merge 2 commits into
Conversation
…orever frankenphp_force_kill_thread() cannot interrupt a write parked in Go's non-blocking netpoller: the stall isn't a blocking syscall, so there's no opcode boundary or EINTR-able syscall for it to reach. A client that stops reading holds the handling thread in go_ub_write indefinitely. Add an idle write deadline via http.ResponseController, mirroring the existing read-deadline handling in go_read_post (#2538). Exposed as WithResponseWriteTimeout() and the response_write_timeout Caddyfile directive, defaulting to 60s like request_body_timeout.
This was referenced Jul 28, 2026
dunglas
marked this pull request as ready for review
July 28, 2026 12:52
| fc.responseController = http.NewResponseController(fc.responseWriter) | ||
| } | ||
| rc = fc.responseController | ||
| _ = rc.SetWriteDeadline(time.Now().Add(fc.responseWriteTimeout)) |
Member
There was a problem hiding this comment.
SetWriteDeadline bounds the whole Write call, and with output_buffering=0 php_output_op hands ub_write an entire echo in a single call, so this is a "transfer this buffer within 60s" cap rather than an idle timeout. A 20 MB response to a sub-340 KB/s client is truncated via php_handle_aborted_connection(). nginx's send_timeout is explicitly "set only between two successive write operations, not for the transmission of the whole response", and Caddy ships write_timeout unset for this reason.
So, I guess you should slice the write and re-arm per chunk, or drop the idle-timeout/send_timeout framing from module.go, docs/config.md and docs/security.md.
Member
Author
There was a problem hiding this comment.
You're right, and it's a real bug — verified it, not just in theory. Fixed by slicing the write into 64 KiB chunks and resetting the deadline before each one, mirroring go_read_post's existing per-read reset, so this now bounds a stall rather than the whole transfer.
Added TestResponseWriteTimeoutDoesNotTruncateSlowButSteadyTransfer, which drip-feeds a 2 MiB response slowly enough that the full transfer takes well over the configured timeout. First version of the test passed even against the old single-deadline code, because 2 MiB fit entirely in default OS socket buffers and the write never actually stalled — had to shrink the client's receive window to force genuine backpressure before it actually caught the bug. Confirmed it now fails against the single-deadline version and passes with the per-chunk fix.
Kept the docs/option framing as an idle timeout rather than watering it down, since with this fix that's now what it actually is.
… write @alexandre-daubois caught this: SetWriteDeadline bounds the whole Write() call it wraps, not idle time within it. With output_buffering=0, PHP hands an entire echo() to go_ub_write in a single call, so a single deadline set before that call turns "idle timeout" into "finish transferring this buffer within N seconds" - a large-but-steady transfer to a slow (not stalled) client would be truncated via php_handle_aborted_connection() for merely taking longer than the timeout, not for actually stalling. Exactly the failure mode nginx's send_timeout docs call out by name, and why Caddy ships write_timeout unset. Fix: slice the write into responseWriteChunkSize (64 KiB) pieces and reset the deadline before each one, mirroring go_read_post's existing per-read reset. Bounds a stall, not a large transfer. TestResponseWriteTimeoutDoesNotTruncateSlowButSteadyTransfer drip-feeds a 2 MiB response slowly enough that the full transfer takes well over the configured timeout, with the client's receive window shrunk so the server's writes genuinely backpressure on that pace (otherwise 2 MiB fits in default OS socket buffers and never stalls at all, and the test verified nothing). Confirmed it fails against the single-deadline version and passes with the per-chunk fix.
dunglas
added a commit
to dunglas/caddy
that referenced
this pull request
Jul 29, 2026
SetWriteDeadline bounds the whole call it precedes, not just a stall within it. net.Conn.Write loops internally until a buffer is fully sent (unlike Read, which returns after one syscall), and ResponseWriter.ReadFrom hands the entire remaining source to the connection in one call. A single large Write, or any body copied via io.Copy triggering the ReadFrom fast path (http.ServeContent, static file serving), had its whole transfer bounded by one deadline, silently truncating a slow-but-healthy transfer exactly like a hard WriteTimeout would - the same bug found and fixed the same way in FrankenPHP's go_ub_write (php/frankenphp#2574). Cap each underlying call at 64 KiB and reset the deadline between chunks instead. net/sendfile.go special-cases *io.LimitedReader, so chunking ReadFrom still uses the sendfile fast path per chunk.
Member
Author
|
I'm tempted to close this one in favor of caddyserver/caddy#7913. Caddy users will be protected. Library users should implement a similar feature by themselves if they need one, but it's in line with our current policy (we don't want to re-implement a full web server in the FrankenPHP library). WDYT @php/frankenphp-collaborators? |
Contributor
|
I agree, these timeouts are better handled in Caddy, which controls the underlying net/http server. |
mholt
pushed a commit
to caddyserver/caddy
that referenced
this pull request
Aug 22, 2026
* caddyhttp: mitigate slowloris via idle read/write deadlines ReadTimeout and WriteTimeout previously applied as a single hard deadline over the whole body/response through http.Server, so any non-zero value also killed large transfers from legitimately slow clients. Reset the deadline on every successful read/write instead (via http.ResponseController), and give both a sane 1m default now that doing so no longer penalizes slow-but-progressing clients. * caddyhttp: split idle read/write timeouts from the existing hard ones Reworking ReadTimeout/WriteTimeout's own semantics was an unwanted behavior change for existing configs relying on the hard deadline. Leave them untouched and add ReadIdleTimeout/WriteIdleTimeout instead, reset on every successful read/write; both default to 1m since, being new, no existing config could have depended on a different value. Combining an idle timeout with its hard counterpart now gives the same base+ceiling shape as Apache's mod_reqtimeout, for free. * caddyhttp: cap idle-reset deadlines at the hard timeout ceiling Deadlines are a single absolute value on the connection, not a min of several: ReadTimeout/WriteTimeout's own hard deadline, set once by net/http before the handler runs, was silently getting overwritten by the first idle-reset Read/Write, voiding it entirely. Clamp the idle-reset deadline to the hard one when both are set, so combining them actually behaves like the advertised base+ceiling. * caddyhttp: add ReadMinRate/WriteMinRate, Apache MinRate equivalent Pure idle-reset alone doesn't bound a trickle that sends just enough to never go idle. ReadMinRate/WriteMinRate (bytes/second) grow the allowed deadline from a fixed start based on bytes transferred so far instead of resetting to a flat window on every call, so a transfer that doesn't sustain the configured rate falls behind real time and gets cut, matching Apache mod_reqtimeout's MinRate. Zero (default) keeps the existing flat idle-reset behavior unchanged. * caddyhttp: use named return and consistent blank lines in idleDeadline Matches the named-return style already used by ResponseWriterWrapper.ReadFrom. * caddyhttp: chunk idleTimeoutWriter's Write/ReadFrom, cap at 64 KiB SetWriteDeadline bounds the whole call it precedes, not just a stall within it. net.Conn.Write loops internally until a buffer is fully sent (unlike Read, which returns after one syscall), and ResponseWriter.ReadFrom hands the entire remaining source to the connection in one call. A single large Write, or any body copied via io.Copy triggering the ReadFrom fast path (http.ServeContent, static file serving), had its whole transfer bounded by one deadline, silently truncating a slow-but-healthy transfer exactly like a hard WriteTimeout would - the same bug found and fixed the same way in FrankenPHP's go_ub_write (php/frankenphp#2574). Cap each underlying call at 64 KiB and reset the deadline between chunks instead. net/sendfile.go special-cases *io.LimitedReader, so chunking ReadFrom still uses the sendfile fast path per chunk. * caddyhttp: export idle-timeout types, add configurable MaxWriteChunk Export IdleTimeoutReader/IdleTimeoutWriter/IdleDeadline so other packages (request_body next) can reuse the same idle-reset mechanism instead of reimplementing it, and turn the hardcoded 64 KiB write chunk size into a configurable MaxWriteChunk field defaulting to the same value - nginx's sendfile_max_chunk exists for the identical reason and is admin-tunable rather than fixed. * requestbody: idle-reset ReadTimeout/WriteTimeout, add MinRate/MaxWriteChunk ReadTimeout/WriteTimeout set a single deadline once, so any transfer running longer than the timeout got cut regardless of whether it was actually stalled - the same bug the server-wide timeouts had before switching to idle-reset. Reuse caddyhttp.IdleTimeoutReader/Writer here too, giving per-route granularity nginx/Apache have via location/ directory scoping and Caddy's server-wide timeouts don't: a route matching this handler can now set its own idle window independently from the rest of the server block. * caddyhttp: fold read/write min_rate into the idle-timeout directive Two directives per rate (read_body_idle + read_body_min_rate) for a value that's meaningless without the other. Fold min_rate into the idle-timeout directive as an optional second argument instead. * caddyhttp: split write pacing out of request_body into new timeouts handler request_body is a request-body concern (max_size, set); ReadTimeout/ WriteTimeout/MinRate/MaxWriteChunk pace both directions, and write pacing has nothing to do with the request body. Move all of it to a dedicated http.handlers.timeouts module instead, mirroring the server-wide timeouts option one level down.
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
4 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.
Context
Follow-up from review of #2570 and #2564 for #2553, and from #2573.
go_read_postalready bounds a stalled request body read with a deadline (#2538).go_ub_writehad no equivalent: a client that stops reading the response holds the handling thread in a blockingWrite()indefinitely.frankenphp_force_kill_thread()can't rescue it either — its own doc comment says it interrupts the Zend VM "at the next opcode boundary" or wakes a real blocking syscall via EINTR, but this stall is neither: Go'snet/httpwrites go through the non-blocking netpoller, so an unresponsive peer just parks the goroutine in Go's scheduler, invisible to a signal sent to the OS thread.This is the same mechanism behind #2573's
rebootAllThreads()fix, just hit from the request-handling side instead of the reboot side: a thread stuck here can wedge a reboot or shutdown exactly like the one #2573 addresses.Fix
Mirror
go_read_post's pattern: set an idle write deadline viahttp.ResponseControllerbefore eachgo_ub_writecall, driven by a new opt-infc.responseWriteTimeout. A deadline-exceeded write is treated as a genuine disconnect (php_handle_aborted_connection()fires), not silently retried.Exposed as:
WithResponseWriteTimeout()at the library level (zero disables it, matchingWithRequestBodyTimeout).response_write_timeoutin the Caddyfile, defaulting to 60s — same default and rationale asrequest_body_timeout(mirrors nginx'ssend_timeout).Documented alongside the existing slow-POST mitigation in
docs/security.mdanddocs/config.md.Test
TestResponseWriteTimeoutdrives a raw TCP connection that sends a request and never reads the response, so the server's socket send buffer fills and the underlyingWrite()blocks. Verified it fails (blocks past 5s) on the code before this fix, and passes in ~0.3s with it.Full
go test -race ./...(root +internal/...) and thecaddypackage suite both pass;go vet/gofmtclean.This narrows the real-world cases #2573's
Abandoned-state fallback needs to handle — any stalled HTTP/1.1 or HTTP/2 write now self-heals within the timeout — but doesn't replace it: it says nothing aboutflock(), a slow DNS lookup, a hung stream wrapper, or any other blocking call outsidego_ub_write, and can be configured off.