🐛 Set X-Accel-Buffering: no on JSONL streaming responses#15813
🐛 Set X-Accel-Buffering: no on JSONL streaming responses#15813torrresagus wants to merge 2 commits into
X-Accel-Buffering: no on JSONL streaming responses#15813Conversation
Server-Sent Events responses set `Cache-Control: no-cache` and `X-Accel-Buffering: no` so proxies (e.g. Nginx) deliver events incrementally instead of buffering the whole response. JSONL streaming responses are incremental in the same way, but were missing these headers, so a buffering proxy would hold back the lines and defeat the streaming. Set the same headers on the JSONL response for consistency with SSE.
|
Hello @torrresagus, thanks for the PR! I understand the point, but I'm not fully convinced yet. JSONL isn't 1 to 1 "SSE but in JSON", it's also used for plain bulk/streamed exports where caching can be legitimate. So I'd split the two headers:
In any case the picked behaviour should be clearly pointed out in the docs. |
|
Thanks for the careful review @luzzodev — that's a fair distinction, and you've convinced me. You're right that JSONL isn't semantically "SSE in JSON": it's also a transport for bulk/streamed exports where caching can be legitimate. And since
So I'll drop |
…g, document behavior Per review feedback: JSON Lines is not only a live event stream like SSE, it's also used for bulk/streamed exports where caching can be legitimate. `Cache-Control: no-cache` forces revalidation and would take away the `max-age` "serve from cache" option from those endpoints, so it shouldn't be imposed by default. Keep only `X-Accel-Buffering: no` (buffering defeats incremental streaming in every case) and leave caching headers to the user. Document the proxy buffering behavior in the JSON Lines tutorial.
📝 Docs previewLast commit ac21385 at: https://153935d9.fastapitiangolo.pages.dev Modified Pages |
|
LGTM! |
|
Thanks for the review @luzzodev! |
mohamedtaqysalmi
left a comment
There was a problem hiding this comment.
Mirroring the SSE anti-buffering behavior for JSONL is the right fix behind Nginx, and intentionally not imposing Cache-Control preserves legitimate export caching — the new docs section explains that well.
Small inconsistency: the PR title mentions both Cache-Control and X-Accel-Buffering, but
outing.py only sets X-Accel-Buffering. Either add the header for SSE parity or tighten the title/description so reviewers aren't looking for a header that isn't in the diff.
X-Accel-Buffering: no on JSONL streaming responses
|
Good catch @mohamedtaqysalmi — the title and description were stale after I dropped |
|
@tiangolo Let me know if you need anything else from me. |
What's wrong
FastAPI's Server-Sent Events responses set
X-Accel-Buffering: noso streaming survives a buffering proxy (theis_sse_streambranch infastapi/routing.py, added in #15030):The JSON Lines streaming responses (the
yield-basedapplication/jsonlstreaming added in #15022, theis_json_streambranch) stream items incrementally in the same way, but don't set it.Behind a proxy that buffers by default (e.g. Nginx with
proxy_buffering on), the JSONL lines are buffered and flushed together, which defeats the streaming: the client receives the200immediately, but the lines don't arrive until the proxy buffer fills. SSE isn't affected because it already sendsX-Accel-Buffering: no. The two features landed in separate PRs (SSE #15030, JSONL #15022), which is likely why only the SSE path got the header.Change
Set
X-Accel-Buffering: noon the JSONLStreamingResponse, so incremental streaming works through a buffering proxy the same way SSE does.Cache-Controlis intentionally not set: unlike SSE, JSON Lines is also used for bulk/streamed exports where caching can be legitimate, so the caching policy is left to the user (it can be set on theResponse). This is documented in the JSON Lines tutorial. (Thanks @luzzodev for the review that clarified this.)Tests
test_stream_disables_proxy_bufferingassertsx-accel-buffering: noacross the four JSONL streaming tutorial variants (parametrized like the existing SSE streaming tests).Context
Discussed first in #15794, where the same failure mode was confirmed independently, including a report behind Nginx in production.