Skip to content

Commit ab9d401

Browse files
GiHoon1123aduh95
authored andcommitted
quic: retain readers for readable streams
Create readers when readable QuicStream instances are constructed so data received before the first async iterator pull survives native stream cleanup. Add a regression test for a peer-initiated unidirectional stream that sends data and FIN before the reader is pulled. Assisted-by: Codex Signed-off-by: GiHoon1123 <rlaejrqo465@naver.com> PR-URL: #66407 Fixes: #66347 Reviewed-By: Tim Perry <pimterry@gmail.com>
1 parent c05a97a commit ab9d401

4 files changed

Lines changed: 96 additions & 9 deletions

File tree

‎doc/api/quic.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1993,6 +1993,11 @@ Immediately and abruptly destroys the stream. If `error` is provided and
19931993
[`stream.onerror`][] is set, the `onerror` callback is invoked before
19941994
destruction. The `stream.closed` promise rejects with the error.
19951995

1996+
Buffered incoming data remains readable after destruction, even if iteration
1997+
has not started. If the readable side ended without a FIN, the iterator yields
1998+
that data before reporting the truncation or error according to
1999+
[`sessionOptions.truncatedReads`][].
2000+
19962001
When the stream is destroyed with an `error` (or with an explicit
19972002
`options.code`), the QUIC stack signals the abort to the peer:
19982003

@@ -4888,6 +4893,7 @@ throughput issues caused by flow control.
48884893
[`sessionOptions.sessionTicket`]: #sessionoptionssessionticket
48894894
[`sessionOptions.sni`]: #sessionoptionssni-server-only
48904895
[`sessionOptions.token`]: #sessionoptionstoken-client-only
4896+
[`sessionOptions.truncatedReads`]: #sessionoptionstruncatedreads
48914897
[`stream.destroy()`]: #streamdestroyerror-options
48924898
[`stream.headers`]: #streamheaders
48934899
[`stream.onerror`]: #streamonerror

‎lib/internal/quic/quic.js‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1679,6 +1679,12 @@ class QuicStream {
16791679
this.#handle = handle;
16801680
handle[kOwner] = this;
16811681
const inner = this.#inner;
1682+
// Keep a reader alive for readable streams before the native handle can
1683+
// be destroyed. A peer may send data and FIN before the consumer asks
1684+
// for its async iterator.
1685+
if (!isLocal || direction === kStreamDirectionBidirectional) {
1686+
inner.reader = handle.getReader();
1687+
}
16821688
inner.session = session;
16831689
inner.direction = direction;
16841690
inner.isLocal = isLocal;
@@ -2062,8 +2068,8 @@ class QuicStream {
20622068
}
20632069

20642070
/**
2065-
* Immediately destroys the stream. Any queued data is discarded. If
2066-
* an error is given, the closed promise will be rejected with that
2071+
* Immediately destroys the stream. Buffered incoming data remains readable.
2072+
* If an error is given, the closed promise will be rejected with that
20672073
* error. If no error is given, the closed promise will be resolved.
20682074
* When destroying with an error, RESET_STREAM and/or STOP_SENDING
20692075
* are emitted to the peer for any still-open writable / readable

‎test/parallel/test-quic-stream-truncated-reads-destroy.mjs‎

Lines changed: 35 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -43,17 +43,16 @@ const serve = (stream) => { stream.setBody(stallingBody(1000)); };
4343
assert.strictEqual(threw.errorCode, 0n);
4444
}
4545

46-
// The session is destroyed before iteration even starts: the buffered data
47-
// is gone, but the truncation is still reported rather than a clean empty
48-
// end.
46+
// Destroying the session before iteration preserves the buffered data,
47+
// but still reports truncation after it has been read.
4948
{
5049
const { received, threw } = await readStream(serve, {
5150
beforeIterate: async ({ stream, session }) => {
52-
while (stream.stats.bytesReceived === 0n) await setTimeout(5);
51+
while (stream.stats.bytesReceived < 1000n) await setTimeout(5);
5352
session.destroy();
5453
},
5554
});
56-
assert.strictEqual(received, 0);
55+
assert.strictEqual(received, 1000);
5756
assert.strictEqual(threw?.code, 'ERR_QUIC_STREAM_ABORTED');
5857
}
5958

@@ -62,11 +61,40 @@ const serve = (stream) => { stream.setBody(stallingBody(1000)); };
6261
{
6362
const { received, threw, closedError } = await readStream(serve, {
6463
beforeIterate: async ({ stream }) => {
65-
while (stream.stats.bytesReceived === 0n) await setTimeout(5);
64+
while (stream.stats.bytesReceived < 1000n) await setTimeout(5);
6665
stream.destroy();
6766
},
6867
});
69-
assert.strictEqual(received, 0);
68+
assert.strictEqual(received, 1000);
7069
assert.strictEqual(threw?.code, 'ERR_QUIC_STREAM_ABORTED');
7170
assert.strictEqual(closedError, undefined);
7271
}
72+
73+
// An explicit error must survive draining the buffered data, even when
74+
// truncation itself is ignored. Use more than one reader batch of data.
75+
for (const target of ['session', 'stream']) {
76+
for (const truncatedReads of ['error', 'ignore']) {
77+
const size = 128 * 1024;
78+
const boom = new Error(`${target}: ${truncatedReads}`);
79+
const { received, threw, closedError } = await readStream((stream) => {
80+
stream.setBody(stallingBody(size));
81+
}, {
82+
clientOptions: {
83+
truncatedReads,
84+
transportParams: {
85+
initialMaxStreamDataBidiLocal: size * 2,
86+
initialMaxData: size * 2,
87+
},
88+
},
89+
beforeIterate: async (context) => {
90+
while (context.stream.stats.bytesReceived < BigInt(size)) {
91+
await setTimeout(5);
92+
}
93+
context[target].destroy(boom);
94+
},
95+
});
96+
assert.strictEqual(received, size);
97+
assert.strictEqual(threw, boom);
98+
assert.strictEqual(closedError, boom);
99+
}
100+
}
Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
// Flags: --experimental-quic --experimental-stream-iter --no-warnings
2+
3+
// Test: data received before the async iterator is pulled remains readable
4+
// after a peer-initiated unidirectional stream closes.
5+
6+
import { hasQuic, skip, mustCall } from '../common/index.mjs';
7+
import assert from 'node:assert';
8+
9+
if (!hasQuic) {
10+
skip('QUIC is not enabled');
11+
}
12+
13+
const { listen, connect } = await import('../common/quic.mjs');
14+
const { bytes } = await import('stream/iter');
15+
16+
const encoder = new TextEncoder();
17+
const expected = encoder.encode('data before the first pull');
18+
const done = Promise.withResolvers();
19+
20+
const serverEndpoint = await listen(mustCall(async (serverSession) => {
21+
await serverSession.opened;
22+
const stream = await serverSession.createUnidirectionalStream({
23+
body: expected,
24+
});
25+
await stream.closed;
26+
serverSession.close();
27+
}));
28+
29+
const clientSession = await connect(serverEndpoint.address);
30+
await clientSession.opened;
31+
32+
clientSession.onstream = mustCall(async (stream) => {
33+
const iterator = stream[Symbol.asyncIterator]();
34+
35+
// Delay the first pull until after the peer has sent FIN and the native
36+
// stream handle has been closed.
37+
await stream.closed;
38+
39+
const received = await bytes(iterator);
40+
assert.deepStrictEqual(received, expected);
41+
clientSession.close();
42+
done.resolve();
43+
});
44+
45+
await done.promise;
46+
await clientSession.closed;
47+
await serverEndpoint.close();

0 commit comments

Comments
 (0)