Readable stream change from v10 to v11 with for-await loop

[btd]

Hi.

We noticed that our app breaking v11 of nodejs. We located this to pg-query-stream. Simplified test case:

"use strict";

const { Readable } = require("stream");

class MyStream extends Readable {
  constructor() {
    super({ objectMode: true });

    this._reading = false;
    this._closed = false;

    this._count = 5;
  }

  _read(size) {
    if (this._reading || this._closed) {
      return false;
    }
    this._reading = true;

    setTimeout(() => {
      if (this._closed) {
        return;
      }

      if (this._count === 0) {
        this._closed = true;
        setImmediate(() => this.emit("close"));
        return this.push(null);
      }

      this._reading = false;
      this._count -= 1;

      this.push({ count: this._count });
    }, 100);
  }

  close(callback) {
    this._closed = true;
    const cb = callback || (() => this.emit("close"));
    setImmediate(cb);
  }
}

const delay = timeout => new Promise(resolve => setTimeout(() => resolve(), timeout));

const run = async () => {
  const stream = new MyStream();

  for await (const row of stream) {
    console.log(row);
    await delay(1000);
  }
};

run();

Results:

$ node test.js
(node:3199) ExperimentalWarning: Readable[Symbol.asyncIterator] is an experimental feature. This feature could change at any time
{ count: 4 }
{ count: 3 }
{ count: 2 }
{ count: 1 }
{ count: 0 }
$ node --version
v10.15.2

and

$ node test.js                                                                                    
(node:1657) ExperimentalWarning: Readable[Symbol.asyncIterator] is an experimental feature. This feature could change at any time
{ count: 4 }
$ node --version
v11.9.0

I tried to search for breaking changes and issues but did not noticed anythign similar. Also i understand that asyncIterator is experimenal, but this looks like some sort of bug.
I am thinking this could be a bug because stream already pushed items and after this it emits close event (this is from pg-query-stream), but in v11 iteration stops right after close event, ignoring already pushed items.

[ZYSzys]

/cc @nodejs/streams

FYI, this change seems come from v11.4 to v11.5

[mcollina]

The code is working as expected. You are emitting “close” too early. “close” means that there is nothing more to read, and the iteration ends.

Note that it is emitted automatically when there is nothing else to read. In other terms, do not emit close and it will all work.

[btd]

@mcollina yes i know reason is 'close' event. I specifically wrote it is problem in pg-query-stream.
Should not iteration continues for already pushed items?

[mcollina]

The problem is that it is a bug in pg-query-stream, not here. It should not be emitting that event there.

From https://nodejs.org/api/stream.html#stream_event_close_1:

The 'close' event is emitted when the stream and any of its underlying resources (a file descriptor, for example) have been closed. The event indicates that no more events will be emitted, and no further computation will occur.

Note that the behavior of the iterator is actually correct according to this docs.

You should open an issue on pg-query-stream about the close event. Note that will also break pump and stream.finished and stream.pipeline.

[btd]

@mcollina thank you for explanation. I created bug.