Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[x] stream: error multiple iterators #29064

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -1703,6 +1703,11 @@ An attempt was made to call [`stream.pipe()`][] on a [`Writable`][] stream.
A stream method was called that cannot complete because the stream was
destroyed using `stream.destroy()`.

<a id="ERR_STREAM_ITERATOR_EXISTS"></a>
### ERR_STREAM_ITERATOR_EXISTS

Stream cannot be consumed by multiple iterators.

<a id="ERR_STREAM_NULL_VALUES"></a>
### ERR_STREAM_NULL_VALUES

Expand Down
1 change: 1 addition & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -1130,6 +1130,7 @@ E('ERR_SRI_PARSE',
SyntaxError);
E('ERR_STREAM_CANNOT_PIPE', 'Cannot pipe, not readable', Error);
E('ERR_STREAM_DESTROYED', 'Cannot call %s after a stream was destroyed', Error);
E('ERR_STREAM_ITERATOR_EXISTS', 'Iterator already exists', Error);
E('ERR_STREAM_NULL_VALUES', 'May not write null values to stream', TypeError);
E('ERR_STREAM_PREMATURE_CLOSE', 'Premature close', Error);
E('ERR_STREAM_PUSH_AFTER_EOF', 'stream.push() after EOF', Error);
Expand Down
11 changes: 11 additions & 0 deletions lib/internal/streams/async_iterator.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,18 @@ const { Object } = primordials;

const finished = require('internal/streams/end-of-stream');

const {
ERR_STREAM_ITERATOR_EXISTS
} = require('internal/errors').codes;

const kLastResolve = Symbol('lastResolve');
const kLastReject = Symbol('lastReject');
const kError = Symbol('error');
const kEnded = Symbol('ended');
const kLastPromise = Symbol('lastPromise');
const kHandlePromise = Symbol('handlePromise');
const kStream = Symbol('stream');
const kIterator = Symbol('iterator');

function createIterResult(value, done) {
return { value, done };
Expand Down Expand Up @@ -126,6 +131,10 @@ const ReadableStreamAsyncIteratorPrototype = Object.setPrototypeOf({
}, AsyncIteratorPrototype);

const createReadableStreamAsyncIterator = (stream) => {
if (stream[kIterator]) {
throw new ERR_STREAM_ITERATOR_EXISTS();
}

const iterator = Object.create(ReadableStreamAsyncIteratorPrototype, {
[kStream]: { value: stream, writable: true },
[kLastResolve]: { value: null, writable: true },
Expand Down Expand Up @@ -182,6 +191,8 @@ const createReadableStreamAsyncIterator = (stream) => {

stream.on('readable', onReadable.bind(null, iterator));

stream[kIterator] = true;

return iterator;
};

Expand Down
20 changes: 5 additions & 15 deletions test/parallel/test-readline-async-iterators-destroy.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,24 +58,14 @@ async function testMutualDestroy() {
crlfDelay: Infinity
});

const expectedLines = fileContent.split('\n');
if (expectedLines[expectedLines.length - 1] === '') {
expectedLines.pop();
}
expectedLines.splice(2);

const iteratedLines = [];
for await (const k of rli) {
iteratedLines.push(k);
for await (const l of rli) {
iteratedLines.push(l);
break;
try {
for await (const l of rli) {
}
} catch (err) {
assert.strictEqual(err.code, 'ERR_STREAM_ITERATOR_EXISTS');
ronag marked this conversation as resolved.
Show resolved Hide resolved
}
assert.deepStrictEqual(iteratedLines, expectedLines);
}

assert.deepStrictEqual(iteratedLines, expectedLines);

rli.close();
readable.destroy();
}
Expand Down
33 changes: 1 addition & 32 deletions test/parallel/test-readline-async-iterators.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,35 +43,4 @@ async function testSimple() {
}
}

async function testMutual() {
for (const fileContent of testContents) {
fs.writeFileSync(filename, fileContent);

const readable = fs.createReadStream(filename);
const rli = readline.createInterface({
input: readable,
crlfDelay: Infinity
});

const expectedLines = fileContent.split('\n');
if (expectedLines[expectedLines.length - 1] === '') {
expectedLines.pop();
}
const iteratedLines = [];
let iterated = false;
for await (const k of rli) {
// This outer loop should only iterate once.
assert.strictEqual(iterated, false);
iterated = true;

iteratedLines.push(k);
for await (const l of rli) {
iteratedLines.push(l);
}
assert.deepStrictEqual(iteratedLines, expectedLines);
}
assert.deepStrictEqual(iteratedLines, expectedLines);
}
}

testSimple().then(testMutual).then(common.mustCall());
testSimple().then(common.mustCall());
19 changes: 17 additions & 2 deletions test/parallel/test-stream-readable-async-iterators.js
Original file line number Diff line number Diff line change
Expand Up @@ -375,11 +375,26 @@ async function tests() {
// eslint-disable-next-line no-unused-vars
for await (const a of r) {
}
// eslint-disable-next-line no-unused-vars
for await (const b of r) {
try {
// eslint-disable-next-line no-unused-vars
for await (const b of r) {
}
} catch (err) {
assert.strictEqual(err.code, 'ERR_STREAM_ITERATOR_EXISTS');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test would have to be split in two. The previous code verified that iterating on an ended stream completes - this would have to be tested without the first for await. Then , we should add a new test about the ERR_STREAM_ITERATOR_EXISTS error.

Copy link
Member

@addaleax addaleax Aug 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fwiw, this also doesn’t guarantee that an error was thrown here, because in that case the catch() { ... } block simply wouldn’t run. You typically want assert.throws() or, in this case, assert.rejects(), instead.

}
}

{
console.log('creating multiple iterators');
const r = new Readable();
r[Symbol.asyncIterator]();
common.expectsError(() => {
r[Symbol.asyncIterator]();
}, {
code: 'ERR_STREAM_ITERATOR_EXISTS'
});
}
ronag marked this conversation as resolved.
Show resolved Hide resolved

{
console.log('destroy mid-stream does not error');
const r = new Readable({
Expand Down