Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

streams documentation for writable.write(chunk) describes return value incorrectly #9247

Closed
binki opened this issue Oct 24, 2016 · 1 comment
Labels
doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem.

Comments

@binki
Copy link
Contributor

binki commented Oct 24, 2016

The return value indicates whether the written chunk was buffered internally and the buffer has exceeded the highWaterMark configured when the stream was created. If false is returned, further attempts to write data to the stream should be paused until the 'drain' event is emitted.

This suggests that when writable.write(chunk) returns false, the passed chunk was not buffered. In reality, write() always unconditionally buffers the chunk regardless of what it returns. Its return value is only advisory.

I would suggest language like this

The return value is true if the internal buffer does not exceed highWaterMark configured when the stream was created after admitting chunk. If false is returned, further attempts to write data to the stream should be paused until the 'drain' event is emitted. However, the false return value is only advisory and the writable stream will unconditionally accept chunk even if it has not not been allowed to drain.

@mscdex mscdex added stream Issues and PRs related to the stream subsystem. doc Issues and PRs related to the documentations. labels Oct 24, 2016
@tanujasawant tanujasawant mentioned this issue Nov 4, 2016
2 tasks
@binki
Copy link
Contributor Author

binki commented Nov 25, 2016

I was thinking about how highWaterMark works and how I hope it should work today, and I realized that the text I suggested is wrong. I suggested this:

The return value is true if the internal buffer does not exceed highWaterMark

when, in reality, (and more sensibly with how streams should work):

The return value is true if the internal buffer is smaller than highWaterMark

The associated PR needs to be updated accordingly. I’m basing this on my reading of

var ret = state.length < state.highWaterMark;
and also this demo:

> require('through2')({highWaterMark: 1, objectMode: true, }, function (chunk, encoding, callback) {}).write({})
false
> require('through2')({highWaterMark: 2, objectMode: true, }, function (chunk, encoding, callback) {}).write({})
true

binki added a commit to binki/node that referenced this issue Jan 3, 2017
Fix the explanation which stated that write() would return false if
highWaterMark is exceeded to correctly state that false is returned
once highWaterMark is reached. See nodejs#9247.
evanlucas pushed a commit that referenced this issue Jan 3, 2017
stream.md is updated to explain the return value of
writable.write(chunk) precisely.

PR-URL: #9468
Fixes: #9247
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
Reviewed-By: Roman Reiss <me@silverwind.io>
evanlucas pushed a commit that referenced this issue Jan 4, 2017
stream.md is updated to explain the return value of
writable.write(chunk) precisely.

PR-URL: #9468
Fixes: #9247
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
Reviewed-By: Roman Reiss <me@silverwind.io>
mcollina pushed a commit that referenced this issue Jan 12, 2017
Fix the explanation which stated that write() would return false if
highWaterMark is exceeded to correctly state that false is returned
once highWaterMark is reached. See #9247.

PR-URL: #10582
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 18, 2017
Fix the explanation which stated that write() would return false if
highWaterMark is exceeded to correctly state that false is returned
once highWaterMark is reached. See nodejs#9247.

PR-URL: nodejs#10582
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this issue Jan 23, 2017
stream.md is updated to explain the return value of
writable.write(chunk) precisely.

PR-URL: #9468
Fixes: #9247
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
Reviewed-By: Roman Reiss <me@silverwind.io>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 23, 2017
Fix the explanation which stated that write() would return false if
highWaterMark is exceeded to correctly state that false is returned
once highWaterMark is reached. See nodejs#9247.

PR-URL: nodejs#10582
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this issue Jan 24, 2017
stream.md is updated to explain the return value of
writable.write(chunk) precisely.

PR-URL: #9468
Fixes: #9247
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
Reviewed-By: Roman Reiss <me@silverwind.io>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 24, 2017
Fix the explanation which stated that write() would return false if
highWaterMark is exceeded to correctly state that false is returned
once highWaterMark is reached. See nodejs#9247.

PR-URL: nodejs#10582
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 27, 2017
Fix the explanation which stated that write() would return false if
highWaterMark is exceeded to correctly state that false is returned
once highWaterMark is reached. See nodejs#9247.

PR-URL: nodejs#10582
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this issue Jan 31, 2017
stream.md is updated to explain the return value of
writable.write(chunk) precisely.

PR-URL: #9468
Fixes: #9247
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
Reviewed-By: Roman Reiss <me@silverwind.io>
MylesBorins pushed a commit that referenced this issue Mar 7, 2017
Fix the explanation which stated that write() would return false if
highWaterMark is exceeded to correctly state that false is returned
once highWaterMark is reached. See #9247.

PR-URL: #10582
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this issue Mar 9, 2017
Fix the explanation which stated that write() would return false if
highWaterMark is exceeded to correctly state that false is returned
once highWaterMark is reached. See #9247.

PR-URL: #10582
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants