Repository navigation
streams: backpressure is broken in multi push cases on master #19601
Description
- 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
Activity
mafintosh commentedon Mar 25, 2018
/cc @nodejs/streams @mcollina
addaleax commentedon Mar 25, 2018
Confirmed that this is coming from 1e0f331, and in particular I believe that
state.needReadableis being set totruehere when it shouldn’t:node/lib/_stream_readable.js
Line 539 in cde98ce
mcollina commentedon Mar 26, 2018
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:
needReadablewas alwaystruein there. I'll keep everybody posted.mafintosh commentedon Mar 26, 2018
mcollina commentedon Mar 26, 2018
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 commentedon Mar 26, 2018
Here is my proposed fix: #19613.
stream: fix backpressure when multiple sync