quic: add stream.cancel() sending H3_REQUEST_CANCELLED - #66007
Open
trivenay wants to merge 1 commit into
Open
Conversation
When an endpoint deliberately abandons a request or response, RFC 9114 section 4.1.1 says the stream should be aborted with the error code H3_REQUEST_CANCELLED. There is currently no way to express this: the reset primitives default to the no-error code and destroy(error) uses the internal error code. Add stream.cancel([reason]), which abruptly terminates both directions of the stream using the cancellation code defined by the negotiated application protocol (H3_REQUEST_CANCELLED for HTTP/3; the no-error code for other applications). The code value is plumbed from the application layer through the session state, following the same pattern as the request-rejected code. Refs: nodejs#65509 Signed-off-by: Naman Trivedi <trivenay@amazon.com>
Collaborator
|
Review requested:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #66007 +/- ##
=======================================
Coverage 89.98% 89.99%
=======================================
Files 785 785
Lines 269282 269308 +26
Branches 51296 51292 -4
=======================================
+ Hits 242323 242357 +34
+ Misses 17464 17449 -15
- Partials 9495 9502 +7
🚀 New features to boost your workflow:
|
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.
Follow-up to #65442. I opened #65509 to finish RFC 9114 compliance for the stream closure error codes, and this implements the cancellation half.
There is currently no way to signal that a request or response was deliberately abandoned:
resetStream()/stopSending()default to the no-error code anddestroy(error)sends the internal error code. Per RFC 9114 section 4.1.1, a cancelled request should be aborted withH3_REQUEST_CANCELLED.This adds
stream.cancel([reason]), which terminates both directions of the stream using the cancellation code defined by the negotiated application protocol --H3_REQUEST_CANCELLED(0x10c) for HTTP/3, the no-error code for other applications. The code value is plumbed from the application layer through the session state, same pattern as the request-rejected code in #65442. The existing reset primitives are unchanged.The main alternative considered in #65509 was exposing the cancellation code as a named constant and letting callers pass it to the existing
resetStream()/stopSending(). That keeps the API surface smaller, but cancelling is a single intent that requires aborting both halves of the stream -- aRESET_STREAMand aSTOP_SENDING, each carrying the right code for the negotiated protocol. Making every caller do that two-call dance and pick the code is exactly the part that seems worth encapsulating, the same way #65442 picks the rejected code internally. I'm using this PR as a vehicle for that discussion -- happy to change the approach if a different shape is preferred.The other item from #65509,
H3_REQUEST_INCOMPLETE, is still pending: nghttp3 defines the constant but exposes no hook for detecting an incomplete message, so emitting it would need message-completeness tracking on our side. That's considerably more invasive, and honestly I'm not yet convinced the value justifies the complexity -- left out of this PR.Refs: #65509