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

docs: fix description of _writev() #28690

Closed
wants to merge 1 commit into from

Conversation

parkerbjur
Copy link

@parkerbjur parkerbjur commented Jul 15, 2019

The documentation gave the impression that when the _writev method is
used, it is used instead of _write. However, when _writev is used,
_write loads the first chunk of data, then _writev loads all the
remaining bufferred chunks in the write queue. The docs have been
changed to reflect this behavior.

Fixes: #28408

Checklist

The documentation gave the impression that when the _writev method is
used, it is used instead of _write. However, when _writev is used,
_write loads the first chunk of data, then _writev loads all the
remaining bufferred chunks in the write queue. The docs have been
changed to reflect this behavior.

Fixes: nodejs#28408
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. labels Jul 15, 2019
multiple chunks of data at once. If implemented, the method will be called with
all chunks of data currently buffered in the write queue.
multiple chunks of data at once. If implemented, the first chunk will be written with
`_write()`. When the initial write is complete, buffered chunks in the write queue, if available,
Copy link
Member

@Trott Trott Jul 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure our linter will complain that this line is longer than 80 chars and has trailing white space. At least I hope it will flag those things. Run make lint-md (or vcbuild lint-md if on Windows) to run the documentation linter locally.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also: Welcome @tyof45 and thanks for the pull request!

multiple chunks of data at once. If implemented, the method will be called with
all chunks of data currently buffered in the write queue.
multiple chunks of data at once. If implemented, the first chunk will be written with
`_write()`. When the initial write is complete, buffered chunks in the write queue, if available,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it’s not just the first chunk, but rather, _write() is always used when there is only one chunk in the queue?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the queue is empty.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't current docs correct in that case @addaleax @lpinca ? Though still bit ambiguous. Does below seems more correct -

if implemented, only the writable._writev() method will be called with all chunks of data currently buffered in the write queue. In case there is only a single data chunk to be processed writable._write() is called instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erabhimanyu I think your suggestion is still confusing. In particular this sentence

In case there is only a single data chunk to be processed writable._write() is called instead

If there is buffered data even a single chunk, writable._writev() will be used.

@BridgeAR
Copy link
Member

Ping @tyof45 this needs a rebase.

@HarshithaKP HarshithaKP mentioned this pull request Jan 14, 2020
4 tasks
HarshithaKP added a commit to HarshithaKP/node that referenced this pull request Jan 16, 2020
the exact context of invocation of _writev API is not well known
also, the choice between _write and _writev is not well known.
add a description to make it explicit.

Fixes: nodejs#28408
Refs: nodejs#28690

Co-authored-by: Parker Bjur <bjur.parker45@gmail.com>
Trott pushed a commit that referenced this pull request Jan 16, 2020
The exact context of invocation of _writev API is not well known.
Also, the choice between _write and _writev is not well known.
Add a description to make it explicit.

Fixes: #28408
Refs: #28690

Co-authored-by: Parker Bjur <bjur.parker45@gmail.com>

PR-URL: #31356
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Feb 17, 2020
The exact context of invocation of _writev API is not well known.
Also, the choice between _write and _writev is not well known.
Add a description to make it explicit.

Fixes: #28408
Refs: #28690

Co-authored-by: Parker Bjur <bjur.parker45@gmail.com>

PR-URL: #31356
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 14, 2020
The exact context of invocation of _writev API is not well known.
Also, the choice between _write and _writev is not well known.
Add a description to make it explicit.

Fixes: #28408
Refs: #28690

Co-authored-by: Parker Bjur <bjur.parker45@gmail.com>

PR-URL: #31356
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
The exact context of invocation of _writev API is not well known.
Also, the choice between _write and _writev is not well known.
Add a description to make it explicit.

Fixes: #28408
Refs: #28690

Co-authored-by: Parker Bjur <bjur.parker45@gmail.com>

PR-URL: #31356
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@HarshithaKP
Copy link
Member

This can be closed as the intention is fulfilled in #31356.

@Trott Trott closed this Apr 4, 2020
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 this pull request may close these issues.

writable stream _write and _writev
8 participants