Skip to content

stream: make sure readable is called on empty chunk followed by eof #29762

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.

Sign up for GitHub

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

Closed
ronag wants to merge 4 commits into from
Closed

stream: make sure readable is called on empty chunk followed by eof #29762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
92bb697
stream: make sure readable is called on empty chunk followed by eof
ronag Sep 29, 2019
2c720a1
fixup: less significant changes
ronag Sep 29, 2019
2f13cdd
fixup: test
ronag Sep 29, 2019
9253f5e
fixup: test
ronag Sep 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion lib/_stream_readable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,8 @@ function readableAddChunk(stream, chunk, encoding, addToFront) {
}
} else if (!addToFront) {
state.reading = false;
if (state.needReadable)
emitReadable(stream);
maybeReadMore(stream, state);
}
}
Expand Down Expand Up @@ -593,9 +595,12 @@ function emitReadable_(stream) {
debug('emitReadable_', state.destroyed, state.length, state.ended);
if (!state.destroyed && (state.length || state.ended)) {
stream.emit('readable');
state.emittedReadable = false;
}

// Reset emittedReadable once it is safe to schedule another
// emitReadable.
state.emittedReadable = false;
Copy link
Member Author

@ronag ronag Sep 30, 2019

Choose a reason for hiding this comment

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

@mcollina: Please also observe this change. I'm pretty sure it's fine but just making sure I have a second opinion on that.


// The stream needs another readable event if
// 1. It is not flowing, as the flow mechanism will take
// care of it.
Expand Down
19 changes: 19 additions & 0 deletions test/parallel/test-stream-duplex-empty-chunk.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
'use strict';

const common = require('../common');
const makeDuplexPair = require('../common/duplexpair');

const { clientSide, serverSide } = makeDuplexPair();

serverSide.resume();
serverSide.on('end', function(buf) {
serverSide.write('out');
serverSide.write('');
serverSide.write('');
serverSide.end();
});

clientSide.end();

clientSide.on('data', common.mustCall());
clientSide.on('end', common.mustCall());
2 changes: 1 addition & 1 deletion test/parallel/test-stream2-compatibility.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ class TestReader extends R {
}

const reader = new TestReader();
setImmediate(function() {
process.nextTick(function() {
Copy link
Member Author

@ronag ronag Sep 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This test kind of contradicts the added test, i.e. it expects the stream to stop reading after pushing an empty chunk.

Not sure if this is something that indicates whether this fix will cause other problems.

Copy link
Member

@mcollina mcollina Sep 30, 2019

Choose a reason for hiding this comment

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

Can you please articulate this a bit more? Is this change needed to make the test pass?

Copy link
Member Author

@ronag ronag Sep 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, otherwise it live locks. Basically it gets into an infinite loop (with nextTick) between. Because it pushes an empty chunk, which invokes a read call which pushes an empty chunk etc...

By changing it to nextTick instead of setImmediate it's possible to get between that loop and end it. But as I wrote I'm unsure of the implications.

Copy link
Member

@mcollina mcollina Sep 30, 2019

Choose a reason for hiding this comment

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

I think we should avoid that live lock in code.

Copy link
Member Author

@ronag ronag Sep 30, 2019

Choose a reason for hiding this comment

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

I think we should avoid that live lock in code.

What does that mean in practice? Should we add a guard? Or is the current state of the PR ok in that regard?

Copy link
Member

@BridgeAR BridgeAR Oct 1, 2019

Choose a reason for hiding this comment

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

I think @mcollina would like to add a guard in the streams implementation that this can not cause an infinite loop / that this test does not need to change.

assert.strictEqual(ondataCalled, 1);
console.log('ok');
reader.push(null);
Expand Down