Multiple fixes for async iterators

[mcollina]

Fixes: #23890
Fixes: #23891

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

[nodejs-github-bot]

@mcollina build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1377/pipeline

[mcollina]

CI: https://ci.nodejs.org/job/node-test-pull-request/18170/

[TimothyGu]

Is this the only way to fix it? I would’ve expected us to perhaps do something with end-of-stream rather than selectively ignoring errors.

[mcollina]

end-of-stream treats non-ending (destroy) streams that close early as an error condition. I think it is right in doing so.

However this does not play well with async iterators as you pointed out. We had a different logic before that did the same but was less explicit about it: I like this one better.

[TimothyGu]

end-of-stream treats non-ending (destroy) streams that close early as an error condition. I think it is right in doing so.

Can you elaborate? It sounds to me that destroyed streams (with null as error value) should be treated no differently as any other stream that ended. I.e., it's reasonable to believe a stream can be destroyed whenever.

In fact, I would say that the following test case added in #23785 should in fact finish gracefully with an { done: true, value: undefined } rather than throwing an error:

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

Lines 328 to 343 in 482b56a

	await (async function() {
	console.log('.next() on destroyed stream');
	const readable = new Readable({
	read() {
	// no-op
	}
	});
	readable.destroy();
	try {
	await readable[Symbol.asyncIterator]().next();
	} catch (e) {
	assert.strictEqual(e.code, 'ERR_STREAM_PREMATURE_CLOSE');
	}
	})();

We see precedent for invalidated iterator still operating gracefully in ECMA-262 iterators:

const array = [0, 1, 2, 3];
const iterated = [];
for (const a of array) {
  iterated.push(a);
  if (a === 1) {
    array.splice(0);
  }
  // loop ends immediately.
}
// iterated is [0, 1]

[mcollina]

First, end-of-stream is one of the most downloaded utilities on npm, and keeping the API and behavior similar has been a goal. Second, from a consumer perspective a stream that was ended or destroyed is significantly different. That could have easily been a flag as an argument. I think it errors because in the context of pipeline/pump, in fact it is an error: the pipeline did not complete correctly but was interrupted (as an example, gzipping a file).

[mcollina]

I’ll check again that test asap, it does not sound correct with this change.

[TimothyGu]

Okay, as long as async iteration isn't affected it works for me.

[mcollina]

@TimothyGu the test was botched anyway, and the catch clause was never executed in this case. I've fixed it.

[mcollina]

CI: https://ci.nodejs.org/job/node-test-pull-request/18184/

[targos]

Landed in 398418d and b1e1fe4

[MylesBorins]

landed into 10.x with #23785

opted to not land in 8.x (I don't believe the async iteration work is on that release stream)

[mcollina]

It should not backported to 8. Il giorno lun 26 nov 2018 alle 22:41 Myles Borins <[email protected]> ha scritto:

…

landed into 10.x with #23785 <#23785> opted to not land in 8.x (I don't believe the async iteration work is on that release stream) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23901 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADL41zXgv77KrIyOZbZagNzfwRotU-Aks5uzGAVgaJpZM4X8EO3> .