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

http: overridable keep-alive behavior of Agent #13005

Closed

Conversation

indutny
Copy link
Member

@indutny indutny commented May 12, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

http

Introduce two overridable Agent methods:

  • keepSocketAlive(socket)
  • reuseSocket(socket, req)

These methods can be overridden by particular Agent class child to
make keep-alive behavior customizable.

Motivation: destroy persisted sockets after some configurable timeout.
It is very non-trivial to do it with available primitives. Such program
will most likely need to poke with undocumented events and methods of
Agent. With introduced API such behavior is easy to implement.

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label May 12, 2017
@indutny
Copy link
Member Author

indutny commented May 12, 2017

cc @nodejs/http @nodejs/collaborators

@nodejs/ctc this is mostly a proposal for a new API, so your attention is very desired.

@refack
Copy link
Contributor

refack commented May 12, 2017

Is this much of a performance boost. I thought creating sockets was cheap(ish)...

@indutny
Copy link
Member Author

indutny commented May 12, 2017

@refack this doesn't change performance in any way. Just a way to override defaults.

@refack
Copy link
Contributor

refack commented May 12, 2017

I assume that reusing the socket has some benefit?

@indutny
Copy link
Member Author

indutny commented May 12, 2017

@refack it has many benefits: congestion improvement, DNS lookup time economy, SYN+ACK time saving.

@refack
Copy link
Contributor

refack commented May 12, 2017

Got it. You're just exposing internal keep-alive semantics for overriding...


let expectedSocket;

agent.keepSocketAlive = (socket) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrap this in common.mustCall(...., 3) and remove the two unnecessary assertions (one in here and at the end)?

return false;
};

agent.reuseSocket = (socket, req) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto, wrap in common.mustCall().

const http = require('http');

let keepSocketAliveCalled = 0;
let reuseSocketCalled = 0;
Copy link
Contributor

@mscdex mscdex May 13, 2017

Choose a reason for hiding this comment

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

Both of these variables are unused

Copy link
Member Author

Choose a reason for hiding this comment

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

Overlooked, thanks!

@mscdex
Copy link
Contributor

mscdex commented May 13, 2017

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

return true;
};

Agent.prototype.reuseSocket = function reuseSocket(socket, req) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is req passed in here? It appears to be unused. Is it for agents that override the function and want to use it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is correct.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Couple of doc nits. Otherwise looks fine to me.

doc/api/http.md Outdated
* `socket.setKeepAlive()` with the `agent.keepAliveMsecs` as an argument

This method can be overridden by particular `Agent` class child. If this method
returns `false`y value - the socket will be destroyed instead of persisting it
Copy link
Member

Choose a reason for hiding this comment

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

returns a falsy value

Copy link
Member

@Trott Trott May 21, 2017

Choose a reason for hiding this comment

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

Nit: Dash (-) should be changed to a comma (,).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack.

<!-- YAML
added: REPLACEME
-->

Copy link
Member

Choose a reason for hiding this comment

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

this should list the arguments... e.g.

* `socket` {net.Socket|tls.TLSSocket} description...

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack.

doc/api/http.md Outdated
<!-- YAML
added: REPLACEME
-->

Copy link
Member

Choose a reason for hiding this comment

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

as above... this should list the arguments...

* `socket` {net.Socket|tls.TLSSocket} ...
* `req` ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack.

@rvagg
Copy link
Member

rvagg commented May 24, 2017

+1

@rvagg
Copy link
Member

rvagg commented May 24, 2017

Brought up in CTC meeting today, nothing was added there, no objections and I voiced my support for this. Unless there's an objection brought up here I don't see why this couldn't move forward.

@jasnell
Copy link
Member

jasnell commented May 24, 2017

I do have one outstanding comment that has not been addressed: #13005 (comment)

@mikeal
Copy link
Contributor

mikeal commented May 25, 2017

Can we get a citgm run on this @MylesBorins? This is a historically brittle area of code where modules tend to depend on esoteric and undocumented internal semantics.

@refack
Copy link
Contributor

refack commented May 25, 2017

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

CITGM is clean.
Rubber stamp code.

doc/api/http.md Outdated
* `socket.unref()`
* `socket.setKeepAlive()` with the `agent.keepAliveMsecs` as an argument

This method can be overridden by particular `Agent` class child. If this method
Copy link
Contributor

Choose a reason for hiding this comment

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

s/by/by a/

Also, I'm not sure that "Agent class child" is the best terminology? What about 'subclass' or 'implementation'?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack.

jasnell pushed a commit that referenced this pull request Jun 7, 2017
Introduce two overridable `Agent` methods:

* `keepSocketAlive(socket)`
* `reuseSocket(socket, req)`

These methods can be overridden by particular `Agent` class child to
make keep-alive behavior customizable.

Motivation: destroy persisted sockets after some configurable timeout.
It is very non-trivial to do it with available primitives. Such program
will most likely need to poke with undocumented events and methods of
`Agent`. With introduced API such behavior is easy to implement.

PR-URL: #13005
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@Trott Trott removed the ctc-review label Jun 11, 2017
@indutny
Copy link
Member Author

indutny commented Aug 2, 2017

@nodejs/lts was it ever backported to v6.x?

@MylesBorins
Copy link
Contributor

@indutny not yet, it will land in a future semver minor

@MylesBorins
Copy link
Contributor

LTS WG agreed that this can be packported

@MylesBorins
Copy link
Contributor

@indutny this will need to be manually backported. Would you be able to open one?

MylesBorins pushed a commit that referenced this pull request Jan 18, 2018
Introduce two overridable `Agent` methods:

* `keepSocketAlive(socket)`
* `reuseSocket(socket, req)`

These methods can be overridden by particular `Agent` class child to
make keep-alive behavior customizable.

Motivation: destroy persisted sockets after some configurable timeout.
It is very non-trivial to do it with available primitives. Such program
will most likely need to poke with undocumented events and methods of
`Agent`. With introduced API such behavior is easy to implement.

Backport-PR-URL: #18168
PR-URL: #13005
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2018
MylesBorins pushed a commit that referenced this pull request Feb 11, 2018
Introduce two overridable `Agent` methods:

* `keepSocketAlive(socket)`
* `reuseSocket(socket, req)`

These methods can be overridden by particular `Agent` class child to
make keep-alive behavior customizable.

Motivation: destroy persisted sockets after some configurable timeout.
It is very non-trivial to do it with available primitives. Such program
will most likely need to poke with undocumented events and methods of
`Agent`. With introduced API such behavior is easy to implement.

Backport-PR-URL: #18168
PR-URL: #13005
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins added a commit that referenced this pull request Feb 11, 2018
This LTS release comes with 109 commits, 17 of which are considered
Semver-Minor. This includes 32 which are doc related, 29 which are test
related, 8 which are build / tool related and 1 commit which updates
a dependency.

Notable Changes:

* console:
  - added console.count() and console.clear() (James M Snell)
    #12678
* crypto:
  - expose ECDH class (Bryan English)
    #8188
  - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas)
    #10209
  - warn on invalid authentication tag length (Tobias Nießen)
    #17566
* deps:
  - upgrade libuv to 1.16.1 (cjihrig)
    #16835
* dgram:
  - added socket.setMulticastInterface() (Will Young)
    #7855
* http:
  - add agent.keepSocketAlive and agent.reuseSocket as to allow
    overridable keep-alive behavior of `Agent` (Fedor Indutny)
    #13005
* lib:
  - return this from net.Socket.end() (Sam Roberts)
    #13481
* module:
  - add builtinModules api that provides list of all builtin modules in
    Node (Jon Moss)
    #16386
* net:
  - return this from getConnections() (Sam Roberts)
    #13553
* promises:
  - more robust stringification for unhandled rejections (Timothy Gu)
    #13784
* repl:
  - improve require() autocompletion (Alexey Orlenko)
    #14409
* src:
  - add openssl-system-ca-path configure option (Daniel Bevenius)
    #16790
  - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius)
    #12087
  - add process.ppid (cjihrig)
    #16839
* tls:
  - accept `lookup` option for `tls.connect()` (Fedor Indutny)
    #12839
* tools, build:
  - a new macOS installer! (JP Wesselink)
    #15179
* url:
  - WHATWG URL api support (James M Snell)
    #7448
* util:
  - add %i and %f formatting specifiers (Roman Reiss)
    #10308

PR-URL: #18342
MylesBorins pushed a commit that referenced this pull request Feb 12, 2018
Introduce two overridable `Agent` methods:

* `keepSocketAlive(socket)`
* `reuseSocket(socket, req)`

These methods can be overridden by particular `Agent` class child to
make keep-alive behavior customizable.

Motivation: destroy persisted sockets after some configurable timeout.
It is very non-trivial to do it with available primitives. Such program
will most likely need to poke with undocumented events and methods of
`Agent`. With introduced API such behavior is easy to implement.

Backport-PR-URL: #18168
PR-URL: #13005
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins added a commit that referenced this pull request Feb 12, 2018
This LTS release comes with 109 commits, 17 of which are considered
Semver-Minor. This includes 32 which are doc related, 29 which are test
related, 8 which are build / tool related and 1 commit which updates
a dependency.

Notable Changes:

* console:
  - added console.count() and console.clear() (James M Snell)
    #12678
* crypto:
  - expose ECDH class (Bryan English)
    #8188
  - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas)
    #10209
  - warn on invalid authentication tag length (Tobias Nießen)
    #17566
* deps:
  - upgrade libuv to 1.16.1 (cjihrig)
    #16835
* dgram:
  - added socket.setMulticastInterface() (Will Young)
    #7855
* http:
  - add agent.keepSocketAlive and agent.reuseSocket as to allow
    overridable keep-alive behavior of `Agent` (Fedor Indutny)
    #13005
* lib:
  - return this from net.Socket.end() (Sam Roberts)
    #13481
* module:
  - add builtinModules api that provides list of all builtin modules in
    Node (Jon Moss)
    #16386
* net:
  - return this from getConnections() (Sam Roberts)
    #13553
* promises:
  - more robust stringification for unhandled rejections (Timothy Gu)
    #13784
* repl:
  - improve require() autocompletion (Alexey Orlenko)
    #14409
* src:
  - add openssl-system-ca-path configure option (Daniel Bevenius)
    #16790
  - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius)
    #12087
  - add process.ppid (cjihrig)
    #16839
* tls:
  - accept `lookup` option for `tls.connect()` (Fedor Indutny)
    #12839
* tools, build:
  - a new macOS installer! (JP Wesselink)
    #15179
* url:
  - WHATWG URL api support (James M Snell)
    #7448
* util:
  - add %i and %f formatting specifiers (Roman Reiss)
    #10308

PR-URL: #18342
MylesBorins pushed a commit that referenced this pull request Feb 13, 2018
Introduce two overridable `Agent` methods:

* `keepSocketAlive(socket)`
* `reuseSocket(socket, req)`

These methods can be overridden by particular `Agent` class child to
make keep-alive behavior customizable.

Motivation: destroy persisted sockets after some configurable timeout.
It is very non-trivial to do it with available primitives. Such program
will most likely need to poke with undocumented events and methods of
`Agent`. With introduced API such behavior is easy to implement.

Backport-PR-URL: #18168
PR-URL: #13005
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins added a commit that referenced this pull request Feb 13, 2018
This LTS release comes with 109 commits, 17 of which are considered
Semver-Minor. This includes 32 which are doc related, 29 which are test
related, 8 which are build / tool related and 1 commit which updates
a dependency.

Notable Changes:

* console:
  - added console.count() and console.clear() (James M Snell)
    #12678
* crypto:
  - expose ECDH class (Bryan English)
    #8188
  - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas)
    #10209
  - warn on invalid authentication tag length (Tobias Nießen)
    #17566
* deps:
  - upgrade libuv to 1.16.1 (cjihrig)
    #16835
* dgram:
  - added socket.setMulticastInterface() (Will Young)
    #7855
* http:
  - add agent.keepSocketAlive and agent.reuseSocket as to allow
    overridable keep-alive behavior of `Agent` (Fedor Indutny)
    #13005
* lib:
  - return this from net.Socket.end() (Sam Roberts)
    #13481
* module:
  - add builtinModules api that provides list of all builtin modules in
    Node (Jon Moss)
    #16386
* net:
  - return this from getConnections() (Sam Roberts)
    #13553
* promises:
  - more robust stringification for unhandled rejections (Timothy Gu)
    #13784
* repl:
  - improve require() autocompletion (Alexey Orlenko)
    #14409
* src:
  - add openssl-system-ca-path configure option (Daniel Bevenius)
    #16790
  - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius)
    #12087
  - add process.ppid (cjihrig)
    #16839
* tls:
  - accept `lookup` option for `tls.connect()` (Fedor Indutny)
    #12839
* tools, build:
  - a new macOS installer! (JP Wesselink)
    #15179
* url:
  - WHATWG URL api support (James M Snell)
    #7448
* util:
  - add %i and %f formatting specifiers (Roman Reiss)
    #10308

PR-URL: #18342
MylesBorins added a commit that referenced this pull request Feb 13, 2018
This LTS release comes with 112 commits, 17 of which are considered
Semver-Minor. This includes 32 which are doc related, 30 which are test
related, 8 which are build / tool related and 1 commit which updates
a dependency.

Notable Changes:

* console:
  - added console.count() and console.clear() (James M Snell)
    #12678
* crypto:
  - expose ECDH class (Bryan English)
    #8188
  - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas)
    #10209
  - warn on invalid authentication tag length (Tobias Nießen)
    #17566
* deps:
  - upgrade libuv to 1.16.1 (cjihrig)
    #16835
* dgram:
  - added socket.setMulticastInterface() (Will Young)
    #7855
* http:
  - add agent.keepSocketAlive and agent.reuseSocket as to allow
    overridable keep-alive behavior of `Agent` (Fedor Indutny)
    #13005
* lib:
  - return this from net.Socket.end() (Sam Roberts)
    #13481
* module:
  - add builtinModules api that provides list of all builtin modules in
    Node (Jon Moss)
    #16386
* net:
  - return this from getConnections() (Sam Roberts)
    #13553
* promises:
  - more robust stringification for unhandled rejections (Timothy Gu)
    #13784
* repl:
  - improve require() autocompletion (Alexey Orlenko)
    #14409
* src:
  - add openssl-system-ca-path configure option (Daniel Bevenius)
    #16790
  - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius)
    #12087
  - add process.ppid (cjihrig)
    #16839
* tls:
  - accept `lookup` option for `tls.connect()` (Fedor Indutny)
    #12839
* tools, build:
  - a new macOS installer! (JP Wesselink)
    #15179
* url:
  - WHATWG URL api support (James M Snell)
    #7448
* util:
  - add %i and %f formatting specifiers (Roman Reiss)
    #10308

PR-URL: #18342
MylesBorins added a commit that referenced this pull request Feb 13, 2018
This LTS release comes with 112 commits, 17 of which are considered
Semver-Minor. This includes 32 which are doc related, 30 which are test
related, 8 which are build / tool related and 1 commit which updates
a dependency.

Notable Changes:

* console:
  - added console.count() and console.clear() (James M Snell)
    #12678
