Repository navigation
HTTP/2 SETTINGS as a live negotiation; websocket Received step contract (#87, #86) - #100
Merged
Merged
Conversation
`Connection` owned SETTINGS state that only the host knows the policy for, and five of the six gaps Perry's node:http2 lane hit forced a host to interfere with it. Outstanding SETTINGS are now a queue. RFC 9113 section 6.5.3 acknowledges frames in order, and a single `settings_awaiting_ack` flag turned the second ack of two outstanding frames into "unsolicited SETTINGS ack" and failed the connection - so `session.settings()` was unimplementable. An ack with nothing queued is still PROTOCOL_ERROR. Each ack is now `Event::SettingsAck(Settings)`, carrying the parameters it acknowledged. A host that timestamps each send pairs them in FIFO order and has its round trip; no clock is read here. The ack clears the host's SETTINGS deadline, since that deadline was for the frame just acknowledged. `Event::Settings` carries a `SettingsFrame` borrowed from the input: the peer's parameters in wire order, duplicates and unknown identifiers included, with `get` returning the last (in-force) value. `remote_settings()` and `local_settings()` accumulate what each side has put in force. `Event::Goaway` carries `debug: Option<&[u8]>`: `None` when the frame had no opaque data, which Node reports as `undefined`. The wire cannot express "present but empty", so it is never `Some(&[])`. `Connection::with_settings(role, limits, &Settings)` sends exactly the host's initial SETTINGS - `Settings::new()` gives the empty frame Node's server sends - serialised in ascending identifier order, as Node does. `Connection::new` is `with_settings` of `Settings::from_limits`, so a server's bytes are unchanged and a client's differ only in ENABLE_PUSH moving to the front. A client that names no ENABLE_PUSH still sends ENABLE_PUSH=0: push defaults to on and PUSH_PROMISE is a connection error here. `Connection::settings(&Settings)` sends a change to a live connection and applies it. A loosening (larger MAX_FRAME_SIZE, INITIAL_WINDOW_SIZE, MAX_HEADER_LIST_SIZE, MAX_CONCURRENT_STREAMS) takes effect at once, because the peer may act on it before its ack reaches us; a tightening takes effect on the ack, so nothing the peer sent under the old value is treated as an error. INITIAL_WINDOW_SIZE moves every open stream's receive window by the difference (section 6.9.2), and new streams start at the new size - the per-stream receive window was a hard-coded 65535 before. `limits()` reports what is enforced. This is a method rather than a `Limits` setter because the wire-visible limits cannot change without telling the peer; values this endpoint cannot honour (ENABLE_PUSH=1, HEADER_TABLE_SIZE above the decoder's fixed 4096, out-of-range values) are refused before anything is sent. Breaking: `Event::Settings` is a tuple variant, `Event::Goaway` has a third field, and `Event::SettingsAck` is new; the SETTINGS ack is no longer a no-event step, so the step-contract docs and the shape test say so. `asynchronous::client`'s two GOAWAY patterns gain `..`. Tests in tests/http2_settings.rs cover each item: three outstanding frames acked in order, the ack event and deadline, the peer frame's contents, GOAWAY debug data present and absent, empty and ordered initial frames, refused values, and live frame-size, stream-limit and initial-window changes applied before and after the ack. h2spec strict: 147 tests, 147 passed. Closes #87
`Received` had the same undocumented independence of `consumed` and `message` as turnloop-http's `Step`, but measuring it showed a different shape from the one the issue predicted, so the contract is written from the measurement. `receive` hands input to tungstenite, which reads it into its own buffer and parses from that buffer before reading again. So: - `consumed == 0` with a message is normal: a second frame that arrived in the same read completes on the next call, from bytes already consumed. "consumed == 0 means wait for input" is therefore wrong, and a host that loops while `consumed > 0` drops that message outright - it never looks at the step. This is the HTTP/1 `Event::End` shape (#50), not HTTP/2's. - `consumed > 0` without a message is a partial frame or non-final fragment, and all of the input has been taken in. Calling again returns the stop shape. - A host that reads the transport whenever its input is empty stalls on the first case: nothing is left to hand in, yet a message is waiting. - Looping while a message came back does *not* stall for this type, unlike `http2::Step` at the preface: a step with no message always took all of its input, so nothing is stranded. The test pins that, since it rests on how tungstenite reads. The documented loop condition is the workspace one, `consumed > 0 || message.is_some()`, now on `Received` with a table and a doctest. Tests: the measured shapes (23/Some, 0/Some, 0/None; 6/Some; 5/None, 0/None), the loop-on-consumed host losing "two", the read-when-empty host finding a message waiting after it would have blocked, and the loop-on-message host seeing all three. Idempotence, as the issue asked: `receive` is not idempotent, and the failure is worse than HTTP/2's. Re-feeding undrained input does not fail the connection; a complete message in it is silently delivered twice. Documented and tested. The Node interop test's own server loop read the socket whenever its input was empty - the stall above - and now reads only when a step made no progress. `asynchronous::WebSocketStream::receive` already calls `receive` before every transport read, so it was correct. Closes #86
|
Warning Review limit reachedNext included review available in 11 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 (7)
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 |
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.
#87: SETTINGS is a negotiation, not a constructor argument
All six items, in
protocols/turnloop-http/src/http2.rs:Event::SettingsAck(Settings)fires when the peer acknowledges. Acks come back in send order, so a host can timestamp each send and compute the round trip. The crate still reads no clock.Event::Settings(SettingsFrame<'a>)carries the peer's frame as sent.remote_settings()andlocal_settings()report what is in force on each side.Event::Goawaygainsdebug: Option<&[u8]>, which isNonewhen the frame had no opaque data (Node'sundefined).Connection::with_settings(role, limits, &Settings)sends exactly the host's initial frame, including an empty one, in ascending id order.Connection::settings(&Settings)changes settings on a live connection. A change that loosens a limit applies immediately; one that tightens waits for the ack. INITIAL_WINDOW_SIZE shifts every open stream's receive window.limits()reports what is currently enforced.h2spec: 147/147.
#86:
turnloop_websocket::ReceivedMeasured behaviour differs from what the issue predicted. tungstenite buffers input, so
consumed == 0with a message is normal. A host that loops whileconsumed > 0loses that message rather than spinning, and a host that reads the transport whenever its input is empty stalls. Looping while "a message came back" is correct for this type. All of this is documented onReceivedwith a table and a doctest, and pinned by three tests. The Node interop test's own server loop had the read-when-empty stall and is fixed.receiveis not idempotent: feeding the same input again delivers a complete message twice, now documented.Breaking changes
Event::Settingsis now a tuple variant,Event::Goawayhas a third field, andEvent::SettingsAckis new.Checks
Local: fmt,
cargo test -p turnloop-http --all-features,cargo test -p turnloop-websocket --all-features, and h2spec 147/147 pass. The workspace clippy, doc and MSRV checks were cut short when the disk filled up; CI covers them.Closes #87
Closes #86