Skip to content

Commit e432622

Browse files
committed
fixup! fixup! quic: do not destroy incoming streams that have a consumer
Signed-off-by: Naman Trivedi <trivenay@amazon.com>
1 parent dbccdfa commit e432622

3 files changed

Lines changed: 62 additions & 13 deletions

File tree

‎doc/api/quic.md‎

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -307,9 +307,9 @@ separate APIs for creating each kind:
307307
[`session.createUnidirectionalStream()`][]. Streams initiated by a remote
308308
peer are delivered via the [`session.onstream`][] callback. When the
309309
negotiated application protocol supports the stream-level callbacks (e.g.
310-
HTTP/3) and any of them are configured, incoming streams can instead be
311-
consumed entirely through those callbacks (such as `onheaders`) and
312-
registering `onstream` is optional.
310+
HTTP/3) and an `onheaders` callback is configured, incoming streams can
311+
instead be consumed entirely through it and registering `onstream` is
312+
optional.
313313

314314
There are two ways to write data to a stream:
315315

@@ -414,8 +414,8 @@ A typical client session progresses through these stages:
414414
On the server side, call [`quic.listen()`][] with a callback. The callback
415415
fires for each incoming session after the TLS handshake begins. Incoming
416416
streams arrive via the [`session.onstream`][] callback, or, for HTTP/3
417-
sessions with stream-level callbacks configured, directly through those
418-
callbacks (see the [minimal HTTP/3 server][] example).
417+
sessions with an `onheaders` callback configured, directly through that
418+
callback (see the [minimal HTTP/3 server][] example).
419419

420420
[`session.destroy()`][] is available for immediate teardown — all open streams
421421
are destroyed and the session is closed without waiting for them to finish.
@@ -1117,11 +1117,13 @@ added: v23.8.0
11171117
The callback to invoke when a new stream is initiated by a remote peer. Read/write.
11181118

11191119
If no `onstream` callback is set and the stream has no other consumer, an
1120-
incoming stream is destroyed on arrival and a warning is emitted. Stream-level
1121-
callbacks (such as `onheaders`) count as a consumer when the negotiated
1122-
application protocol supports them (e.g. HTTP/3), so an HTTP/3 server that
1123-
handles requests entirely through those callbacks does not need to set
1124-
`onstream`.
1120+
incoming stream is destroyed on arrival and a warning is emitted. An
1121+
`onheaders` callback counts as a consumer when the negotiated application
1122+
protocol supports it (e.g. HTTP/3), because it is invoked for every incoming
1123+
request stream. Other stream-level callbacks (`ontrailers`, `oninfo`,
1124+
`onwanttrailers`) do not, since they are conditional or outbound-only and
1125+
would leave the stream unobservable. An HTTP/3 server that handles requests
1126+
entirely through `onheaders` does not need to set `onstream`.
11251127

11261128
### `session.ondatagram`
11271129

@@ -4023,7 +4025,7 @@ const encoder = new TextEncoder();
40234025
const endpoint = await listen((session) => {
40244026
// The session.onstream callback fires for each new client-initiated
40254027
// stream. It is optional here: with `onheaders` configured below,
4026-
// request streams are consumed through the stream-level callbacks.
4028+
// request streams are consumed through that callback.
40274029
}, {
40284030
sni: { '*': { keys: [defaultKey], certs: [defaultCert] } },
40294031
// ALPN defaults to 'h3'.

‎lib/internal/quic/quic.js‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4138,7 +4138,12 @@ class QuicSession {
41384138
*/
41394139
#hasStreamConsumer() {
41404140
if (typeof this.#inner.onstream === 'function') return true;
4141-
if (this[kStreamCallbacks] == null) return false;
4141+
// Only onheaders is guaranteed to fire for every incoming stream when
4142+
// the negotiated application supports stream callbacks (e.g. HTTP/3).
4143+
// Other stream callbacks are conditional (ontrailers, oninfo) or
4144+
// outbound-only (onwanttrailers) and do not expose the stream, so they
4145+
// do not count as a consumer.
4146+
if (typeof this[kStreamCallbacks]?.onheaders !== 'function') return false;
41424147
return getQuicSessionState(this).streamCallbacksSupported === 1;
41434148
}
41444149

‎test/parallel/test-quic-h3-stream-without-onstream.mjs‎

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
// streams (and emits a warning), so unconsumed streams cannot
1212
// accumulate and hold flow control credit.
1313

14-
import { hasQuic, skip, mustCall } from '../common/index.mjs';
14+
import { hasQuic, skip, mustCall, mustNotCall } from '../common/index.mjs';
1515
import assert from 'node:assert';
1616
import * as fixtures from '../common/fixtures.mjs';
1717

@@ -129,3 +129,45 @@ function failOnConsumerWarning(warning) {
129129
await clientSession.close();
130130
await serverEndpoint.close();
131131
}
132+
133+
// --- Stream callbacks that do not expose the stream are not a consumer ---
134+
// Only `onheaders` is invoked for every incoming h3 request stream.
135+
// `ontrailers` and `oninfo` are conditional and `onwanttrailers` is
136+
// outbound-only, so a session registering only those callbacks has no way
137+
// to observe an incoming stream and it must be destroyed with the warning.
138+
for (const callbackName of ['ontrailers', 'oninfo', 'onwanttrailers']) {
139+
const warned = Promise.withResolvers();
140+
process.on('warning', function onWarning(warning) {
141+
if (warning.message === kWarning) {
142+
process.off('warning', onWarning);
143+
warned.resolve();
144+
}
145+
});
146+
147+
const serverEndpoint = await listen(mustCall((serverSession) => {
148+
serverSession.onerror = () => {};
149+
}), {
150+
sni: { '*': { keys: [key], certs: [cert] } },
151+
[callbackName]: mustNotCall(),
152+
});
153+
154+
const clientSession = await connect(serverEndpoint.address, {
155+
servername: 'localhost',
156+
verifyPeer: 'manual',
157+
});
158+
await clientSession.opened;
159+
160+
const stream = await clientSession.createBidirectionalStream({
161+
headers: {
162+
':method': 'GET',
163+
':path': '/test',
164+
':scheme': 'https',
165+
':authority': 'localhost',
166+
},
167+
});
168+
stream.onerror = () => {};
169+
170+
await warned.promise;
171+
await clientSession.close();
172+
await serverEndpoint.close();
173+
}

0 commit comments

Comments
 (0)