fix needDrain emitting to match the amount of pauses

[dignifiedquire]

Trying to work out a test for this, which I will hopefully push soon

[dominictarr]

what is currently happening? the way you phrased it it sounds like it's emitting too many pauses not not enough drains. it should not emit another pause unless it's emitted drain since the last pause.

As a regular expression: /(PD)*/
if it's emitting PPPD that is a too many pauses,
PPPDDD has the same number of pauses and drains, but is not valid.

[dignifiedquire]

I was able to simulate the problem I had in my code with a test, that I just added, the change fixes that. I hope that code explains better what issue I am trying to solve.

[dominictarr]

it would help if you describe the problem? it's harder to interpret code than text

[dignifiedquire]

it would help if you describe the problem? it's harder to interpret code than text

Of course.

Given a src readable stream and a dest writable stream, which are src.pipe(dest) together, the following happens

• If src receives false from a write call it moves into a paused state, increasing an awaitDrain counter and pause itself. (https://github.com/nodejs/node/blob/master/lib/_stream_readable.js#L555-L568)
• Once 'drain' is emitted from dest, it will check the awaitDrain counter, and decrement it. If it is at 0 it will resume it self. (https://github.com/nodejs/node/blob/master/lib/_stream_readable.js#L619-L623)
• On every 'data' event from src it tries to call write on dest, independent of the state of src. (https://github.com/nodejs/node/blob/master/lib/_stream_readable.js#L551-L552)

The above behaviour results in what you mentioned of pausing multiple times one after another, without 'drain' in between. Because there is a counter, which checks how often writes returned false, dest needs to emit the exact same amount of 'drain' events to makes src return into flowing mode.

With the current code where I put the needDrain = false into the nextTick call, this does not happen, as those write() === false calls in between are not tracked properly.

The best solution I could come up with was to move the needDrain = false out, such that it is process synchronously, triggering in the next round a new 'drain' event, corresponding to the write call that returned false.

[dominictarr]

Thanks! Now I understand.

When I learnt nodestreams, it worked the way I described! emitting lots of drain events will still work correctly with old streams. unfortunately they can't describe the state of a node stream with a finite state machine now! you have to have a stack-based automaton.

but if node uses a counter here, I think we should use a counter too, even if we emit drain inside a loop

[dominictarr]

as I think about this more, I realize that actually that change in node streams would not be backwards compatible with older node streams. I think we better figure out what is really going on here

[dominictarr]

for reference, this is the commit that introduced that feature nodejs/node@0f8de5e

[dignifiedquire]

Found the original referenced issue with discussion: nodejs/node-v0.x-archive#5860

[dignifiedquire]

I realize that actually that change in node streams would not be backwards compatible with older node streams

What exactly do you think break compat here, and to which streams version?
In your blog on the history of streams you write

The version of streams now in contemporary node@5 is considered streams3. it’s a significant refactor from streams2, but, is still backwards compatible.

do you think this is not true anymore, or just the implementation in here?

[dominictarr]

I'm a bit confused by this drain counter thing. Although, maybe ... it would make sense if it's for when you pipe a stream to multiple destinations?

I'm gonna study your test code and figure out what is going on there

[dominictarr]

oh, now I see it. this is a big no-no d5b8ffd#diff-168726dbe96b3ce427e7fedce31bb0bcR68

the problem here is that you have emitted the event, then updated the state, but it should be the other way around. You must be very careful with this, because the anything (sync) can happen during the event listener, including triggering other events. In this case, you emit the drain event, which triggers more writes, etc, but the stream still thinks it's no longer needing to drain, when actually, it may have reentered the needDrain state. if you just shift the needDrain = false before the emit, then your test passes.

[dignifiedquire]

@dominictarr rebased based on your changes and passes all my integration tests :)

[dominictarr]

cool, merging!

[dignifiedquire]

@dominictarr I think you published without pushing to github :)

[daviddias]

Hey! Just to confirm, has this been merged? Seems that there is a new version on npm, but the last commit in github master is from 6 days ago

[dominictarr]

yes, sorry - I just forgot which branch I was on.

[dominictarr]

merged into 1.3.2

[dignifiedquire]

Thank you 🙂