Repository navigation
perf(http): don't add Vary header if response is not compressed - #33892
Conversation
fibibot
left a comment
fibibot
left a comment
There was a problem hiding this comment.
Substance reads correct as a perf optimization, but this is a behavior change to the default Deno.serve response shape — every uncompressed response now drops Vary: Accept-Encoding. One inline question on the cache-correctness trade-off the deleted comment was capturing. CI: 61 success, 34 in_progress, 0 fail — holding at COMMENT pending CI completion.
For reference, the legacy Deno.serveHttp path in ext/http/lib.rs:1161 still calls ensure_vary_accept_encoding(hmap) unconditionally. That's why tests/unit/http_test.ts proxy with fetch (which uses Deno.serveHttp) still asserts vary: Accept-Encoding and didn't need updating — only the Deno.serve spec fixtures (tests/specs/run/all_proxy/test.out, tests/specs/run/tunnel/test.out) needed the Vary lines stripped. Reasonable since serveHttp is legacy, but worth mentioning so the inconsistency is intentional rather than a forgotten site.
| return Compression::None; | ||
| } | ||
| ensure_vary_accept_encoding(headers); | ||
| let encoding = match compression { |
There was a problem hiding this comment.
The deleted comment at the (now removed) preceding block captured the load-bearing intent: Note: we set the header irrespective of whether or not we compress the data to make sure cache services do not serve uncompressed data to clients that support compression. That scenario is the one with concrete failure mode under the new policy — first request from a no-Accept-Encoding client populates a downstream cache without Vary, then a compression-capable client gets the cached uncompressed body for that URL until TTL.
In practice this is usually fine (the bandwidth waste recovers on TTL expiry; nginx and Apache by default also only Vary: Accept-Encoding when serving compressed) and the 1MB RSS win is real, but since the deleted comment was specifically calling out this trade-off, would you mind dropping a short note here explaining the new policy choice? Future readers shouldn't have to git-archaeology to understand why we no longer match RFC 9110's recommendation here.
…land#33892) Saves about 1MB in RSS under load, and in practice the thing it was guarding against isn't worth the hit
Saves about 1MB in RSS under load, and in practice the thing it was guarding against isn't worth the hit