GeezerVison

ping @nodejs/streams @mcollina

From reading your test I'm not sure I completely understand what the problem is?

@mafintosh From the view of a nodejs user, I think a readable stream should end if only calling read() in a 'readable' listener. The key code is as below:

const r = new Readable();
r.on('readable', () => {
  r.read();
});
r.on('end', () => {
  // calling `read()` in a 'readable' listener should ultimately emit the 'end' event.
});

But if _read() pushes a empty string or undefined, the key code doesn't emit the 'end' event. But I'm not sure that the situation exists. It can be avoided by stream implementors.

I am 👎 to this change. undefined is a legit value in object mode stream, and it should not terminate.

Yes, I'm agreeing with @mcollina as well. Stream termination should be as explicit as possible. I think we should consider having an explicit API for it at some point.

@mcollina @mafintosh My purpose may be misunderstood. I updated the test so that you can better understand it. I am so sorry for my poor english.

We can just consider that this.push() pushes a empty string. I updated the title of the PR.

Here are some automated runs:

CI: https://ci.nodejs.org/job/node-test-pull-request/12611/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1209/

Anyway, if we want to relax this it needs to be done earlier: https://github.com/MoonBall/node/blob/49ed0814f2750075fb64ab167f9a129f70f37d52/lib/_stream_readable.js#L234 esplicitly checks for a string longer than zero. This check was made pretty explicit, so if we want to make this change we should edit it there.

I'm not super-confident in adding '' as a valid chunk, because you have actually written nothing and the stream knows it should push things. However I see why it could improve the API.

cc @mafintosh?

The linter seems to have failed with:

10:49:31 not ok 36 - /usr/home/iojs/build/workspace/node-test-linter/test/parallel/test-stream-readable-event.js
10:49:31 ---
10:49:31 message: '''underlyingData'' is never reassigned. Use ''const'' instead.'
10:49:31 severity: error
10:49:31 data:
10:49:31 line: 90
10:49:31 column: 7
10:49:31 ruleId: prefer-const

@addaleax I fixed it.
@mcollina I changed the code as you suggested.

Can you also rebase on top of master?

ok.

Rebased.

@mcollina In non-objectMode, we can call this.push() without a param. Is it valid?
The code of the PR will throw Error, if pushing a undefined in non-objectMode.

I fixed that pushing undefined in another PR(#18244).

What happens if you push undefined with this change?

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1215/

@mcollina
if I push undefined, the following code will execute.

      if (typeof chunk !== 'string' &&
          !state.objectMode &&
          Object.getPrototypeOf(chunk) !== Buffer.prototype) {
        chunk = Stream._uint8ArrayToBuffer(chunk);
      }

The error is:

_stream_readable.js:237
          Object.getPrototypeOf(chunk) !== Buffer.prototype) {
                 ^

TypeError: Cannot convert undefined or null to object
    at Function.getPrototypeOf (<anonymous>)
    at readableAddChunk (_stream_readable.js:237:18)
    at Readable.push (_stream_readable.js:215:10)

I doesn't fix it, because I think undefined is invalid. But, if we allow to push undefined in non-object mode, I will fix it.

@mcollina I modified the code because that push() maybe pass a undefined chunk.

I'm not super convinced by this change, I'll have more time to review it in a week.
Also, I would like to fix #18294 before landing this, as they are definitely connected.

@mafintosh can you check this?

Let me try and look into it tonight

@mcollina can you please have another look? #18294 got fixed and some time passed by.

@MoonBall @mcollina The tests doesn't pass for me when rebasing on top of latest master, do they for you? Also the test isn't completely clear still imo. Does this make it emit an empty string on read?

My bad with the tests, was using the wrong branch. They pass.

@MoonBall what happens if you do push(Buffer.alloc(0)) ?

@mafintosh The result is same if I push() a empty Buffer, a empty string or undefined because of the condition.

@MoonBall Thanks for the explanation! Added a comment on your test case. @mcollina I'm leaning 👍 on this, think it makes sense.

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1290/
CI: https://ci.nodejs.org/job/node-test-pull-request/13159/

Landing

Landed as faeee11.