GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — stream: remove unreachable code by lpinca · Pull Request #18239 · nodejs/node

Conversation

@lpinca

@lpinca lpinca commented Jan 18, 2018

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
lpinca added 2 commits January 18, 2018 18:39
@lpinca
stream: remove unreachable code
5593b51 
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.
@lpinca
test: use correct size in test-stream-buffer-list
4461fa5 
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 Node.js streams. test Issues and PRs related to Node.js core tests and test infrastructure. labels Jan 18, 2018
@MoonBall

MoonBall commented Jan 20, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lpinca

lpinca commented Jan 20, 2018

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MoonBall

MoonBall commented Jan 20, 2018

Copy link
Copy Markdown
Member

ok. I agree.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@targos

targos commented Jan 20, 2018

Copy link
Copy Markdown
Member

/cc @nodejs/streams
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented Jan 20, 2018

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lpinca

lpinca commented Jan 20, 2018

Copy link
Copy Markdown
Member Author

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).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented Jan 20, 2018

Copy link
Copy Markdown
Member

Can you restore the test then?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lpinca
lpinca commented Jan 20, 2018
View reviewed changes
Comment thread test/parallel/test-stream-buffer-list.js

const copy = list.concat(3);

assert.strictEqual(list.concat(1), 'foo');

@lpinca lpinca Jan 20, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@lpinca

lpinca commented Jan 20, 2018

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mcollina
mcollina approved these changes Jan 20, 2018
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM, perfect

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@lpinca

lpinca commented Jan 21, 2018

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/12650/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lpinca added a commit that referenced this pull request Jan 24, 2018
@lpinca
stream: remove unreachable code
80b3acc 
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
@lpinca
test: use correct size in test-stream-buffer-list
2313424 
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

lpinca commented Jan 24, 2018

Copy link
Copy Markdown
Member Author

Landed in e65a6e8...2313424.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lpinca lpinca closed this Jan 24, 2018
@lpinca
lpinca deleted the remove/unreachable-code branch January 24, 2018 15:32
@evanlucas evanlucas mentioned this pull request Jan 30, 2018
v9.5.0 release proposal #18464
Merged
evanlucas pushed a commit that referenced this pull request Jan 30, 2018
@lpinca @evanlucas
stream: remove unreachable code
f7d57d0 
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
@lpinca @evanlucas
test: use correct size in test-stream-buffer-list
7d27228 
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]>
This was referenced Apr 7, 2024
[Snyk] Security upgrade eslint from 4.19.1 to 9.0.0 aliscco/alisco-node#1365
Open
[Snyk] Security upgrade eslint from 4.19.1 to 9.0.0 aliscco/alisco-node#1366
Open
This was referenced Apr 7, 2024
[Snyk] Security upgrade eslint from 8.57.0 to 9.0.0 leonardoadame/node#760
Open
[Snyk] Security upgrade eslint from 8.57.0 to 9.0.0 leonardoadame/node#761
Open
[Snyk] Security upgrade eslint from 8.14.0 to 9.0.0 leonardoadame/node#762
Open
[Snyk] Security upgrade eslint from 7.32.0 to 9.0.0 leonardoadame/node#763
Open
[Snyk] Security upgrade eslint from 8.14.0 to 9.0.0 leonardoadame/node#764
Open
This was referenced Apr 7, 2024
[Snyk] Security upgrade eslint from 5.16.0 to 9.0.0 aliscco/alisco-node#1367
Open
[Snyk] Security upgrade eslint from 6.8.0 to 9.0.0 aliscco/alisco-node#1368
Open
[Snyk] Security upgrade eslint from 6.8.0 to 9.0.0 aliscco/alisco-node#1369
Open
[Snyk] Security upgrade eslint from 7.32.0 to 9.0.0 aliscco/alisco-node#1370
Open
[Snyk] Security upgrade eslint from 7.32.0 to 9.0.0 aliscco/alisco-node#1371
Open
@baby636 baby636 mentioned this pull request Apr 7, 2024
[Snyk] Security upgrade eslint from 7.32.0 to 9.0.0 baby636/node#145
Open
@ryan-ally ryan-ally mentioned this pull request Apr 7, 2024
[Snyk] Security upgrade eslint from 6.8.0 to 9.0.0 ryan-ally/node#106
Open
This was referenced Apr 7, 2024
[Snyk] Security upgrade eslint from 7.32.0 to 9.0.0 leonardoadame/node#765
Open
[Snyk] Security upgrade eslint from 8.57.0 to 9.0.0 leonardoadame/node#766
Open
[Snyk] Security upgrade eslint from 4.12.1 to 9.0.0 leonardoadame/node#767
Open
[Snyk] Security upgrade eslint from 8.37.0 to 9.0.0 leonardoadame/node#768
Open
[Snyk] Security upgrade eslint from 7.32.0 to 9.0.0 leonardoadame/node#769
Open
@aliscco aliscco mentioned this pull request Apr 7, 2024
[Snyk] Security upgrade eslint from 3.19.0 to 9.0.0 aliscco/alisco-node#1372
Open
@leonardoadame leonardoadame mentioned this pull request Apr 7, 2024
[Snyk] Security upgrade eslint from 3.19.0 to 9.0.0 leonardoadame/node#770
Open
This was referenced Apr 8, 2024
[Snyk] Security upgrade eslint from 4.19.1 to 9.0.0 aliscco/alisco-node#1375
Open
[Snyk] Security upgrade eslint from 7.32.0 to 9.0.0 aliscco/alisco-node#1376
Open
[Snyk] Security upgrade eslint from 5.16.0 to 9.0.0 aliscco/alisco-node#1377
Open
@wedataintelligence wedataintelligence mentioned this pull request Oct 9, 2024
[Snyk] Fix for 3 vulnerabilities wedataintelligence/node#127
Open
@ron-a20 ron-a20 mentioned this pull request Oct 9, 2024
[Snyk] Fix for 3 vulnerabilities ron-a20/node#142
Open
@aliceUnhinged613 aliceUnhinged613 mentioned this pull request Oct 11, 2024
[Snyk] Fix for 4 vulnerabilities aliceUnhinged613/node#146
Open
This was referenced Oct 21, 2024
[Snyk] Fix for 6 vulnerabilities Graybar-codespace/node#38
Open
[Snyk] Fix for 16 vulnerabilities Graybar-codespace/node#61
Open
[Snyk] Fix for 5 vulnerabilities Graybar-codespace/node#93
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

stream Issues and PRs related to Node.js streams. test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@lpinca @MoonBall @targos @mcollina @MylesBorins
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.