h3: tell the peer why an nghttp3 connection died, and use the codes RFC 9114 defines - #208
Merged
Conversation
…FC 9114 defines Closes #206. Nghttp3Connection had exactly one Close call, gated on `!_protocolFailed` - so a protocol failure could never reach it by construction. PushToEngine recorded the failure, the run loop broke, and the finally did DecRef and Dispose, neither of which puts anything on the wire. The client's request simply hung: the shim sets no max_idle_timeout, and the transport's 60 s sweep is refreshed by ANY inbound datagram including ones ngtcp2 discards, so a peer sending ACKs holds a dead connection open indefinitely. The pure-C# stack fixed this in 1214005; the stack the README calls its drop-in replacement did not get it. The code comes from nghttp3 rather than from a table here. It already owns the mapping - nghttp3_err_infer_quic_app_error_code knows which of its errors are H3_FRAME_ERROR, H3_MESSAGE_ERROR or QPACK_DECOMPRESSION_FAILED, and returns H3_INTERNAL_ERROR for what it does not recognise - so the shim gained a four-line export for it instead. Same principle as everywhere else here: we vendor these libraries, we do not re-derive what they already know. All ELEVEN sites that fail a connection are routed, not the two that were easiest to find. Four carry a library error and go through FailProtocol; five are our own failure - a control stream that would not open, a callback that threw - and take H3_INTERNAL_ERROR through FailInternal. The twelfth site is deliberately left alone, and that is the part worth reading. Streamed.cs sets _protocolFailed in a finally on a response that completed PERFECTLY WELL, purely to unpark waiters, because IsBroken is what makes them return rather than loop. Closing on that flag would have sent an error code to every client that received a streamed body successfully. So what the peer is told is tracked separately, as a nullable code, and the flag keeps meaning "stop looping". Two codes in the pure-C# stack were also wrong, both pinned by tests asserting the wrong value. RFC 9114 section 8.1 splits them by WHY a frame is wrong: UNEXPECTED (0x0105) is one not permitted in this state or on this stream, ERROR (0x0106) is one whose layout or size is invalid. A DATA frame before HEADERS is well formed and merely too early - section 4.1 names that exact case as H3_FRAME_UNEXPECTED - and SETTINGS on a request stream is legal on the control stream. Both now send 0x0105, and both tests say why they changed. The nghttp3 test for this was already in the tree as a Pending; it flipped on the fix and is now a Test. All suites: 420 passed, 0 failed, 19 pending.
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.
Closes #206.
A request to an HTTP/3 server on the native stack could hang forever.
Nghttp3Connectionhad exactly oneClosecall, and it was gated on!_protocolFailed— so a protocol failure could never reach it by construction. The failure was recorded, the run loop broke, and thefinallydidDecRefandDispose, neither of which puts anything on the wire.Nothing rescued the client either. The shim sets no
max_idle_timeout, and the transport's 60 s sweep is refreshed by any inbound datagram — including ones ngtcp2 discards — so a peer sending ACKs or PINGs holds a dead connection open indefinitely.The pure-C# stack fixed this in
1214005. The stack the README calls its drop-in replacement did not get it, which is the recurring shape on this branch: one behaviour, two implementations, a fix landing on one side.The code comes from nghttp3
nghttp3_err_infer_quic_app_error_codealready owns the mapping — which of its errors areH3_FRAME_ERROR,H3_MESSAGE_ERROR,QPACK_DECOMPRESSION_FAILED, and that anything unrecognised isH3_INTERNAL_ERROR. The shim gained a four-line export for it rather than a table maintained here. We vendor these libraries; re-deriving what they already know is how the two wrongNGTCP2_ERR_*constants got introduced earlier in this release.Eleven sites, not the two that were easy to find
The issue described
PushToEngine.grepfound eleven places that fail a connection. Four carry a library error and go throughFailProtocol; five are our own failure — a control stream that would not open, a callback that threw — and takeH3_INTERNAL_ERRORthroughFailInternal.The twelfth site is deliberately untouched
This is the part worth reviewing.
Streamed.cssets_protocolFailedin afinallyon a response that completed perfectly well, purely to unpark waiters —IsBrokenis what makes them return rather than loop.Keying the close on that flag would have sent an error code to every client that received a streamed body successfully. So what the peer is told is tracked separately as a nullable code, and the flag keeps meaning "stop looping". The comment at that site says so, because it looks like an oversight.
Two RFC codes were wrong, and two tests pinned them that way
RFC 9114 §8.1 splits the two by why a frame is wrong:
H3_FRAME_UNEXPECTED(0x0105) is a frame not permitted in the current state or on the current stream;H3_FRAME_ERROR(0x0106) is one whose layout or size is invalid.H3_FRAME_UNEXPECTED.Both sent 0x0106, and both had passing tests asserting it. Both now send 0x0105, and both tests say why they changed rather than silently flipping a constant.
Verification
The nghttp3 test was already in the tree as a
Pendingfrom the review round. It flipped on the fix, the runner failed the run to say so, and it is now aTest.Native rebuilt from the pinned refs with the BuildID verified into the test output before any result was believed. All suites: 420 passed, 0 failed, 19 pending.