Conversation
|
I'm not sure if delaying the content write is a good change. It breaks as soon as a handler triggers sending the response, for example by writing directly to the response IO (e.g. with the Headers are supposed to be set before sending any content. So an after hook seems like a naturally bad place for setting headers and I'm questioning the use case for that. Supporting HTTP trailing headers could be worth considering, but that's a very niche use case. |
|
You're right that this can't cover handlers writing directly to the response IO. My motivation wasn't really to endorse setting headers in after hooks. It's the inconsistency that bothers me: the same That said, I'm not attached to the defer part. If you prefer, I can trim this down to the |
|
That sounds good. At least as a first remedy. We can still move forward with more changes if there's a strong use case for it. In order to remove the inconsistency, we might consider explicitly flushing the response before running after hooks. That might seem a bit harsh, but I believe it's correct. After hooks should not expect to contribute to the response. |
|
@straight-shoota updated the PR according to our discussion going with the simpler approach |
Co-authored-by: Johannes Müller <[email protected]>
Fixes #759
Problem
Changing response headers in an
after_allfilter raisesHeaders already sent (IO::Error)when the response body exceeds the 8KB output buffer, because Crystal flushes the headers to the socket mid-write. The exception handler made it worse by trying to render a 500 page, which also sets headers and raises again, producing an unhandled exception in the logs and a half-written response.Solution
Headers must be set before the body is written, so
before_*filters are the right place for header changes. This PR doesn't try to make header changes in after filters work (see the discussion below), it fixes the secondary crash instead:HTTP::Server::Response#headers_sent?and guardedrender_500, the custom error handler paths and the payload-too-large handler with it. When the headers are already on the wire, the exception handler now delivers the response as-is instead of crashing with a second exception.headers_sent?helper is available on all Crystal versions; the oldOutput#closeoverride is still only compiled on Crystal < 1.3.before_*filters and why.An earlier revision of this PR deferred writing the route's response body until the after filters had run, so header changes in after filters would work for any response size. That approach was dropped after review feedback: it can't cover handlers that write directly to the response IO (e.g.
send_file), so it would have fixed the inconsistency only partially. See the discussion with @straight-shoota below.Behavior changes