stream: readable stream continues to read when pushing a empty string #18211

MoonBall · 2018-01-17T18:49:48Z

Checklist

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

Affected core subsystem(s)

stream

jasnell · 2018-01-17T21:30:56Z

ping @nodejs/streams @mcollina

mafintosh · 2018-01-17T21:40:22Z

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

MoonBall · 2018-01-18T00:40:39Z

@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.

mcollina · 2018-01-18T10:08:21Z

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

mafintosh · 2018-01-18T10:24:12Z

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.

MoonBall · 2018-01-18T15:22:41Z

@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.

MoonBall · 2018-01-18T15:33:44Z

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

mcollina · 2018-01-18T15:52:17Z

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?

addaleax · 2018-01-18T16:36:16Z

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

MoonBall · 2018-01-18T16:45:55Z

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

mcollina · 2018-01-18T16:50:40Z

Can you also rebase on top of master?

MoonBall · 2018-01-18T16:54:35Z

ok.

MoonBall · 2018-01-18T18:02:07Z

Rebased.

MoonBall · 2018-01-19T01:46:00Z

@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).

mcollina · 2018-01-19T11:05:58Z

What happens if you push undefined with this change?

mcollina · 2018-01-19T11:06:47Z

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

MoonBall · 2018-01-19T13:34:50Z

@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.

MoonBall · 2018-01-22T15:34:20Z

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

mcollina · 2018-01-23T12:22:25Z

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?

mafintosh · 2018-01-23T17:08:58Z

Let me try and look into it tonight

BridgeAR · 2018-02-01T16:33:57Z

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

mcollina

I'm not convinced that changing this is the right thing to do, yet. Maybe somebody else should weight in on why.

BTW, this is semver-major.

mafintosh · 2018-02-02T23:04:48Z

@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?

mafintosh · 2018-02-02T23:13:08Z

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

mafintosh · 2018-02-02T23:18:53Z

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

MoonBall · 2018-02-03T11:02:25Z

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

mafintosh · 2018-02-03T17:36:47Z

test/parallel/test-stream-readable-event.js

+
+  r.on('readable', () => {
+    const data = r.read();
+    if (data !== null) result += data;


could you collect the chunks in an array and deepEqual those and the end instead of the concatenated string? That'd make it a bit easier for me (and others) to understand and show that it isn't returning an empty string here ever :)

There is a misunderstanding. This change only fixed that the 'end' event isn't emitted when we push a empty string, a empty buffer or undefined.

Currently, a readable stream in non-object mode doesn't inform users a empty string or a empty buffer. It is reasonable.

I'd like it if the test was a bit more explicit.

result += data doesn't tell me if r.read() returns an empty buffer, which from reading the test, you might expect.

Doing result.push(data) and then on end assert.deepEquals(result, [...]) would help

@mafintosh I did it.

mafintosh · 2018-02-03T17:37:17Z

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

mcollina

LGTM

mcollina · 2018-02-14T00:41:42Z

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

mcollina · 2018-02-15T11:39:47Z

Landing

mcollina · 2018-02-15T11:54:30Z

Landed as faeee11.

PR-URL: #18211 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: nodejs#18211 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Jan 17, 2018

MoonBall changed the title ~~stream: readable stream continues to read when pushing undefined~~ stream: readable stream continues to read when pushing a empty string Jan 18, 2018

MoonBall force-pushed the readable-stream-continue-to-read-when-pushing-undefined branch 2 times, most recently from eef4179 to 9547feb Compare January 18, 2018 17:40

MoonBall force-pushed the readable-stream-continue-to-read-when-pushing-undefined branch from 9547feb to aaf9bc7 Compare January 22, 2018 15:31

jasnell approved these changes Jan 22, 2018

View reviewed changes

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

mcollina requested changes Feb 2, 2018

View reviewed changes

mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 2, 2018

mafintosh reviewed Feb 3, 2018

View reviewed changes

MoonBall force-pushed the readable-stream-continue-to-read-when-pushing-undefined branch from aaf9bc7 to 29554ed Compare February 5, 2018 15:03

stream: readable stream continues to read when calling push('')

77a66d3

MoonBall force-pushed the readable-stream-continue-to-read-when-pushing-undefined branch from 29554ed to 77a66d3 Compare February 5, 2018 15:16

BridgeAR approved these changes Feb 13, 2018

View reviewed changes

BridgeAR requested review from mcollina and a team February 13, 2018 05:46

mcollina approved these changes Feb 14, 2018

View reviewed changes

mcollina closed this Feb 15, 2018

stream: readable stream continues to read when pushing a empty string #18211

stream: readable stream continues to read when pushing a empty string #18211

Conversation

MoonBall commented Jan 17, 2018

Checklist

Affected core subsystem(s)

jasnell commented Jan 17, 2018

mafintosh commented Jan 17, 2018

MoonBall commented Jan 18, 2018 • edited Loading

mcollina commented Jan 18, 2018

mafintosh commented Jan 18, 2018

MoonBall commented Jan 18, 2018

MoonBall commented Jan 18, 2018 • edited Loading

mcollina commented Jan 18, 2018

addaleax commented Jan 18, 2018

MoonBall commented Jan 18, 2018

mcollina commented Jan 18, 2018

MoonBall commented Jan 18, 2018

MoonBall commented Jan 18, 2018

MoonBall commented Jan 19, 2018 • edited Loading

mcollina commented Jan 19, 2018

mcollina commented Jan 19, 2018

MoonBall commented Jan 19, 2018 • edited Loading

MoonBall commented Jan 22, 2018

mcollina commented Jan 23, 2018

mafintosh commented Jan 23, 2018

BridgeAR commented Feb 1, 2018

mcollina left a comment

Choose a reason for hiding this comment

mafintosh commented Feb 2, 2018 • edited Loading

mafintosh commented Feb 2, 2018

mafintosh commented Feb 2, 2018

MoonBall commented Feb 3, 2018

mafintosh Feb 3, 2018

Choose a reason for hiding this comment

MoonBall Feb 4, 2018

Choose a reason for hiding this comment

MoonBall Feb 4, 2018 • edited Loading

Choose a reason for hiding this comment

mafintosh Feb 5, 2018

Choose a reason for hiding this comment

MoonBall Feb 5, 2018

Choose a reason for hiding this comment

mafintosh commented Feb 3, 2018

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Feb 14, 2018

mcollina commented Feb 15, 2018

mcollina commented Feb 15, 2018

MoonBall commented Jan 18, 2018 •

edited

Loading

MoonBall commented Jan 18, 2018 •

edited

Loading

MoonBall commented Jan 19, 2018 •

edited

Loading

MoonBall commented Jan 19, 2018 •

edited

Loading

mafintosh commented Feb 2, 2018 •

edited

Loading

MoonBall Feb 4, 2018 •

edited

Loading