streams: backpressure is broken in multi push cases on master

[mafintosh]

• Version: master (eeb5702)
• Platform: Linux
• Subsystem: streams

Try out the following test case with a fast readable stream, and a slow writable one that are piped together. There seems to be a bug in master where the readable one is not backpressured properly when multiple pushes are done in the read fn.

var stream = require('stream')

var rs = stream.Readable({
  read: function () {
    this.push(Buffer.alloc(65500))
    for (var i = 0; i < 40; i++) {
      this.push(Buffer.alloc(1024))
    }
  }
})

var ws = stream.Writable({
  write: function (data, enc, cb) {
    setTimeout(cb, 10)
  }
})

setInterval(function () {
  const state = rs._readableState
  console.log('state.length %d, state.buffer.length %d, state.hwm %d', state.length, state.buffer.length, state.highWaterMark)
}, 1000)

rs.pipe(ws)

Notice how the internal buffer just keeps growing

[mafintosh]

/cc @nodejs/streams @mcollina

[addaleax]

Confirmed that this is coming from 1e0f331, and in particular I believe that state.needReadable is being set to true here when it shouldn’t:

node/lib/_stream_readable.js

Line 539 in cde98ce

state.needReadable = !state.flowing && !state.ended;

[mcollina]

I'm traveling this week, so I will have time to have a look next week, or this week during my flights.
If someone beats me to fixing this earlier, please don't wait for me.

@addaleax it's definitely that line. However, something else is at play: needReadable was always true in there. I'll keep everybody posted.

[mafintosh]

I think that line needs a `&& !state.length`

…

On Mon, Mar 26, 2018, 08:48 Matteo Collina ***@***.***> wrote: I'm traveling this week, so I will have time to have a look next week, or this week during my flights. If someone beats me to fixing this earlier, please don't wait for me. @addaleax <https://github.com/addaleax> it's definitely that line. However, something else is at play: needReadable was always true in there. I'll keep everybody posted. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#19601 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAW_VQK1GrJ24_N-5rVe8p05XJK1tlo2ks5tiI9GgaJpZM4S6VoO> .

[mcollina]

I think it should be state.needReadable = state.needReadable && !state.flowing && !state.ended. It fixes the test and it passes dicer test suite. Doing a full test run right now.

[mcollina]

Here is my proposed fix: #19613.