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

doc: reword ambigous docs for socket.setNoDelay #7995

Closed
wants to merge 1 commit into from

Conversation

Xerkus
Copy link

@Xerkus Xerkus commented Aug 6, 2016

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

Reworded description of socket.setNoDelay to make it clear that function parameter defaults to true and not the noDelay option itself.
See nodejs/node-v0.x-archive#8929

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. net Issues and PRs related to the net subsystem. labels Aug 6, 2016
@@ -625,15 +625,16 @@ initialDelay will leave the value unchanged from the default

Returns `socket`.

### socket.setNoDelay([noDelay])
### socket.setNoDelay([enable])
Copy link
Member

Choose a reason for hiding this comment

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

This breaks the links, I presume.

The following line should also be changed:

doc/api/http.md:1524:[`socket.setNoDelay()`]: net.html#net_socket_setnodelay_nodelay

Copy link
Member

Choose a reason for hiding this comment

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

That said, I'm not sure if it really makes sense to rename the argument to enable.

Thoughs?

Copy link
Author

Choose a reason for hiding this comment

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

it is too ambiguous otherwise and parameter name in code is enable: https://github.com/nodejs/node/blob/master/lib/net.js#L337

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't help, we used to have two concepts, "no delay" and "true/false", and we could tell that "nodelay" (the param name and method name) could be set to true.

Now, you enable (something?) and when something is enabled, "no delay" is... what? true? false? So I have to map enable to true, and true to "no delay" being on, which is a hop too many.

Copy link
Contributor

@sam-github sam-github Dec 6, 2016

Choose a reason for hiding this comment

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

or maybe when "enable" is false, nagle is disabled... so "no delay" is true? In which case, the meaning of true/false to this function would be inverted, so that's an API change, and we would never do it.

connections use the Nagle algorithm, they buffer data before sending it off.
Setting `true` for `noDelay` will immediately fire off data each time
`socket.write()` is called.
`enable` defaults to `true`.
Copy link
Member

Choose a reason for hiding this comment

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

Another way this could be reworded is:

By default, TCP connections use the Nagle algorithm to buffer data before
sending. Calling `socket.setNoDelay(true)` disables such buffering so that
data is sent each time `socket.write()` is called.

The `enable` argument defaults to `true`.

@silverwind
Copy link
Contributor

Also to note, the assumption By default TCP connections use the Nagle algorithm is not entirely correct. Node.js currently does not explicitely set this socket option, it just happens to be the default on Linux, but not on Windows for example. #906 shall eventually resolve this, but I haven't gotten around to it yet.

@rvagg rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02
@jasnell jasnell added the stalled Issues and PRs that are stalled. label Mar 1, 2017
@jasnell
Copy link
Member

jasnell commented Mar 1, 2017

Still want to pursue this?

@Xerkus
Copy link
Author

Xerkus commented Mar 1, 2017

I am not a native speaker, and there were objections to the change. I am not sure if i was wrong here.

@jasnell
Copy link
Member

jasnell commented Mar 1, 2017

@Xerkus ... no problem. I'll keep it open and will take a look to see what else we can do here.

@fhinkel
Copy link
Member

fhinkel commented May 26, 2017

@silverwind and @sam-github can you make suggestions how you'd rather have the documentation worded so we can change it and land it? Thanks.

@sam-github
Copy link
Contributor

sam-github commented May 26, 2017

We did make suggestions, didn't we? EDIT: yes, there were specific suggestions made, including text by @jasnell, that the OP has not followed up on.

@jasnell
Copy link
Member

jasnell commented Aug 24, 2017

Closing due to lack of forward progress on this. It's likely still a change that should be made tho.

@jasnell jasnell closed this Aug 24, 2017
@chrisbutler
Copy link

please please please can we fix this madness? i'd be happy to take a stab at it

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. net Issues and PRs related to the net subsystem. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants