-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: add note regarding net.Socket default timeout #31379
Conversation
5ffe33f
to
4c10965
Compare
This should also be added to master for consistency. |
Also, this kind of note should probably be moved to each of the separate, relevant ( |
Reviewing this it seems like perhaps this should target master and then get backported to 12.x. If it is relevant to master we really shouldn't start with targeting LTS |
Prior to Node.js v13, http[2s] have a specific default timeout value which should not be confused with net.Socket default timeout. Fixes: nodejs#31378 Fixes: nodejs#27556 Refs: nodejs#27704
4c10965
to
97f0ad8
Compare
To me, docs there are already specific enough regarding the default timeout value, what are the changes you suggest @mscdex? |
@aduh95 I meant the change in default timeout should be noted in the documentation for each appropriate subsystem, most likely listed in the |
@@ -895,6 +895,9 @@ added: v0.1.90 | |||
Sets the socket to timeout after `timeout` milliseconds of inactivity on | |||
the socket. By default `net.Socket` do not have a timeout. | |||
|
|||
Prior to Node.js 13.0.0, [`http.Server`][], [`https.Server`][] and | |||
[`http2.Http2Server`][] have a different default socket timeout value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we incorporate this information into the YAML matter instead of having it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we incorporate this information into the YAML matter instead of having it here?
Oh, wait, no, not that YAML matter. What's the rationale for putting this in net
instead of http
, https
, and http2
docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently some users are referring to net
docs when working with http.Server
(see #31378); prior to Node.js 13, default values are consistent across all sockets in Node.js codebase EXCEPT for the timeout, I figured adding a note would help avoid the confusion.
Users who refer to http
[s2] docs already have the correct information, I'm not sure there is much confusion from that side.
In that case, can we just leave it out here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making it explicit that this should not land as-is for the reasons given by the others:
- This should not go into the
net
docs - This should be presented as YAML metadata.
I’m going to close this, docs on http* modules provide the correct information, I haven’t seen any report (except #31378) of a confusion on that area of the docs, and the default timeout thing doesn’t exist anymore as of Node.js 13. All in all, I don’t think it’s worth it, if someone feels differently, you’re welcome to open another PR. We could also close the linked issues maybe. |
Adding a note stating the difference between default timeout on http[2s] servers and on
net.Socket
.Should be backported to v10 as well.
Fixes: #31378
Fixes: #27556
Refs: #27704
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes