stream: Correct bufferedRequestCount in writes

[ngsankha]

Checklist

• tests and code linting passes
• a test and/or benchmark is included
• documentation is changed or added
• the commit message follows commit guidelines

Affected core subsystem(s)

stream

Description of change

_writableState.bufferedRequestCount now gives the correct value if the _write is asynchronous. Fixes #6758.

[mscdex]

/cc @nodejs/streams

[calvinmetcalf]

new test please that will fail before the change and not after

[calvinmetcalf]

you can likely adapt the test from #6758

[mcollina]

First of all, the "bug" is harmless as bufferedWriteCount is used only to increase the throughput of _writev, and that value is completely useless when _writev is not present. The count is actually accurate when _writev is present, because the affected branch in clearBuffer is executed only when there is a single element in the buffer. Things are done this way to save a couple of CPU cycles, maybe with negligible performance impact. This is probably not explained well in https://github.com/nodejs/node/blob/master/lib/_stream_writable.js#L112-L113 (my bad), feel free to add a more explanatory comment there.

This might cause a bug if _writev is added during the life of a stream, but that's probably a mad use case.

I'm generally 👍, pending a comparative benchmark analysis, as clearBuffer is probably one of the hottest function in core. I am confident this will turn out to be ok, but let's check.

Some more considerations:

1. as far as I know, _writableState is not part of the supported API
2. I do not think we have any tests testing _writableState or _readableState, and I am not 100% sure we should if we do not put those in the public API
3. I think we should put part of _writableState and _readableState in the public API, and they should be tested as well.

[mcollina]

@sankha93 any updates on this? This would need a test.

cc @italoacasas

[fhinkel]

There hasn't been any activity here. I'm closing this. Feel free to reopen (or ping a collaborator) if I closed this in error.

[jasnell]

ping @mcollina ... what's the status on this?

[mcollina]

@jasnell It seems @sankha93 is not committed to adding a test. I'll edit his commit and include one in a new PR. I'll close this when I file the new one.

[BridgeAR]

Ping @mcollina

[mcollina]

@BridgeAR I'm a bit busy atm. If someone else would like to take care of this, it would be fantastic.

[BridgeAR]

I am closing this due to the long inactivity and add a good first contribution label on the bug issue.

@sankha93 thanks a lot for your contribution. Your work is much appreciated nevertheless. If you would like to pursue this further, please leave a comment or open a new PR.