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: clarify socket and keepAliveTimeout interaction #25748

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@inversion
Copy link
Contributor

inversion commented Jan 27, 2019

doc: clarify socket and keepAliveTimeout interaction

Socket timeouts set using the connection event are replaced by
server.keepAliveTimeout when a response is handled.

Socket timeouts set in the connection event are replaced after a response is sent in the resOnFinish handler

server.on('connection', function(socket) {
  socket.setTimeout(60000);
});

In this case, the socket timeout is 60 seconds until a response is sent, at which point it becomes 5 seconds (the server.keepAliveTimeout default). This bit me when working with load balancers that maintain long-lived connections.

I have added a note to the connection event docs to the effect that is preferred to use server.keepAliveTimeout.

Checklist

@inversion inversion force-pushed the inversion:doc-clarify-http-keepAliveTimeout branch from 344dfc6 to 4696232 Jan 28, 2019

@mscdex

This comment has been minimized.

Copy link
Contributor

mscdex commented Jan 28, 2019

The subsystem prefix in commit messages for doc-only changes should be doc instead of the specific core module name.

@@ -901,6 +901,10 @@ also be accessed at `request.connection`.
This event can also be explicitly emitted by users to inject connections
into the HTTP server. In that case, any [`Duplex`][] stream can be passed.

If `socket.setTimeout()` is called here, note that the timeout will

This comment has been minimized.

@Trott

Trott Jan 28, 2019

Member
Suggested change Beta
If `socket.setTimeout()` is called here, note that the timeout will
If `socket.setTimeout()` is called here, the timeout will

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt Feb 3, 2019

Member

@Trott Are we landing as is or can we add this fix on landing?

This comment has been minimized.

@lpinca

lpinca Feb 8, 2019

Member

@inversion are you ok with @Trott suggestion? If so please accept it so this can land.
Thank you.

This comment has been minimized.

@Trott

Trott Feb 9, 2019

Member

My change is non-blocking. It's not crucial that note that be removed. (It can also be removed by someone else who is landing this--just push to the branch. Or it can be removed in a subsequent PR as I imagine there are lots of opportunities for cleanup in the doc file.)

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Jan 28, 2019

Hi, @inversion! Welcome and thanks for the pull request! Sorry to hear that this behavior caused you some grief!

/ping @nodejs/http to confirm that the behavior described here is correct and is currently undocumented.

/ping @nodejs/documentation Maybe there's a way to simplify or clarify the wording here a bit? Maybe a small piece of sample code with comments indicating the values and when they change? Or is that too much documentation real estate to describe what is likely an edge case?

@inversion

This comment has been minimized.

Copy link
Contributor Author

inversion commented Jan 28, 2019

The subsystem prefix in commit messages for doc-only changes should be doc instead of the specific core module name.

I'll fix this, probably worth adding it to https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#commit-message-guidelines

@lpinca

lpinca approved these changes Jan 28, 2019

doc: clarify http timeouts
Socket timeouts set using the `connection` event are replaced by
server.keepAliveTimeout when a response is handled.

@Trott Trott force-pushed the inversion:doc-clarify-http-keepAliveTimeout branch from 4696232 to 5f761e9 Jan 28, 2019

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Jan 28, 2019

I fixed up the commit message subsystem, rebased against master, and force-pushed.

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/2423/

@Trott Trott changed the title http: Clarify socket and keepAliveTimeout interaction doc: clarify socket and keepAliveTimeout interaction Jan 28, 2019

@Trott Trott added the author ready label Jan 28, 2019

@inversion

This comment has been minimized.

Copy link
Contributor Author

inversion commented Jan 29, 2019

Thanks very much @Trott !

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Feb 3, 2019

@nodejs/collaborators This could use another review.

Show resolved Hide resolved doc/api/http.md

lpinca added a commit to lpinca/node that referenced this pull request Feb 4, 2019

http: remove redundant call to socket.setTimeout()
`Socket.prototype.setTimeout()` clears the previous timer before setting
a new one.

Refs: nodejs#25748 (comment)

@lpinca lpinca referenced this pull request Feb 4, 2019

Closed

http: remove redundant call to socket.setTimeout() #25928

2 of 2 tasks complete

addaleax added a commit that referenced this pull request Feb 6, 2019

http: remove redundant call to socket.setTimeout()
`Socket.prototype.setTimeout()` clears the previous timer before setting
a new one.

Refs: #25748 (comment)

PR-URL: #25928
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax added a commit that referenced this pull request Feb 6, 2019

http: remove redundant call to socket.setTimeout()
`Socket.prototype.setTimeout()` clears the previous timer before setting
a new one.

Refs: #25748 (comment)

PR-URL: #25928
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@mcollina
Copy link
Member

mcollina left a comment

LGTM

@lpinca

This comment has been minimized.

@lpinca

This comment has been minimized.

Copy link
Member

lpinca commented Feb 9, 2019

Landed in 02601c2.

@lpinca lpinca closed this Feb 9, 2019

lpinca added a commit that referenced this pull request Feb 9, 2019

doc: clarify http timeouts
Socket timeouts set using the `'connection'` event are replaced by
`server.keepAliveTimeout` when a response is handled.

PR-URL: #25748
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

@inversion inversion deleted the inversion:doc-clarify-http-keepAliveTimeout branch Feb 9, 2019

targos added a commit that referenced this pull request Feb 10, 2019

doc: clarify http timeouts
Socket timeouts set using the `'connection'` event are replaced by
`server.keepAliveTimeout` when a response is handled.

PR-URL: #25748
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

@targos targos referenced this pull request Feb 14, 2019

Merged

v11.10.0 proposal #26098

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment