Skip to content

Commit 04e5c28

Browse files
everett1992aduh95
authored andcommitted
stream: fix async iteration of undefined chunks
The hand-rolled async iterator introduced in #64447 unwraps thenable chunks before delivering them, but guards the `then` access with `chunk !== null` while the dereference itself requires `chunk != null`. An object mode stream carrying an `undefined` chunk therefore threw TypeError: Cannot read properties of undefined (reading 'then') Because the throw happens in a microtask rather than rejecting the iterator's promise, it surfaces as an uncaught exception that terminates the process instead of a catchable stream error. `undefined` is a legal chunk value: doc/api/stream.md documents object mode chunks as "any JavaScript value other than `null`" and reserves `null` as the end-of-stream sentinel. `readable.push(undefined)` returns true in object mode, so the stream accepts the value and then crashes on the way out. Read `then` with optional chaining at both sites so that `undefined` is delivered as a value while `null` keeps signalling end-of-stream. `then` is still read at most once, so a getter cannot observe a second access. Refs: #64447 Assisted-by: a closed-source coding agent Signed-off-by: Caleb ツ Everett <calebev@amazon.com> PR-URL: #65969 Reviewed-By: Xuguang Mei <meixuguang@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 2bf0824 commit 04e5c28

2 files changed

Lines changed: 44 additions & 4 deletions

File tree

lib/internal/streams/readable.js

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1522,8 +1522,9 @@ function createAsyncIterator(stream, options) {
15221522
const chunk = stream.destroyed ? null : stream.read();
15231523
if (chunk !== null) {
15241524
// Read `then` only once so that a getter cannot observe (or throw
1525-
// on) a second access.
1526-
const then = chunk.then;
1525+
// on) a second access. `undefined` is a valid chunk value, so it must
1526+
// not be dereferenced here.
1527+
const then = chunk?.then;
15271528
if (typeof then === 'function') {
15281529
FunctionPrototypeCall(then, chunk, (value) => {
15291530
inFlight = false;
@@ -1595,8 +1596,9 @@ function createAsyncIterator(stream, options) {
15951596
const chunk = stream.destroyed ? null : stream.read();
15961597
if (chunk !== null) {
15971598
// Read `then` only once so that a getter cannot observe (or
1598-
// throw on) a second access.
1599-
const then = chunk.then;
1599+
// throw on) a second access. `undefined` is a valid chunk value,
1600+
// so it must not be dereferenced here.
1601+
const then = chunk?.then;
16001602
if (typeof then === 'function') {
16011603
inFlight = true;
16021604
return FunctionPrototypeCall(

test/parallel/test-stream-readable-async-iterators.js

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -989,5 +989,43 @@ async function tests() {
989989
})().then(common.mustCall());
990990
}
991991

992+
{
993+
// An `undefined` chunk is a value, not end-of-stream. Here it is already
994+
// buffered, so it is read on the synchronous fast path.
995+
(async () => {
996+
const r = new Readable({ objectMode: true, read() {} });
997+
r.push(undefined);
998+
r.push(null);
999+
1000+
const it = r[Symbol.asyncIterator]();
1001+
assert.deepStrictEqual(await it.next(), { done: false, value: undefined });
1002+
assert.strictEqual((await it.next()).done, true);
1003+
})().then(common.mustCall());
1004+
}
1005+
1006+
{
1007+
// An `undefined` chunk pushed after next() is delivered once it arrives.
1008+
(async () => {
1009+
const r = new Readable({ objectMode: true, read() {} });
1010+
const it = r[Symbol.asyncIterator]();
1011+
const next = it.next();
1012+
setImmediate(() => {
1013+
r.push(undefined);
1014+
r.push(null);
1015+
});
1016+
1017+
assert.deepStrictEqual(await next, { done: false, value: undefined });
1018+
assert.strictEqual((await it.next()).done, true);
1019+
})().then(common.mustCall());
1020+
}
1021+
1022+
{
1023+
// Readable.from() delivers `undefined` values.
1024+
(async () => {
1025+
assert.deepStrictEqual(await Readable.from([undefined]).toArray(),
1026+
[undefined]);
1027+
})().then(common.mustCall());
1028+
}
1029+
9921030
// To avoid missing some tests if a promise does not resolve
9931031
tests().then(common.mustCall());

0 commit comments

Comments
 (0)