Stream.Writable reports wrong number in _writableState.bufferedRequestCount

[andrey-makhnutin]

• Version: v6.1.0
• Platform: Windows 10 64-bit
• Subsystem: Stream

Was casually reading _stream_writable.js and noticed that clearBuffer() mistakenly zeroes state.bufferedRequestCount at the end of the function in case when _writev is not implemented and _write is not synchronous. The while (entry) loop is breaken from, leaving data in the buffer, but the request counter is zeroed out anyway.

Here's the testing code

'use strict';

const Stream = require('stream');

class testWritable extends Stream.Writable {
  constructor() {
    super({objectMode: true});
  }

  _write(chunk, encoding, cb) {
    console.log(`_writing chunk ${chunk}`);
    setTimeout(cb, 1000);
  }
}

const testStream = new testWritable();

testStream.cork();
for (let i = 1; i <= 5; ++i) {
  testStream.write(i, () => { 
    console.log(`chunk ${i} cb called`);
    console.log(`_writableState.bufferedRequestCount = ${testStream._writableState.bufferedRequestCount}`);
    console.log(`real buffered request count = ${testStream._writableState.getBuffer().length}`);
  });
}
testStream.end();
console.log('main program ends here');

And the output:

_writing chunk 1
main program ends here
_writing chunk 2
chunk 1 cb called
_writableState.bufferedRequestCount = 0
real buffered request count = 3
_writing chunk 3
chunk 2 cb called
_writableState.bufferedRequestCount = 0
real buffered request count = 2
_writing chunk 4
chunk 3 cb called
_writableState.bufferedRequestCount = 0
real buffered request count = 1
_writing chunk 5
chunk 4 cb called
_writableState.bufferedRequestCount = 0
real buffered request count = 0
chunk 5 cb called
_writableState.bufferedRequestCount = 0
real buffered request count = 0

The implications of this are super low, I understand, but you know, just in case.

[Trott]

@nodejs/streams

[mcollina]

@Trott the bug still exists, but it does not have any implication. #6761 had no followup after a review.

Is there anyone that wants to help on this?

[andrey-makhnutin]

Since I'm the troublemaker here, let me try to give it a go.
I will try to continue and hopefully end #6761 this week

[mcollina]

@andrey-makhnutin I reopened that PR, if you needed that open.

[BridgeAR]

The bug itself is not difficult to fix (have a look at #6761). That PR just needs a test, so it is a awesome good first contribution!

[jlvivero]

I'd like to give it a go, this is my first time contributing thought, so It'll take a while to get used to the workflow. It seems the fix itself is already on a pull request (but the pull request is closed), should I make a pull request with the same change but adding tests? Or is the fix wrong and should I also look into how to fix it properly (comment above me suggests it works fine, but I'd like to make sure)

[BridgeAR]

@jlvivero the fix should be fine as it and it only needs a test. Ideally you could cherry-pick the commit and add a test on top of that or you just do the same change on your own and add a test.