Repository navigation
http3: closing or resetting the peer's control or QPACK stream ends the connection with H3_CLOSED_CRITICAL_STREAM instead of being ignored - #382
Open
MDA2AV wants to merge 1 commit into
Conversation
…he connection with H3_CLOSED_CRITICAL_STREAM instead of being ignored
This branch has not been deployed
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
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.
Fixes #351.
What was wrong. ioxide.http3 kept serving after the client closed or reset one of its critical streams. RFC 9114 6.2.1 makes closing a control stream "at any point" a connection error of type H3_CLOSED_CRITICAL_STREAM. RFC 9204 4.2 says the same of the QPACK encoder and decoder streams. Two paths swallowed it:
FeedUnitreated the control stream's FIN as the end of its record (_unis.Remove) and kept serving. A QPACK stream's FIN did not even get that far: the non-control branch returned first.Feed's lifecycle branch dropped the record on a RESET_STREAM the same way.What changed.
UniStreamgets one bool,IsCritical, set when the stream type is read: control (0x00), QPACK encoder (0x02) or decoder (0x03). Two places now callFatal("critical stream closed", H3_CLOSED_CRITICAL_STREAM)for a critical stream:FeedUni;Feed.So the run loop ends and
Close(0x104)reaches the peer. Theif (fin) _unis.Remove(sid)at the end ofFeedUniis gone. Only the control stream ever reached it, and its FIN is now fatal before that point.A reserved-type (grease) stream may still close or reset. So may a stream that ends before its type arrives, which RFC 9114 6.2 requires tolerating.
Proof.
H3ErrorCodeTestshad 17 tests. Its critical-stream group moves to a newH3CriticalStreamTests, which leaves it at 15. The new file reuses the recording transport:RecordingQuic,StartPure,AssertClosedWith,Served,WellFormedGetand the code constant go fromprivatetointernal.http3/codes: closing the peer's control stream is a connection error H3_CLOSED_CRITICAL_STREAM: the Pending, promoted.http3/codes: resetting the peer's control stream is a connection error H3_CLOSED_CRITICAL_STREAM: new.http3/codes: closing or resetting a QPACK encoder or decoder stream is a connection error H3_CLOSED_CRITICAL_STREAM: new, four cases.control - a client control stream is parsed and requests still serve(moved).control - a uni stream of another type may close or reset, and requests still serve(new). It covers a grease FIN, a grease reset, and a FIN inside the type varint; the GET is still served and nothing is closed.h3/codes: nghttp3 - closing or resetting the peer's control stream ends the connection with H3_CLOSED_CRITICAL_STREAM: pins the other stack (below).Run both ways:
expected the connection to close with H3_CLOSED_CRITICAL_STREAM (0x104), got no Close at all. Both controls and the nghttp3 test pass.protocol error: critical stream closedlines, all from those tests. No other test produced one, so a normal connection teardown does not trip it.nghttp3 was already right, and this pins it. Its
read_streamreturnsNGHTTP3_ERR_H3_CLOSED_CRITICAL_STREAMfor a FIN on any of the three streams. For a reset,Nghttp3Connectionpasses on the Closed item that follows it, andnghttp3_conn_close_streamrefuses any typed uni stream with the same error. Both reachFailProtocol, which closes with 0x104.Suites.
ioxide.http3builds for net10.0 and net11.0 with no warnings.Overlap. Merges cleanly with #383 (issue #350). Both touch
Http3Connection.cs, in different hunks. #381 (issue #352) also touchesH3ErrorCodeTests.cs: it adds an nghttp3 ALPN test there, while this PR moves the critical-stream group out and makes the shared helpers internal. The two still merge cleanly, andH3ErrorCodeTestsends at 16 tests once both land. Checked withgit merge-treeagainst every other PR of this batch (#378, #379, #381, #376, #377, #380): all merge cleanly. #350 + #351 + #352 merged together build, and E2E passes there: 305 passed, 0 failed, 2 pending (the #195 and #349 tests).All eight together. This batch's PRs (#376 to #383) were merged onto main 71aab91 in one scratch tree. The only conflict is the binary
libioxide_ngtcp2.sobetween #376 and #377, rebuilt there from the merged shim with the script's defaults. Everything builds, and Unit 102, E2E 309, Http 88, Chaos 80, Tls 169 (14 kTLS cases skipped) and File 5 pass, with 0 failed and 0 pending. The repo's only pending test left is the Redis suite's (#336).Noticed, not changed: