Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stream: remove unreachable code #18239

Closed
wants to merge 2 commits into from

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Jan 18, 2018

First commit remove some unreachable code. BufferList.prototype.concat() is not called when there is only a buffer in the list.

Second commit fixes a test.

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, test

To avoid a function call `BufferList.prototype.concat()` is not called
when there is only a buffer in the list. That buffer is instead
accessed directly.
The `n` argument of `BufferList.prototype.concat()` is not the number
of `Buffer` instances in the list, but their total length when
concatenated.
@lpinca lpinca added stream Issues and PRs related to the stream subsystem. test Issues and PRs related to the tests. labels Jan 18, 2018
@MoonBall
Copy link
Member

MoonBall commented Jan 20, 2018

It's better to modify function fromList(n, state) in lib/_stream_readable.js by deleting the code, because BufferList is a independent module.

@lpinca
Copy link
Member Author

lpinca commented Jan 20, 2018

@MoonBall BufferList is an internal module so it doesn't really matter and I guess it was done like this to avoid the function call.

The _stream_readable module is plenty of code that use internal details of BufferList.

@MoonBall
Copy link
Member

ok. I agree.

@targos
Copy link
Member

targos commented Jan 20, 2018

/cc @nodejs/streams

@mcollina
Copy link
Member

I'm not super comfortable with this change.
What would happen if it happens? Any error would be good.

@lpinca
Copy link
Member Author

lpinca commented Jan 20, 2018

What would happen if it happens?

Do you mean if BufferList#concat() is called when there is only one element in the list and this patch is applied? If so, nothing, it will still work as expected just without the fast path (which is already here).

@mcollina
Copy link
Member

Can you restore the test then?


assert.strictEqual(list.concat(1), 'foo');
Copy link
Member Author

@lpinca lpinca Jan 20, 2018

Choose a reason for hiding this comment

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

@mcollina this is wrong whether or not the change in BufferList.js is applied.

  • The test uses a string, probably for simplicity, but concat() only works with buffers.
  • The n argument (1) was wrong and irrelevant. You could have used any argument and the test would have still passed.

@lpinca
Copy link
Member Author

lpinca commented Jan 20, 2018

FWIW the fixed test actually tests the impossible case where BufferList#concat() is called and there is only one buffer in the list. Unlike the fast path, it creates a copy.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM, perfect

@lpinca
Copy link
Member Author

lpinca commented Jan 21, 2018

lpinca added a commit that referenced this pull request Jan 24, 2018
To avoid a function call `BufferList.prototype.concat()` is not called
when there is only a buffer in the list. That buffer is instead
accessed directly.

PR-URL: #18239
Reviewed-By: Matteo Collina <[email protected]>
lpinca added a commit that referenced this pull request Jan 24, 2018
The `n` argument of `BufferList.prototype.concat()` is not the number
of `Buffer` instances in the list, but their total length when
concatenated.

PR-URL: #18239
Reviewed-By: Matteo Collina <[email protected]>
@lpinca
Copy link
Member Author

lpinca commented Jan 24, 2018

Landed in e65a6e8...2313424.

@lpinca lpinca closed this Jan 24, 2018
@lpinca lpinca deleted the remove/unreachable-code branch January 24, 2018 15:32
evanlucas pushed a commit that referenced this pull request Jan 30, 2018
To avoid a function call `BufferList.prototype.concat()` is not called
when there is only a buffer in the list. That buffer is instead
accessed directly.

PR-URL: #18239
Reviewed-By: Matteo Collina <[email protected]>
evanlucas pushed a commit that referenced this pull request Jan 30, 2018
The `n` argument of `BufferList.prototype.concat()` is not the number
of `Buffer` instances in the list, but their total length when
concatenated.

PR-URL: #18239
Reviewed-By: Matteo Collina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants