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

[v8.x fix] - stream: fix end not being called when reading some base64 files #21089

Closed
wants to merge 1 commit into from
Closed

Conversation

marcosc90
Copy link
Contributor

@marcosc90 marcosc90 commented Jun 2, 2018

This fixes 'end' not being called when reading base64 files
with highWaterMark equal to: file length - 1,
and fixes 'data' not being called on some edge cases
when highWaterMark was lower than file length.

Fixes: #19862

This issue is still present in v6.x & v9.x so if everything is ok, I'll submit changes for those versions too.

Going through master history, the same line of code was changed in this commit

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

This fixes 'end' not being called when reading base64 files
with highWaterMark equal to: file length - 1,
and fixes 'data' not being called on some edge cases
when highWaterMark was lower than file length.

Fixes: #19862
@nodejs-github-bot nodejs-github-bot added stream Issues and PRs related to the stream subsystem. v8.x labels Jun 2, 2018
@marcosc90 marcosc90 changed the title stream: fix end not being called when reading some base64 files [v8.x fix] - stream: fix end not being called when reading some base64 files Jun 2, 2018
Copy link
Contributor

@mscdex mscdex left a comment

Choose a reason for hiding this comment

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

I think we should instead look at backporting whichever commit(s) fixed this particular issue in master rather than pushing some new changes to one particular branch.

@addaleax
Copy link
Member

addaleax commented Jun 2, 2018

@nodejs/streams – I can’t tell whether this might be breaking or not

@mcollina
Copy link
Member

mcollina commented Jun 2, 2018

I think this is breaking. Unfortunately the full fix in #17979, which is semver-major for good reasons. We might want to backport that if we need to fix is needed.

@marcosc90
Copy link
Contributor Author

Backporting those commits it's not something I would be comfortable with, since I'm not very familiar with the codebase yet. But I would be willing to add some better tests to master to prevent that bug from occurring again.

@mcollina
Copy link
Member

mcollina commented Jun 5, 2018

@marcosc90 go for it. We definitely need better testing on streams.

@MylesBorins
Copy link
Contributor

I'm closing this based on the above comments

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants