doc: document quic stopSending() and resetStream() - #64888
Conversation
Sorry, something went wrong.
`QuicStream` exposes `stopSending()` and `resetStream()`, but neither appeared in the QuicStream API reference. Both matter when half-closing a stream, which protocols such as WebTransport rely on. Document the two methods and list them in the "Aborting a stream" summary, which previously covered only `writer.fail()` and `stream.destroy()`. Unlike those, both send the given code as-is rather than deriving a wire code from an error. Fixes: nodejs#63680 Signed-off-by: Ji Hoon Kang <ivory.ma9ic@gmail.com>
|
Review requested:
|
Sorry, something went wrong.
The reference added earlier in this pull request described only the bidirectional happy path. Three behaviours were missing: * `resetStream()` discards any data still queued for sending. A reset stream is never acknowledged, so that queue can no longer drain. * `resetStream()` is a no-op once the stream has already been reset. * Neither method sends a frame on the unidirectional stream that lacks the side it aborts — `stopSending()` on a locally-initiated one, `resetStream()` on a remote-initiated one. Both fail silently there. The wording for stream directionality follows `stream.destroy()`, which already states that the readable side exists on bidirectional and remote-initiated unidirectional streams. Refs: nodejs#63680 Signed-off-by: Ji Hoon Kang <ivory.ma9ic@gmail.com>
jasnell
left a comment
There was a problem hiding this comment.
With the caveat that the details on this may continue to change, LGTM
Sorry, something went wrong.
Commit Queue failed- Loading data for nodejs/node/pull/64888 ✔ Done loading data for nodejs/node/pull/64888 ----------------------------------- PR info ------------------------------------ Title doc: document quic stopSending() and resetStream() (#64888) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch theSnackOverflow:quic-doc-stopsending-resetstream -> nodejs:main Labels doc, quic Commits 2 - doc: document quic stopSending() and resetStream() - doc: note quic reset and unidirectional edge cases Committers 1 - Ji Hoon Kang <ivory.ma9ic@gmail.com> PR-URL: https://github.com/nodejs/node/pull/64888 Fixes: https://github.com/nodejs/node/issues/63680 Reviewed-By: James M Snell <jasnell@gmail.com> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/64888 Fixes: https://github.com/nodejs/node/issues/63680 Reviewed-By: James M Snell <jasnell@gmail.com> -------------------------------------------------------------------------------- ℹ This PR was created on Sat, 01 Aug 2026 00:15:21 GMT ✔ Approvals: 1 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/64888#pullrequestreview-4889029982 ✔ Last GitHub CI successful ℹ Green GitHub CI is sufficient -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 64888 From https://github.com/nodejs/node * branch refs/pull/64888/merge -> FETCH_HEAD ✔ Fetched commits as 347e26618740..8784c5a601b5 -------------------------------------------------------------------------------- Auto-merging doc/api/quic.md [main 37b84fb75a] doc: document quic stopSending() and resetStream() Author: Ji Hoon Kang <ivory.ma9ic@gmail.com> Date: Sat Aug 1 09:09:08 2026 +0900 1 file changed, 43 insertions(+), 1 deletion(-) Auto-merging doc/api/quic.md [main 4b0921f58d] doc: note quic reset and unidirectional edge cases Author: Ji Hoon Kang <ivory.ma9ic@gmail.com> Date: Sun Aug 2 22:43:42 2026 +0900 1 file changed, 9 insertions(+), 4 deletions(-) ✔ Patches applied There are 2 commits in the PR. Attempting autorebase. (node:486) [DEP0190] DeprecationWarning: Passing args to a child process with shell option true can lead to security vulnerabilities, as the arguments are not escaped, only concatenated. (Use `node --trace-deprecation ...` to show where the warning was created) Rebasing (2/4) Executing: git node land --amend --yes ⚠ Found Fixes: https://github.com/nodejs/node/issues/63680, skipping.. --------------------------------- New Message ---------------------------------- doc: document quic stopSending() and resetStream()
The wording for stream directionality follows stream.destroy(), which Refs: #63680
|
Sorry, something went wrong.
|
The commit queue stopped because this PR has two commits and no landing strategy label, so it fell through to the default --oneCommitMax in tools/actions/commit-queue.sh:
Squashing into a single commit is fine by me — the second commit only extends the same reference section the first one added, so there is no reason to keep them apart in history. Whenever convenient, this needs both commit-queue-squash and commit-queue re-added, since the queue selects on the latter and the bot removed it on the failed run. Thanks for the review. |
Sorry, something went wrong.
`QuicStream` exposes `stopSending()` and `resetStream()`, but neither appeared in the QuicStream API reference. Both matter when half-closing a stream, which protocols such as WebTransport rely on. Document the two methods and list them in the "Aborting a stream" summary, which previously covered only `writer.fail()` and `stream.destroy()`. Unlike those, both send the given code as-is rather than deriving a wire code from an error. Fixes: #63680 Signed-off-by: Ji Hoon Kang <ivory.ma9ic@gmail.com> PR-URL: #64888 Reviewed-By: James M Snell <jasnell@gmail.com>
`QuicStream` exposes `stopSending()` and `resetStream()`, but neither appeared in the QuicStream API reference. Both matter when half-closing a stream, which protocols such as WebTransport rely on. Document the two methods and list them in the "Aborting a stream" summary, which previously covered only `writer.fail()` and `stream.destroy()`. Unlike those, both send the given code as-is rather than deriving a wire code from an error. Fixes: #63680 Signed-off-by: Ji Hoon Kang <ivory.ma9ic@gmail.com> PR-URL: #64888 Reviewed-By: James M Snell <jasnell@gmail.com>
Fixes: #63680
QuicStream.prototype.stopSending() and QuicStream.prototype.resetStream()
are part of the public surface but were missing from the QuicStream reference.
The wording follows the JSDoc already on the implementation:
node/lib/internal/quic/quic.js
Lines 2411 to 2435 in 8a1ca0f
I also added both to the Aborting a stream section, which listed only
writer.fail() and stream.destroy(). The distinction seemed worth calling
out: those two derive the wire code from an error (falling back to the
protocol's "internal error" code), while stopSending() and resetStream()
send the given code as-is.
added: is set to v23.8.0, matching the rest of the file — both methods were
introduced in 062ae6f, the same commit that created doc/api/quic.md, and
were simply omitted from it. Happy to change this if a different version is
more accurate.
Verification
doc/api/quic.md is in skip_apidoc_files (Makefile), so it is not part of the
HTML/JSON doc build and make doc-only does not exercise it. Validation was
therefore limited to:
→ #streamsetpriorityoptions)
Note
This supersedes #63681, which covered the same issue but was closed without
landing.
Update
Added a second commit covering behaviour the first one missed:
once the stream has already been reset.
it aborts — stopSending() on a locally-initiated one, resetStream() on a
remote-initiated one. Both fail silently there.
Verified against Stream::DoStreamReset and Stream::SendStopSending in
src/quic/streams.cc. The directionality wording follows stream.destroy(),
which already frames the readable side as existing on bidirectional and
remote-initiated unidirectional streams.
I left the valid range of code undocumented on purpose: the binding reads it
with Uint64Value() and discards the lossless flag, so out-of-range values are
silently truncated rather than rejected. Documenting a 62-bit limit would state
a contract the implementation does not currently enforce. Happy to open that
separately if it's worth tracking.