* crypto:
  - expose ECDH class (Bryan English)
    #8188
  - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas)
    #10209
  - warn on invalid authentication tag length (Tobias Nießen)
    #17566
* deps:
  - upgrade libuv to 1.16.1 (cjihrig)
    #16835
* dgram:
  - added socket.setMulticastInterface() (Will Young)
    #7855
* http:
  - add agent.keepSocketAlive and agent.reuseSocket as to allow
    overridable keep-alive behavior of `Agent` (Fedor Indutny)
    #13005
* lib:
  - return this from net.Socket.end() (Sam Roberts)
    #13481
* module:
  - add builtinModules api that provides list of all builtin modules in
    Node (Jon Moss)
    #16386
* net:
  - return this from getConnections() (Sam Roberts)
    #13553
* promises:
  - more robust stringification for unhandled rejections (Timothy Gu)
    #13784
* repl:
  - improve require() autocompletion (Alexey Orlenko)
    #14409
* src:
  - add openssl-system-ca-path configure option (Daniel Bevenius)
    #16790
  - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius)
    #12087
  - add process.ppid (cjihrig)
    #16839
* tls:
  - accept `lookup` option for `tls.connect()` (Fedor Indutny)
    #12839
* tools, build:
  - a new macOS installer! (JP Wesselink)
    #15179
* url:
  - WHATWG URL api support (James M Snell)
    #7448
* util:
  - add %i and %f formatting specifiers (Roman Reiss)
    #10308

PR-URL: #18342
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
This LTS release comes with 112 commits, 17 of which are considered
Semver-Minor. This includes 32 which are doc related, 30 which are test
related, 8 which are build / tool related and 1 commit which updates
a dependency.

Notable Changes:

* console:
  - added console.count() and console.clear() (James M Snell)
    nodejs#12678
* crypto:
  - expose ECDH class (Bryan English)
    nodejs#8188
  - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas)
    nodejs#10209
  - warn on invalid authentication tag length (Tobias Nießen)
    nodejs#17566
* deps:
  - upgrade libuv to 1.16.1 (cjihrig)
    nodejs#16835
* dgram:
  - added socket.setMulticastInterface() (Will Young)
    nodejs#7855
* http:
  - add agent.keepSocketAlive and agent.reuseSocket as to allow
    overridable keep-alive behavior of `Agent` (Fedor Indutny)
    nodejs#13005
* lib:
  - return this from net.Socket.end() (Sam Roberts)
    nodejs#13481
* module:
  - add builtinModules api that provides list of all builtin modules in
    Node (Jon Moss)
    nodejs#16386
* net:
  - return this from getConnections() (Sam Roberts)
    nodejs#13553
* promises:
  - more robust stringification for unhandled rejections (Timothy Gu)
    nodejs#13784
* repl:
  - improve require() autocompletion (Alexey Orlenko)
    nodejs#14409
* src:
  - add openssl-system-ca-path configure option (Daniel Bevenius)
    nodejs#16790
  - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius)
    nodejs#12087
  - add process.ppid (cjihrig)
    nodejs#16839
* tls:
  - accept `lookup` option for `tls.connect()` (Fedor Indutny)
    nodejs#12839
* tools, build:
  - a new macOS installer! (JP Wesselink)
    nodejs#15179
* url:
  - WHATWG URL api support (James M Snell)
    nodejs#7448
* util:
  - add %i and %f formatting specifiers (Roman Reiss)
    nodejs#10308

PR-URL: nodejs#18342
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request May 5, 2024
Introduce two overridable `Agent` methods:

* `keepSocketAlive(socket)`
* `reuseSocket(socket, req)`

These methods can be overridden by particular `Agent` class child to
make keep-alive behavior customizable.

Motivation: destroy persisted sockets after some configurable timeout.
It is very non-trivial to do it with available primitives. Such program
will most likely need to poke with undocumented events and methods of
`Agent`. With introduced API such behavior is easy to implement.

PR-URL: nodejs/node#13005
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.