Repository navigation
HTTP/1 step and encoder gaps, and Content-Encoding lists and step reasons in compression - #104
Merged
Merged
Conversation
`State::Upgrade` was reachable only in `Mode::Response`. A server decoding `GET / HTTP/1.1` with `Connection: upgrade` saw an ordinary head and `End`, so a host that matched on the public `Event::Upgrade` - the obvious thing to match on - served every WebSocket handshake as a normal request. Perry's P5 lane found the asymmetry only by reading the decoder. Of the two fixes the issue offers, this takes the first: the enum now means one thing in both modes. Documenting the asymmetry would have kept a public variant that is unreachable in the mode most servers run in, and every server would still have to re-derive the same RFC 9110 7.8 test from the headers. In `Mode::Request` a request ends with `Event::Upgrade` in place of `End` when it is HTTP/1.1 with an `Upgrade` header and the `upgrade` token in `Connection`, or when it is a CONNECT - the request-side mirror of the 101 and CONNECT-2xx cases the response side already had. It is raised after the request body, if any, and like `End` it reads no input, so the next protocol's bytes stay in the host's buffer. An `Upgrade` on an HTTP/1.0 request is ignored, as 7.8 requires. The decision stays with the caller. Unlike the response side, the decoder does not give up the connection: after a request-side `Upgrade` it is `reusable` on the same terms as after `End`, so a server that declines answers normally and calls `reset`. `Event::Upgrade` and `Mode` now say all of this. The in-crate drivers follow. `asynchronous::Http1` counts a request-side upgrade as the end of the message for `reusable`, and `reset` clears it. `server::http1` treats `Upgrade` as the end of the request; a service that declines keeps the connection, and one that answers 101 (or 2xx to CONNECT) now closes it, because that driver cannot hand the transport off and would otherwise parse the next protocol's bytes as a request. turnloop-websocket's `accept` already accepted `Upgrade | End`. Behaviour change: a request-mode host that matched only `End` must now also handle `Upgrade` for upgrade and CONNECT requests. Tests: `http1_request_side_raises_upgrade` (codecs) checks the event, its zero consumption, the untouched trailing bytes, body-then-upgrade, CONNECT, reuse after a decline, and the three non-upgrade shapes; before the fix every upgrade case ended in `End`. `http1_server_upgrade_request_is_the_services_decision` (asynchronous) drives `server::http1` over TCP: a declined upgrade serves the next request on the same connection, and an accepted one closes without parsing the bytes that follow - on the old code the service saw `End` and the driver failed parsing "not HTTP at all" as a request. The interop CONNECT proxy fixture now stops on `Upgrade` as well as `End`. Closes #46
Three response shapes Node puts on the wire could not go through the encoder,
and Perry's P5 lane wrote the head by hand for each:
- `Encoder::start` always wrote the canonical reason phrase, so
`res.writeHead(404, "Nope")` meant patching the status line afterwards by
re-finding the CRLF in the encoder's own output.
- `BodyLength` had no close-delimited framing.
- A HEAD response advertises the `Content-Length` it would have sent and sends
no body. `Known(0)` rejected that head as a conflict with the header, and
`Known(n)` refused to `finish` without n bytes.
Now:
- `Encoder::start_with_reason(head, reason, body, out)` writes the given
phrase. It is validated against RFC 9112's reason-phrase grammar (tab, space,
visible ASCII, obs-text) before anything is written, so a phrase cannot inject
a header line, and it is refused on a request. `start` is unchanged. A
separate constructor rather than a field on `Head` keeps every existing
`Head { .. }` literal compiling, and a decoded head has no reason to carry.
- `BodyLength::CloseDelimited`: responses only, no framing header, body bytes
written raw, `finish` writes nothing and the host's close ends the body. A
head with `Content-Length` or `Transfer-Encoding`, a request, and a
1xx/204/304 status are refused, as are trailers.
- `BodyLength::Omitted`: responses only; the head's framing headers go out
verbatim, none is added, and no body follows. That is a response to HEAD
(length or chunked coding advertised) and a 304. A 204 or 1xx may not
advertise a length at all (RFC 9110 8.6), so a framing header there is still
refused. Of the issue's two shapes for this - a variant or a `head_response`
flag on `start` - the variant fits: body framing is exactly what `BodyLength`
already chooses, and a flag would be a second argument that changes what the
first means.
Breaking: `BodyLength` gains two variants, so an exhaustive `match` on it needs
two more arms. No in-tree match was exhaustive.
Tests: `http1_encoder_writes_a_custom_reason_phrase`,
`http1_encoder_writes_a_close_delimited_body` and
`http1_encoder_writes_a_body_forbidden_response` assert the exact wire bytes,
decode the close-delimited and HEAD responses back with `Decoder` (a body ended
by `eof`, and a HEAD response that ends at its head and stays reusable), and
check every refusal leaves the output untouched.
Closes #47
`Event::End` (and `Event::Upgrade`) come from a step that consumes nothing: the step that consumes the last byte of a message only moves the decoder to `End`, and the next call delivers it. A host that feeds the decoder "while there is input" hands over every byte and stops, so the message sits finished inside the decoder. Two lanes lost a debugging cycle to it: P6's client took ~5 s per fetch with no connection reuse, and P11's first real-server probe timed out on every URL - the peer's idle timeout was the only thing ending the exchange. #85 stated the `Step` contract (loop while `consumed > 0 || event.is_some()`) in the crate root and on both `Step` types. The issue's comment argues that two independent consumers hitting the same trap is the case for a predicate over another note, and a host keyed on input rather than on steps has no reason to read the loop condition anyway. So both: - `Decoder::wants_step()` is true exactly when a zero-input event is pending (`State::End` or `State::Upgrade`): after the last body byte or final chunk, after the head of a bodiless message, and after `eof` ends a close-delimited body. `while !input.is_empty() || decoder.wants_step()` is a correct loop. - `http1::Step`'s documentation now carries a runnable loop, as `http2::Step`'s does, and a paragraph on the zero-consumption trap naming `wants_step`. The crate-root contract names the "while it has input" loop beside the "while consumed" one. Tests: `http1_end_needs_a_step_after_the_last_byte` runs the loop both lanes wrote over a Content-Length, a chunked and a 204 response, and asserts it consumes every byte yet never sees `End` and leaves the decoder unreusable - the P6 symptom - while `wants_step` is true. The same loop keyed on `wants_step` too reaches `End` from a zero-consumption step, leaves the decoder reusable, and ends with `wants_step` false. It also covers `eof` on a close-delimited body and a request-side upgrade. The existing `step_contract_rejects_both_wrong_loop_conditions` still covers the "stop when nothing was consumed" loop. Closes #50
The only way to learn whether a zero-byte read was a legal end of body or a truncated response was to call `eof()` and see whether it failed - and `eof()` commits: on the wrong answer it has already failed the decoder. Perry's P11 client needs the answer on every connection, before choosing between retrying, failing with its own diagnosis, or logging. The issue offers a `wants_step()`-style predicate or an explicit query. `wants_step` (#50) answers a different question - whether an event is owed - so this adds the explicit pair, both read-only: - `is_mid_message()`: a final head has been decoded and the end of the message has not been reached (fixed-length or close-delimited body, chunk framing, trailers). An informational response is not the message. - `eof_is_clean()`: true exactly where `eof()` would return `Ok` - inside a close-delimited body and once the message is complete. `eof()` is now written in terms of it, so the two cannot drift. Two predicates rather than one because the question P11 asks has three answers: not in a message, in a message where EOF is truncation, and in a close-delimited body where EOF is the end. Test: `http1_mid_message_is_queryable_without_feeding_eof` walks a Content-Length, a close-delimited and a chunked response through each state and asserts both answers, that asking changes nothing (the body continues to decode afterwards), that a 100 Continue is not mid-message, and that `eof_is_clean() == false` is followed by `eof()` failing. Closes #80
`StreamingDecoder::process` returned after a single engine call, which can stop anywhere: after a gzip header, at a member boundary, when the output filled, or when the input ran out. Every case looked the same - a step that consumed and wrote something - so a host could only loop until a step did neither. Perry's P6 client does exactly that, and so has no signal for "give me a bigger buffer" versus "feed me more bytes". `process` now runs engine steps until one of three reasons holds, and `DecodeStep` gains `needs_input` to say which: - `finished`: the body is complete. - `needs_input`: every input byte the decoder can use is used; read more, or pass `end`. - neither: `output` is full (`written == output.len()`); drain and call again. The issue asks for a flag rather than an enum, and a flag is enough: with `finished` it separates all three cases, and it keeps `DecodeStep` a plain struct of counts. Running to a stop reason inside `process` is what makes the flag true to its name - a single engine call can stop with input left and space free, which is neither - and it means one call per transport read and one per full buffer, with no no-progress call at the end. Existing loops keep working: their extra call now returns `0, 0, needs_input`. `decode` uses the flag for its truncation check instead of the both-zero test. Breaking: `DecodeStep` has a new public field, so a struct literal of it outside the crate needs `needs_input`. Test: `streaming_decoder_says_whether_it_needs_input_or_output_space`, for gzip, deflate, brotli and zstd (both zstd backends: default and `--all-features`). With all input and a 4-byte buffer, every step but the last writes exactly 4 bytes with `needs_input` false, in at most len/4 + 1 calls; fed one byte at a time into a 1000-byte buffer, every step but the last reports `needs_input`, and the last `finished`. On the old code the first half fails: the gzip header step returns with nothing written and space free. Closes #54
The native zstd engine set `boundary` from every `run_on_buffers` result. A call that moves nothing - empty input after the last byte of a frame - reports the header size of the *next* frame as `remaining`, so it cleared a boundary that had just been reached. The following `end` then looked like a truncated frame and failed a complete body. A host hits this whenever it learns the body is over after feeding the last byte, which is the normal HTTP/1 shape: the last body bytes arrive in one step and `End` in the next, zero-byte one (#50). The previous commit's run-to-a-stop-reason loop makes the empty call itself, so it made the bug reachable from a single `process` per read; a host that looped until no progress could already reach it. Only a call that consumed or wrote something now moves the boundary. Test: `streaming_decoder_finishes_when_end_follows_the_last_byte` feeds each coding byte by byte without `end`, makes an idle call, then declares `end` with no input. Native zstd failed it with UND_ERR_SOCKET before this change; the pure-Rust zstd path, gzip, deflate and brotli pass either way. Refs #54
`StreamingDecoder::new` matched its argument as one exact token, so a legal `gzip, gzip` - or two `Content-Encoding` header lines - failed the whole response with UND_ERR_NOT_SUPPORTED. Perry's P11 client is the first consumer to send its own `Accept-Encoding`, and an independent review of it found the gap; it now splits and chains on the caller side. The issue offers an iterator of tokens or split-and-chain internally. This does both, because they are the same thing at two call shapes: - `StreamingDecoder::new(value, limit)` takes one header value, list included. - `StreamingDecoder::from_codings(values, limit)` takes every header line's value in order and merges them as RFC 9110 5.3 merges any list-valued field. `Head::values(name)` yields those lines (`get` returns only the first). Codings are decoded in reverse of the order listed: the last-listed is outermost. Names are case-insensitive (RFC 9110 8.4.1), `x-gzip` is `gzip`, and `identity` and empty list elements are skipped. An unknown coding still fails with UND_ERR_NOT_SUPPORTED, and so does a chain longer than `MAX_CODINGS` (5): the list is attacker-chosen and each coding costs state and a buffer; undici stops at five for the same reason. A single coding is unchanged: it decodes straight into the caller's output, and the chain vector stays empty and unallocated. Each further coding decodes into a fixed 16 KiB staging buffer - over the 8 KiB a gzip header may take, so the next coding always sees a whole header - allocated at construction and kept across `reset`, so steady-state reuse stays allocation-free. A chained step keeps the #54 contract: it returns only when finished, when it needs input, or when the output is full. `limit` bounds every coding's output, not only the last: an outer coding could otherwise expand without bound into an inner one that produces nothing (empty gzip members, for instance), costing CPU with no decoded bytes to trip the limit. Internally the per-coding state moves into a `Codec`, and `StreamingDecoder` owns the innermost one plus the staged outer ones. Tests: `streaming_decoder_applies_a_content_encoding_list_innermost_last` decodes a 80 KB `gzip, br` body whole, byte-by-byte into 7-byte output, and in other input/output splits, before and after `reset`; the same from two header lines through `Head::values`; the reversed order fails; `gzip, gzip` under case, `x-gzip`, identity and empty elements, through the convenience `decode`; every coding under gzip, deflate and brotli (zstd included) in four input/output splits; truncation; the decoded-size limit; unknown codings and a six-coding chain refused, five accepted. `streaming_decompressors_reuse_scratch` gains a `gzip, br` case and still measures zero allocations across 100 reused bodies. All pass with the native and the pure-Rust zstd backends. Closes #79
|
Warning Review limit reachedNext included review available in 21 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (11)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
asynchronous::client::Decoders::start read only the first Content-Encoding line, so a response carrying `gzip` on two separate lines was decoded once and handed the caller still-gzipped bytes. RFC 9110 5.3 makes repeated lines one list; StreamingDecoder already takes a list, so the lines are joined into the decoder's cache key. The single-line case still borrows the header value. The new unit test decodes a body gzipped twice, once with two header lines and once with one `gzip, gzip` line, and fails without the change. Refs #79
…sion # Conflicts: # protocols/turnloop-http/README.md
This was referenced Sep 22, 2026
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
1 participant
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.
Six
turnloop-httpissues inhttp1andcompression, one commit each, plus one commit for a zstd bug that the #54 change made easier to reach. Every behaviour change has a test that fails on the old code.#46:
Event::Upgradeon the request sideOf the two fixes the issue offers, this takes the first. In
Mode::Request, an HTTP/1.1 request with anUpgradeheader and theupgradetoken inConnection, or anyCONNECT, now ends withEvent::Upgradein place ofEnd. It comes after the request body, reads no input, and leaves the bytes that follow in the host's buffer. The caller still decides. The decoder staysreusablejust as it would afterEnd, so a server that declines can answer normally andreset. RFC 9110 7.8 says anUpgradeon HTTP/1.0 is ignored, so it is.Event::UpgradeandModenow document both modes. The in-crate drivers follow this:asynchronous::Http1::reusableandresetnow account for a request-side upgrade.server::http1ends the request onUpgrade. If the service answers101, or2xxto a CONNECT, the connection is closed rather than read as HTTP/1.acceptalready acceptedUpgrade | End.Closes #46
#47: reason phrase, close-delimited body, body-forbidden responses
Encoder::start_with_reason(head, reason, body, out)writes a custom reason phrase. The phrase is checked against RFC 9112's grammar, so it cannot inject a header line, and it is refused on a request.BodyLength::CloseDelimitedis for responses only. It writes no framing header, and the body ends when the host closes the connection.BodyLength::Omittedis for responses only. The head's ownContent-LengthorTransfer-Encodinggoes out as given and no body follows: a HEAD response or a 304. A 204 or 1xx that advertises a length is still refused (RFC 9110 8.6).Closes #47
#50:
Event::Endfrom a zero-byte stepDecoder::wants_step(). It is true exactly while a zero-inputEndorUpgradeis pending, sowhile !input.is_empty() || decoder.wants_step()is a correct loop.http1::Step's docs now include a runnable loop in the same style ashttp2::Step(A terminated HTTP/2 stream no longer takes the connection with it #85), plus a paragraph on the trap.End, and leaves the decoder unreusable. The same loop keyed onwants_stepcompletes all three.Closes #50
#80: asking whether the decoder is mid-message
Two new read-only queries:
Decoder::is_mid_message(): a final head has been decoded and the end of the message has not been reached.Decoder::eof_is_clean(): true exactly whereeof()would succeed.eof()is now written in terms of it, so the two cannot drift.Together they separate three cases: not in a message, truncated if EOF comes now, and inside a close-delimited body where EOF is the legal end.
Closes #80
#54: needs input vs output full
processnow keeps running engine steps until the body is finished, the decoder needs input, or the output is full.DecodeStepgainsneeds_input, which together withfinishedtells the three apart. So there is one call per transport read and one per full buffer, with no final no-progress call. Existing "loop until nothing happens" hosts still work.A follow-up commit fixes the native zstd engine. It lost its frame boundary on an empty step, so an
enddeclared after the last byte failed a complete body. That is the normal HTTP/1 shape, and the new loop makes such an empty step on its own. It has its own regression test.Closes #54
#79:
Content-EncodinglistsStreamingDecoder::newaccepts a list such asgzip, br.StreamingDecoder::from_codings(values, limit)takes every header line's value and merges repeated lines.Head::values(name)yields every line of a header.Codings are decoded innermost-last: the last listed is decoded first. Names are case-insensitive,
x-gzipis accepted, andidentityand empty list elements are skipped. A chain longer thanMAX_CODINGS(5, the same cap undici uses) is refused.A single coding behaves as before. Each extra coding stages its output in a fixed 16 KiB buffer that is allocated at construction and kept across
reset. The allocation test adds agzip, brcase and still measures zero allocations.limitnow bounds every coding's output, not only the final one.Closes #79
Breaking changes (pre-1.0)
http1::Mode::Request: upgrade and CONNECT requests now end withEvent::Upgradeinstead ofEvent::End. A request-side host that matched onlyEndneeds anUpgradearm.server::http1services seeUpgradefor these requests too.http1::BodyLengthhas two new variants,CloseDelimitedandOmitted, so an exhaustivematchneeds two more arms.compression::DecodeStephas a new public field,needs_input.compression::StreamingDecoder::processmay now do more work per call: it returns only at one of the three stop reasons.Not changed here
client.rsandasynchronous/client.rsbelong to a parallel change and are untouched.asynchronous::client::Decoders::startstill reads only the firstContent-Encodingline throughhead.get. With this PR, a list on that single line now decodes. Merging repeated lines there is a one-line change:from_codings(head.values("content-encoding"), ..), which that module's owner can make.Checks
cargo +nightly-2026-08-20 fmt --all --checkcargo +nightly-2026-08-20 clippy --locked --workspace --all-targets --all-features -- -D warnings -D clippy::undocumented_unsafe_blocksRUSTDOCFLAGS='-D warnings' cargo +nightly-2026-08-20 doc --locked --workspace --all-features --no-depscargo +nightly-2026-08-20 test --locked -p turnloop-http --all-features, plus--test codecs --test allocationswith default features for the native zstd backendcargo +nightly-2026-08-20 test --locked -p turnloop-websocket --all-featurespython3 scripts/test-servers.py --services http run -- cargo +nightly-2026-08-20 test --locked -p turnloop-http --all-features --test interop --test asynchronous -- --include-ignored --test-threads=1(30 passed, including the Node/curl fixtures and the CONNECT proxy)cargo +1.97.1 check --locked --workspace --all-targets --all-features