stream: readable stream continues to read when pushing a empty string by MoonBall · Pull Request #18211 · nodejs/node (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation36 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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

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.

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

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

Can you also rebase on top of master?

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

mcollina added the semver-major

PRs that contain breaking changes and should be released in the next major version.

label

Feb 2, 2018

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

mcollina pushed a commit that referenced this pull request

Feb 15, 2018

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

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request

May 8, 2018

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

Labels

semver-major

PRs that contain breaking changes and should be released in the next major version.

stream

Issues and PRs related to the stream subsystem.