Negotiate gzip on the response content type - #51
Conversation
Coverage Report for CI Build 32197728184Coverage increased (+0.04%) to 97.493%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
4931c7c to
c220bee
Compare
c220bee to
eb07e9f
Compare
|
this one is fine on its own, the response-content-type negotiation is the right fix and eb07e9f covers the edge cases well, 1xx passthrough, already-encoded bodies, 206 and Content-Range, and the panic path. problem is it's based on |
021ccba to
de7a045
Compare
de7a045 to
100c791
Compare
|
Rebased and the edge cases from your note are covered, plus three more found on re-review: 101 was being treated as an interim 1xx so the deferred close could append gzip bytes after a protocol upgrade, 205 was not excluded, and sniffing ran before the On retargeting: GitHub refuses it with |
|
#49 is merged, and #52 too, so nothing is blocking this one any more. GitHub retargeted it to master on its own. it still shows conflicting though. Your 23:24 rebase was onto the three extra cases you caught on re-review are the right ones, particularly 101 being treated as interim so the deferred close could append gzip bytes after an upgrade. |
Previously the middleware picked compression from the request Content-Type, which a normal GET does not send, so it fell back to application/octet-stream and skipped compression for exactly the JSON and HTML responses it exists to compress. A request that did carry a matching type could equally get an unrelated binary response compressed. The old tests set a request Content-Type, which is why they passed. The decision now waits until the response content type is known, taken from the handler's header or sniffed from the first chunk when it set none. Along the way: Accept-Encoding is parsed so gzip;q=0 is honoured, bodyless 204 and 304 responses are left alone, Content-Length is dropped only when the body is actually compressed, and Flush, Hijack and Unwrap pass through so streaming and protocol upgrades keep working.
Six defects in the new writer, each with a test that fails without the fix: Flush before the first write committed the headers while the compression decision was still open, so a later textual write went out gzipped under identity headers. The decision is now settled before the headers leave. A panicking handler was committed as 200 by the deferred close, which left an outer Recoverer unable to replace it with a 500. An uncommitted response is now left alone when the handler did not return normally. A response the handler had already encoded was compressed again and relabelled gzip, and a 206 was compressed although its Content-Range offsets describe the uncompressed representation. Both are skipped now. WriteHeader let a second call replace the status when the first could not commit for want of a content type, where net/http keeps the first. Interim 1xx responses pass straight through, so Early Hints no longer becomes the final status. Accept-Encoding gave a wildcard precedence over a named gzip entry, and read only the first header field, so "gzip;q=0" split across fields or ordered after a wildcard read as acceptance.
101 was caught by the interim 1xx passthrough, so the deferred close could append gzip bytes after the connection had been handed to another protocol. It counts as final now, and along with 205 it is excluded from compression as neither carries content. Sniffing also ran before the Content-Encoding check, labelling an already encoded body with a type guessed from its compressed bytes, which net/http suppresses.
100c791 to
ebf24f8
Compare
|
found a gap in the 101 handling after this merged, worth a follow-up.
narrow in practice, gorilla/websocket hijacks and writes the raw 101 itself rather than calling fix looks like committing 101 immediately regardless of content type, with a regression test that omits |
WriteHeader only committed when a Content-Type was already set, so the plain upgrade sequence, 101 with no content type followed by Hijack, recorded the status and never forwarded it: Hijack set hijacked and close returned at that guard, leaving the client with the raw protocol bytes and no status line where net/http would have written one. 101 now commits immediately, which it can do because it carries no body to sniff and is already excluded from compression. The regression test omits Content-Type and reads the raw socket, so it fails without the fix. Reported by umputun on #51.
|
Good catch, confirmed and fixed in #55 (96800ae), which was already open against the same Reproduced exactly as you described: with no The regression test omits |
WriteHeader only committed when a Content-Type was already set, so the plain upgrade sequence, 101 with no content type followed by Hijack, recorded the status and never forwarded it: Hijack set hijacked and close returned at that guard, leaving the client with the raw protocol bytes and no status line where net/http would have written one. 101 now commits immediately, which it can do because it carries no body to sniff and is already excluded from compression. The regression test omits Content-Type and reads the raw socket, so it fails without the fix. Reported by umputun on #51.
3 participants
Stacked on #49, which this branch is based on, so the diff here is the gzip change alone. The base retargets to master once #49 merges.
Gzippicked compression from the requestContent-Type. A normalGETdoes not send one, so the lookup fell back toapplication/octet-streamand compression was skipped for exactly the JSON and HTML responses the middleware exists to compress. In the other direction, a request that happened to carry a matching type could get an unrelated binary response compressed.The middleware was therefore close to a no-op in ordinary use. Wrapping a handler that renders JSON and requesting it with
Accept-Encoding: gzipreturns an uncompressed body on master. The existing tests passed only because they set a requestContent-Typebefore each call, which encoded the faulty contract; two of their assertions changed here and are the reason.The decision now waits until the response content type is known, taken from the header the handler set or, when it set none, sniffed from the first chunk of the body with
http.DetectContentType. The public signature is unchanged, and the default content-type list is the same.Some smaller corrections came with it:
Accept-Encodingis parsed into tokens rather than substring-matched, sogzip;q=0is honoured as a refusal instead of being read as a request for gzip.Content-Encoding.Content-Lengthis dropped only when the body is actually compressed. Previously every response passing through the writer lost it, including uncompressed ones.Flush,HijackandUnwrapare implemented, so streaming responses, protocol upgrades andhttp.ResponseControllerkeep working through the wrapper. A hijacked connection also suppresses the deferred header write, which would otherwise logWriteHeader on hijacked connection.What changes for callers. Nothing at the API level, but responses that were silently going out uncompressed will now be compressed, which is what wrapping a handler in
Gzipasks for. Anything asserting on exact response sizes will see different numbers.Note for whoever runs the suite:
TestBenchmarks_Handleris timing-sensitive and flakes under load on an unrelated assertion,assert.InDelta(t, 50000, res.MaxRespTime, 10000), which requires a 50ms sleep to finish within 60ms. It reproduces on master and with the gzip code not exercised at all, roughly one run in ten on a busy machine. Not addressed here, but worth a look separately.