stream: simpler and faster Readable async iterator

Reimplement as an async generator instead of a custom
iterator class.

PR-URL: #34035
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Author: ronag

benchmark/streams/readable-async-iterator.js

@@ -0,0 +1,42 @@
+'use strict';
+
+const common = require('../common');
+const Readable = require('stream').Readable;
+
+const bench = common.createBenchmark(main, {
+  n: [1e5],
+  sync: ['yes', 'no'],
+});
+
+async function main({ n, sync }) {
+  sync = sync === 'yes';
+
+  const s = new Readable({
+    objectMode: true,
+    read() {
+      if (sync) {
+        this.push(1);
+      } else {
+        process.nextTick(() => {
+          this.push(1);
+        });
+      }
+    }
+  });
+
+  bench.start();
+
+  let x = 0;
+  for await (const chunk of s) {
+    x += chunk;
+    if (x > n) {
+      break;
+    }
+  }
+
+  // Side effect to ensure V8 does not optimize away the
+  // loop as a noop.
+  console.log(x);
+
+  bench.end(n);
+}

lib/_stream_readable.js

@@ -28,6 +28,7 @@ const {
   ObjectDefineProperties,
   ObjectKeys,
   ObjectSetPrototypeOf,
+  Promise,
   Set,
   SymbolAsyncIterator,
   Symbol
@@ -60,11 +61,11 @@ const kPaused = Symbol('kPaused');
 
 // Lazy loaded to improve the startup performance.
 let StringDecoder;
-let createReadableStreamAsyncIterator;
 let from;
 
 ObjectSetPrototypeOf(Readable.prototype, Stream.prototype);
 ObjectSetPrototypeOf(Readable, Stream);
+function nop() {}
 
 const { errorOrDestroy } = destroyImpl;
 
@@ -1055,13 +1056,68 @@ Readable.prototype.wrap = function(stream) {
 };
 
 Readable.prototype[SymbolAsyncIterator] = function() {
-  if (createReadableStreamAsyncIterator === undefined) {
-    createReadableStreamAsyncIterator =
-      require('internal/streams/async_iterator');
+  let stream = this;
+
+  if (typeof stream.read !== 'function') {
+    // v1 stream
+    const src = stream;
+    stream = new Readable({
+      objectMode: true,
+      destroy(err, callback) {
+        destroyImpl.destroyer(src, err);
+        callback();
+      }
+    }).wrap(src);
   }
-  return createReadableStreamAsyncIterator(this);
+
+  const iter = createAsyncIterator(stream);
+  iter.stream = stream;
+  return iter;
 };
 
+async function* createAsyncIterator(stream) {
+  let callback = nop;
+
+  function next(resolve) {
+    if (this === stream) {
+      callback();
+      callback = nop;
+    } else {
+      callback = resolve;
+    }
+  }
+
+  stream
+    .on('readable', next)
+    .on('error', next)
+    .on('end', next)
+    .on('close', next);
+
+  try {
+    const state = stream._readableState;
+    while (true) {
+      const chunk = stream.read();
+      if (chunk !== null) {
+        yield chunk;
+      } else if (state.errored) {
+        throw state.errored;
+      } else if (state.ended) {
+        break;
+      } else if (state.closed) {
+        // TODO(ronag): ERR_PREMATURE_CLOSE?
+        break;
+      } else {
+        await new Promise(next);
+      }
+    }
+  } catch (err) {
+    destroyImpl.destroyer(stream, err);
+    throw err;
+  } finally {
+    destroyImpl.destroyer(stream, null);
+  }
+}
+
 // Making it explicit these properties are not enumerable
 // because otherwise some prototype manipulation in
 // userland will fail.

lib/internal/streams/async_iterator.js

@@ -23,7 +23,7 @@ const {
 
 let EE;
 let PassThrough;
-let createReadableStreamAsyncIterator;
+let Readable;
 
 function destroyer(stream, reading, writing, callback) {
   callback = once(callback);
@@ -113,11 +113,11 @@ function makeAsyncIterable(val) {
 }
 
 async function* fromReadable(val) {
-  if (!createReadableStreamAsyncIterator) {
-    createReadableStreamAsyncIterator =
-      require('internal/streams/async_iterator');
+  if (!Readable) {
+    Readable = require('_stream_readable');
   }
-  yield* createReadableStreamAsyncIterator(val);
+
+  yield* Readable.prototype[SymbolAsyncIterator].call(val);
 }
 
 async function pump(iterable, writable, finish) {

lib/internal/streams/pipeline.js

@@ -230,7 +230,6 @@
       'lib/internal/worker/js_transferable.js',
       'lib/internal/watchdog.js',
       'lib/internal/streams/lazy_transform.js',
-      'lib/internal/streams/async_iterator.js',
       'lib/internal/streams/buffer_list.js',
       'lib/internal/streams/duplexpair.js',
       'lib/internal/streams/from.js',

node.gyp

@@ -75,6 +75,7 @@ async function testMutualDestroy() {
         break;
       }
       assert.deepStrictEqual(iteratedLines, expectedLines);
+      break;
     }
 
     assert.deepStrictEqual(iteratedLines, expectedLines);

test/parallel/test-readline-async-iterators-destroy.js

@@ -11,17 +11,6 @@ const {
 const assert = require('assert');
 
 async function tests() {
-  {
-    const AsyncIteratorPrototype = Object.getPrototypeOf(
-      Object.getPrototypeOf(async function* () {}).prototype);
-    const rs = new Readable({
-      read() {}
-    });
-    assert.strictEqual(
-      Object.getPrototypeOf(Object.getPrototypeOf(rs[Symbol.asyncIterator]())),
-      AsyncIteratorPrototype);
-  }
-
   {
     // v1 stream
 
@@ -53,7 +42,9 @@ async function tests() {
     });
 
     const iter = Readable.prototype[Symbol.asyncIterator].call(stream);
-    iter.next().catch(common.mustCall((err) => {
+    await iter.next();
+    await iter.next();
+    await iter.next().catch(common.mustCall((err) => {
       assert.strictEqual(err.message, 'asd');
     }));
   }
@@ -189,12 +180,19 @@ async function tests() {
     resolved.forEach(common.mustCall(
       (item, i) => assert.strictEqual(item.value, 'hello-' + i), max));
 
-    errors.forEach((promise) => {
+    errors.slice(0, 1).forEach((promise) => {
       promise.catch(common.mustCall((err) => {
         assert.strictEqual(err.message, 'kaboom');
       }));
     });
 
+    errors.slice(1).forEach((promise) => {
+      promise.then(common.mustCall(({ done, value }) => {
+        assert.strictEqual(done, true);
+        assert.strictEqual(value, undefined);
+      }));
+    });
+
     readable.destroy(new Error('kaboom'));
   }
 
@@ -284,28 +282,6 @@ async function tests() {
     assert.strictEqual(received, 1);
   }
 
-  {
-    // Iterator throw.
-
-    const readable = new Readable({
-      objectMode: true,
-      read() {
-        this.push('hello');
-      }
-    });
-
-    readable.on('error', common.mustCall((err) => {
-      assert.strictEqual(err.message, 'kaboom');
-    }));
-
-    const it = readable[Symbol.asyncIterator]();
-    it.throw(new Error('kaboom')).catch(common.mustCall((err) => {
-      assert.strictEqual(err.message, 'kaboom');
-    }));
-
-    assert.strictEqual(readable.destroyed, true);
-  }
-
   {
     console.log('destroyed by throw');
     const readable = new Readable({
@@ -577,12 +553,15 @@ async function tests() {
       assert.strictEqual(e, err);
     })(), (async () => {
       let e;
+      let x;
       try {
-        await d;
+        x = await d;
       } catch (_e) {
         e = _e;
       }
-      assert.strictEqual(e, err);
+      assert.strictEqual(e, undefined);
+      assert.strictEqual(x.done, true);
+      assert.strictEqual(x.value, undefined);
     })()]);
   }