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: fix no data on partial decode #5226

Closed
wants to merge 1 commit into from

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Feb 14, 2016

Before this commit, it was possible to push a partial character to a readable stream where it was decoded as an empty string and then added to the internal buffer. This caused the stream to not emit any data,
even when the rest of the character bytes were pushed separately, because of a non-zero length check of the first chunk in the internal buffer.

Fixes: #5223

@mscdex mscdex added the stream Issues and PRs related to the stream subsystem. label Feb 14, 2016
@mscdex
Copy link
Contributor Author

mscdex commented Feb 14, 2016

@@ -136,26 +136,31 @@ function readableAddChunk(stream, state, chunk, encoding, addToFront) {
const e = new Error('stream.unshift() after end event');
stream.emit('error', e);
} else {
if (state.decoder && !addToFront && !encoding)
var skipAdd;
Copy link
Member

Choose a reason for hiding this comment

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

I would add a little comment here just to clarify why this is being introduced.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment added.

@mcollina
Copy link
Member

LGTM.

Just a quick check, any perf regression? That if (skipAdd) sits in a fairly hot path.

Before this commit, it was possible to push a partial character
to a readable stream where it was decoded as an empty string and
then added to the internal buffer. This caused the stream to not
emit any data, even when the rest of the character bytes were pushed
separately, because of a non-zero length check of the first chunk in
the internal buffer.

Fixes: nodejs#5223
@mscdex
Copy link
Contributor Author

mscdex commented Feb 14, 2016

No idea about perf, there aren't any pure stream benchmarks at the moment.

@mcollina
Copy link
Member

The net ones support passing both buffers and utf8 encoded things, you should be able to get some data (with some noise)

@mscdex
Copy link
Contributor Author

mscdex commented Feb 14, 2016

IMHO there'd be too much variance in those benchmarks to measure any performance difference with a change like this. No matter what though, the bug needs to be fixed.

@mcollina
Copy link
Member

Agreed, LGTM.

@jasnell
Copy link
Member

jasnell commented Feb 15, 2016

LGTM

mscdex added a commit that referenced this pull request Feb 17, 2016
Before this commit, it was possible to push a partial character
to a readable stream where it was decoded as an empty string and
then added to the internal buffer. This caused the stream to not
emit any data, even when the rest of the character bytes were pushed
separately, because of a non-zero length check of the first chunk in
the internal buffer.

Fixes: #5223
PR-URL: #5226
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@mscdex
Copy link
Contributor Author

mscdex commented Feb 17, 2016

Landed in 9221201.

@mscdex mscdex closed this Feb 17, 2016
@mscdex mscdex deleted the fix-readable-partial-decode branch February 17, 2016 19:17
rvagg pushed a commit that referenced this pull request Feb 18, 2016
Before this commit, it was possible to push a partial character
to a readable stream where it was decoded as an empty string and
then added to the internal buffer. This caused the stream to not
emit any data, even when the rest of the character bytes were pushed
separately, because of a non-zero length check of the first chunk in
the internal buffer.

Fixes: #5223
PR-URL: #5226
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 1, 2016
Before this commit, it was possible to push a partial character
to a readable stream where it was decoded as an empty string and
then added to the internal buffer. This caused the stream to not
emit any data, even when the rest of the character bytes were pushed
separately, because of a non-zero length check of the first chunk in
the internal buffer.

Fixes: #5223
PR-URL: #5226
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 17, 2016
Before this commit, it was possible to push a partial character
to a readable stream where it was decoded as an empty string and
then added to the internal buffer. This caused the stream to not
emit any data, even when the rest of the character bytes were pushed
separately, because of a non-zero length check of the first chunk in
the internal buffer.

Fixes: #5223
PR-URL: #5226
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
Before this commit, it was possible to push a partial character
to a readable stream where it was decoded as an empty string and
then added to the internal buffer. This caused the stream to not
emit any data, even when the rest of the character bytes were pushed
separately, because of a non-zero length check of the first chunk in
the internal buffer.

Fixes: #5223
PR-URL: #5226
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants