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

ronag · 2019-09-29T15:57:50Z

An empty chunk should be treated like any other chunk and trigger the corresponding read logic.

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

ronag · 2019-09-29T20:52:32Z

@addaleax @mcollina I think this needs your expertise. I'm not that confident with the Readable stuff but this might be a step in the right direction.

test/parallel/test-streams-highwatermark.js

lib/_stream_readable.js

An empty chunk should be treated like any other chunk and trigger the read logic. Fixes: nodejs#29758

awwright · 2019-09-29T22:42:24Z

This seems to fix the issue on my end.

Should this also be backported?

ronag · 2019-09-29T22:48:33Z

@awwright: Thanks for testing! I'm not sure whether this fix will break something else. We will have to wait for someone more familiar with Readable to chime in.

ronag · 2019-09-29T22:49:31Z

test/parallel/test-stream2-compatibility.js

@@ -44,7 +44,7 @@ class TestReader extends R {
 }

 const reader = new TestReader();
-setImmediate(function() {
+process.nextTick(function() {


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.

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

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.

I think we should avoid that live lock in code.

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?

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.

awwright · 2019-09-29T22:52:27Z

Yeah it looks like test/parallel/test-stream2-compatibility.js timed out for me, I suspect if it breaks that it would break someone in the wild

ronag · 2019-09-29T22:56:06Z

Yeah it looks like test/parallel/test-stream2-compatibility.js timed out for me, I suspect if it breaks that it would break someone in the wild

Yep, the problem is that fixing your issue will per definition break this test, i.e. one of the expects a new read to be scheduled when pushing and empty chunk while the other expects the exact opposite. :/

awwright · 2019-09-29T23:24:07Z

Hm, I'm not very well versed in the details of streams, but both tests here look reasonable; are they really mutually exclusive? There's got to be something more to this.

nodejs-github-bot · 2019-09-30T06:08:39Z

CI: https://ci.nodejs.org/job/node-test-pull-request/25767/

mcollina

I’m unsure about the semver level of this. This fixes a bug for me, but I’m uncertain if it diverges enough to be considered a major.

mcollina · 2019-09-30T07:16:02Z

test/parallel/test-stream2-compatibility.js

@@ -44,7 +44,7 @@ class TestReader extends R {
 }

 const reader = new TestReader();
-setImmediate(function() {
+process.nextTick(function() {


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

mcollina · 2019-09-30T07:18:46Z

Would you mind adding a unit test that only use Readable?

ronag · 2019-09-30T07:37:24Z

lib/_stream_readable.js

  }

+  // Reset emittedReadable once it is safe to schedule another
+  // emitReadable.
+  state.emittedReadable = false;


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

ronag · 2019-09-30T07:52:47Z

@mcollina: I noticed another possible issue while looking into making that Readable test. I would like to investigate it a bit further to see whether it is a problem. Can we get a WIP label on this so it doesn't land by accident.

ronag · 2019-10-02T20:00:53Z

@lpinca: I've been trying to wrap my head around this and I believe something is wrong with DuplexPair.

DuplexPair is as far as I understand under the invariant that every push will result in a _read. However, this breaks down as soon as a push(null) (EOF) is performed (hence the referenced bug).

I think the behaviour of Readable is mostly correct here in that _read is not called since the stream is in ended state. We should NOT call _read in this case since ended means that no further push calls should be performed.

What further confuses me is why this problem is only triggered by empty chunks. As far as I understand empty chunks (for whatever reason) are actually the one behaving correctly.

lpinca · 2019-10-03T05:31:33Z

DuplexPair is as far as I understand under the invariant that every push will result in a _read. However, this breaks down as soon as a push(null) (EOF) is performed (hence the referenced bug).

Yes I agree, that assumption seems wrong.

ronag · 2019-10-03T08:05:17Z

Ok, I think this PR is not correct then. I will try to dig into this from the other direction.

nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Sep 29, 2019

ronag force-pushed the readable-empty-chunk branch 2 times, most recently from fdab801 to 2c0ab44 Compare September 29, 2019 15:58

ronag mentioned this pull request Sep 29, 2019

stream: certain sequence of Writable#write and Writable#end will not end stream #29758

Closed

ronag force-pushed the readable-empty-chunk branch from 2c0ab44 to 3a02aea Compare September 29, 2019 16:06

ronag closed this Sep 29, 2019

ronag reopened this Sep 29, 2019

ronag force-pushed the readable-empty-chunk branch 5 times, most recently from a5c58e9 to eebae1f Compare September 29, 2019 20:51

ronag force-pushed the readable-empty-chunk branch from eebae1f to 4eb889f Compare September 29, 2019 20:55

ronag mentioned this pull request Sep 29, 2019

stream: readable.push(undefined) behaviour? #29765

Closed

ronag commented Sep 29, 2019

View reviewed changes

test/parallel/test-streams-highwatermark.js Outdated Show resolved Hide resolved

ronag commented Sep 29, 2019

View reviewed changes

lib/_stream_readable.js Outdated Show resolved Hide resolved

ronag force-pushed the readable-empty-chunk branch 4 times, most recently from ca14482 to fe92fc7 Compare September 29, 2019 21:23

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

92bb697

An empty chunk should be treated like any other chunk and trigger the read logic. Fixes: nodejs#29758

ronag force-pushed the readable-empty-chunk branch 2 times, most recently from 914138e to 26d983f Compare September 29, 2019 21:35

fixup: less significant changes

2c720a1

ronag force-pushed the readable-empty-chunk branch from 26d983f to 2c720a1 Compare September 29, 2019 21:36

ronag commented Sep 29, 2019

View reviewed changes

fixup: test

2f13cdd

ronag force-pushed the readable-empty-chunk branch from cd10310 to 2f13cdd Compare September 29, 2019 22:51

fixup: test

9253f5e

mcollina reviewed Sep 30, 2019

View reviewed changes

ronag commented Sep 30, 2019

View reviewed changes

lpinca added the wip Issues and PRs that are still a work in progress. label Sep 30, 2019

ronag closed this Oct 3, 2019

ronag mentioned this pull request Oct 3, 2019

test: test server response that waits for request end #29649

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ronag commented Sep 29, 2019 •

edited

Loading

ronag commented Sep 29, 2019 •

edited

Loading

awwright commented Sep 29, 2019

ronag commented Sep 29, 2019

ronag Sep 29, 2019 •

edited

Loading

mcollina Sep 30, 2019

ronag Sep 30, 2019 •

edited

Loading

mcollina Sep 30, 2019

ronag Sep 30, 2019

BridgeAR Oct 1, 2019

awwright commented Sep 29, 2019

ronag commented Sep 29, 2019 •

edited

Loading

awwright commented Sep 29, 2019

nodejs-github-bot commented Sep 30, 2019

mcollina left a comment

mcollina Sep 30, 2019

mcollina commented Sep 30, 2019

ronag Sep 30, 2019

ronag commented Sep 30, 2019 •

edited

Loading

ronag commented Oct 2, 2019 •

edited

Loading

lpinca commented Oct 3, 2019

ronag commented Oct 3, 2019

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

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

Conversation

ronag commented Sep 29, 2019 • edited Loading

Checklist

ronag commented Sep 29, 2019 • edited Loading

awwright commented Sep 29, 2019

ronag commented Sep 29, 2019

ronag Sep 29, 2019 • edited Loading

Choose a reason for hiding this comment

mcollina Sep 30, 2019

Choose a reason for hiding this comment

ronag Sep 30, 2019 • edited Loading

Choose a reason for hiding this comment

mcollina Sep 30, 2019

Choose a reason for hiding this comment

ronag Sep 30, 2019

Choose a reason for hiding this comment

BridgeAR Oct 1, 2019

Choose a reason for hiding this comment

awwright commented Sep 29, 2019

ronag commented Sep 29, 2019 • edited Loading

awwright commented Sep 29, 2019

nodejs-github-bot commented Sep 30, 2019

mcollina left a comment

Choose a reason for hiding this comment

mcollina Sep 30, 2019

Choose a reason for hiding this comment

mcollina commented Sep 30, 2019

ronag Sep 30, 2019

Choose a reason for hiding this comment

ronag commented Sep 30, 2019 • edited Loading

ronag commented Oct 2, 2019 • edited Loading

lpinca commented Oct 3, 2019

ronag commented Oct 3, 2019

ronag commented Sep 29, 2019 •

edited

Loading

ronag commented Sep 29, 2019 •

edited

Loading

ronag Sep 29, 2019 •

edited

Loading

ronag Sep 30, 2019 •

edited

Loading

ronag commented Sep 29, 2019 •

edited

Loading

ronag commented Sep 30, 2019 •

edited

Loading

ronag commented Oct 2, 2019 •

edited

Loading