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: client keep-alive for UNIX domain sockets #13214

Closed
wants to merge 2 commits into from

Conversation

bengl
Copy link
Member

@bengl bengl commented May 25, 2017

Makes Connection: keep-alive behave correctly when making client
connections to UNIX domain sockets.

Prior to this, connections would never be re-used, but the keep-alive
would cause the connections to stick around until they time out. This
would lead to an eventual EMFILE error due to all the connections
staying open. This was due to http.Agent not properly supporting UNIX
domain sockets.

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

http, test

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

bengl commented May 25, 2017

@lpinca
Copy link
Member

lpinca commented May 25, 2017

semver-major? It seems that keep-alive was skipped on purpose when using UNIX domain sockets.

@@ -274,7 +259,7 @@ function ClientRequest(options, cb) {
this._last = true;
this.shouldKeepAlive = false;
if (typeof options.createConnection === 'function') {
newSocket = options.createConnection(options, oncreate);
var newSocket = options.createConnection(options, oncreate);
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's initialized, never re-assigned, scope limited, and quacks like a duck; it's const

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call! I left it as var as it had already been that on line 244 previously, but const is more appropriate here now.

var newSocket;
if (this.socketPath) {
this._last = true;
this.shouldKeepAlive = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

@bnoordhuis I tried to do a deep blame, seems like it's your code

node/lib/http.js

Line 1025 in bc7cfd7

self.shouldKeepAlive = false;

Is there a specific reason why POSIX sockets shouldKeepAlive = false?

@jasnell
Copy link
Member

jasnell commented May 25, 2017

this could be viewed as semver-minor, not major, in that it's adding support for something that didn't work previously.

@lpinca
Copy link
Member

lpinca commented May 25, 2017

req.socketPath should be removed from this condition if these changes are ok.

Refs: #10818

@bengl
Copy link
Member Author

bengl commented May 25, 2017

@jasnell @lpinca I'd be tempted to call this semver-patch, because prior to the change, it's not re-using the socket for keep-alive, and too many file descriptors are used, triggering an EMFILE. Seems like a bug to me.

@jasnell
Copy link
Member

jasnell commented May 25, 2017

That works for me.

http.get({
socketPath: common.PIPE,
headers: {connection: 'keep-alive'}
}, (res) => res.on('data', () => {}).on('error', assert.fail).on('end', cb));
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Instead of .on('data', () => {}), use .on('data', common.mustNotCall()) to make it clear that the expectation is that the handler will never execute in this test.

asyncLoop(makeKeepAliveRequest, 10, () =>
server.getConnections(common.mustCall((err, conns) => {
assert.ifError(err);
assert.strictEqual(1, conns);
Copy link
Member

@lpinca lpinca May 26, 2017

Choose a reason for hiding this comment

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

Nit: can you please swap the arguments?


function asyncLoop(fn, times, cb) {
fn(function handler() {
if (--times) {
Copy link
Member

@lpinca lpinca May 26, 2017

Choose a reason for hiding this comment

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

Nit: if I'm not wrong this should be times--.

@@ -130,6 +130,9 @@ Agent.prototype.getName = function getName(options) {
if (options.family === 4 || options.family === 6)
name += ':' + options.family;

if (options.socketPath)
name += ':' + options.socketPath;
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 makes sense to extend test/parallel/test-http-agent-getname.js and cover this.

Copy link
Member

Choose a reason for hiding this comment

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

Nit: the socketPath should not be set when a family, a port or a localAddress is set. Therefore it would be nicer branch wise to write

if (options.socketPath) {
 name += `:::${options.socketPath}`;
} else {
  if (options.port)
    // ...
  if (options.localAddress)
    // ...
  // ...
}

I think it is similar with servername down below.

@lpinca
Copy link
Member

lpinca commented May 26, 2017

@jasnell @lpinca I'd be tempted to call this semver-patch, because prior to the change, it's not re-using the socket for keep-alive, and too many file descriptors are used, triggering an EMFILE. Seems like a bug to me.

Yes, I think you are right.


common.refreshTmpDir();

server.listen(common.PIPE, () =>
Copy link
Member

Choose a reason for hiding this comment

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

common.mustCall() on these?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a common.mustCall() at the callback that's actually making assertions, and in order to get there, this callback needs to be called, so I think it's implied. (Or, is there a best-test-practice for this stating all callbacks should have a mustCall()?)

Copy link
Member

Choose a reason for hiding this comment

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

(Or, is there a best-test-practice for this stating all callbacks should have a mustCall()?)

Nah, it's a matter of judgment. There are certainly callbacks that should not be wrapped (such as callbacks in cluster workers, which will throw but be ignored by the cluster master, so having them wrapped gives a false sense of security that the callback is being executed).

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess I'll put them in anyway to be super clear that these are all expected to run.

@bengl
Copy link
Member Author

bengl commented Jun 2, 2017

@lpinca @jasnell @Trott Cool I think your comments are all addressed. PTAL

@bnoordhuis As @refack mentioned, it would be great to get your input here. 😃

@lpinca
Copy link
Member

lpinca commented Jun 2, 2017

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

Needs a rebase and I have a nit but it should not block this from landing. Otherwise LGTM

@@ -130,6 +130,9 @@ Agent.prototype.getName = function getName(options) {
if (options.family === 4 || options.family === 6)
name += ':' + options.family;

if (options.socketPath)
name += ':' + options.socketPath;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: the socketPath should not be set when a family, a port or a localAddress is set. Therefore it would be nicer branch wise to write

if (options.socketPath) {
 name += `:::${options.socketPath}`;
} else {
  if (options.port)
    // ...
  if (options.localAddress)
    // ...
  // ...
}

I think it is similar with servername down below.

@lpinca lpinca requested a review from bnoordhuis August 27, 2017 13:14
@bengl
Copy link
Member Author

bengl commented Aug 29, 2017

Hey folks! Sorry, I'm going to close this one for now. I'd tried rebasing it, and when I do, it exposes issues with tls that I'm resolving over here. Once that lands, I think it will be safe to re-open this one (which I'll add a tls test to).

@bengl bengl closed this Aug 29, 2017
@BridgeAR
Copy link
Member

You could just add the blocked label and reopen now?

@bengl bengl added the blocked PRs that are blocked by other issues or PRs. label Aug 29, 2017
@bengl
Copy link
Member Author

bengl commented Aug 29, 2017

@BridgeAR Oh! That hadn't occurred to me. Thanks for pointing out that I could do that.

@bengl bengl reopened this Aug 29, 2017
@BridgeAR
Copy link
Member

As the other PR should land today, this should be rebased.

Makes `Connection: keep-alive` behave correctly when making client
connections to UNIX domain sockets.

Prior to this, connections would never be re-used, but the keep-alive
would cause the connections to stick around until they time out. This
would lead to an eventual EMFILE error due to all the connections
staying open. This was due to http.Agent not properly supporting UNIX
domain sockets.
@bengl
Copy link
Member Author

bengl commented Sep 23, 2017

@BridgeAR BridgeAR removed the blocked PRs that are blocked by other issues or PRs. label Sep 23, 2017
@bengl
Copy link
Member Author

bengl commented Sep 23, 2017

Just a quick call-out to @nodejs/http and @bnoordhuis for review. I think unless there's any objections, I'll probably land this early next week.

bengl added a commit that referenced this pull request Sep 28, 2017
Makes `Connection: keep-alive` behave correctly when making client
connections to UNIX domain sockets.

Prior to this, connections would never be re-used, but the keep-alive
would cause the connections to stick around until they time out. This
would lead to an eventual EMFILE error due to all the connections
staying open. This was due to http.Agent not properly supporting UNIX
domain sockets.

PR-URL: #13214
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR
Copy link
Member

Ping @bengl

@bengl
Copy link
Member Author

bengl commented Sep 28, 2017

Landed as 2043944.

@bengl bengl closed this Sep 28, 2017
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Sep 28, 2017
Makes `Connection: keep-alive` behave correctly when making client
connections to UNIX domain sockets.

Prior to this, connections would never be re-used, but the keep-alive
would cause the connections to stick around until they time out. This
would lead to an eventual EMFILE error due to all the connections
staying open. This was due to http.Agent not properly supporting UNIX
domain sockets.

PR-URL: nodejs#13214
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
lpinca added a commit to lpinca/node that referenced this pull request Sep 28, 2017
`Connection: keep-alive` is now properly supported when making client
connections to UNIX domain sockets so `request.abort()` should not
blindly destroy the underlying socket.

Refs: nodejs#13214 (comment)
MylesBorins pushed a commit that referenced this pull request Sep 29, 2017
Makes `Connection: keep-alive` behave correctly when making client
connections to UNIX domain sockets.

Prior to this, connections would never be re-used, but the keep-alive
would cause the connections to stick around until they time out. This
would lead to an eventual EMFILE error due to all the connections
staying open. This was due to http.Agent not properly supporting UNIX
domain sockets.

PR-URL: #13214
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 30, 2017
Makes `Connection: keep-alive` behave correctly when making client
connections to UNIX domain sockets.

Prior to this, connections would never be re-used, but the keep-alive
would cause the connections to stick around until they time out. This
would lead to an eventual EMFILE error due to all the connections
staying open. This was due to http.Agent not properly supporting UNIX
domain sockets.

PR-URL: nodejs/node#13214
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
Makes `Connection: keep-alive` behave correctly when making client
connections to UNIX domain sockets.

Prior to this, connections would never be re-used, but the keep-alive
would cause the connections to stick around until they time out. This
would lead to an eventual EMFILE error due to all the connections
staying open. This was due to http.Agent not properly supporting UNIX
domain sockets.

PR-URL: #13214
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Oct 3, 2017
MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
Makes `Connection: keep-alive` behave correctly when making client
connections to UNIX domain sockets.

Prior to this, connections would never be re-used, but the keep-alive
would cause the connections to stick around until they time out. This
would lead to an eventual EMFILE error due to all the connections
staying open. This was due to http.Agent not properly supporting UNIX
domain sockets.

PR-URL: #13214
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins
Copy link
Contributor

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

@bengl
Copy link
Member Author

bengl commented Oct 17, 2017

@MylesBorins this depends on #14564, which is marked dont-land-on-v6.x so I'll add the label here as well.

apapirovski pushed a commit that referenced this pull request Oct 23, 2017
`Connection: keep-alive` is now properly supported when making client
connections to UNIX domain sockets so `request.abort()` should not
blindly destroy the underlying socket.

PR-URL: #15650
Refs: #13214 (comment)
Reviewed-By: Anatoli Papirovski <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 23, 2017
`Connection: keep-alive` is now properly supported when making client
connections to UNIX domain sockets so `request.abort()` should not
blindly destroy the underlying socket.

PR-URL: #15650
Refs: #13214 (comment)
Reviewed-By: Anatoli Papirovski <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
`Connection: keep-alive` is now properly supported when making client
connections to UNIX domain sockets so `request.abort()` should not
blindly destroy the underlying socket.

PR-URL: nodejs/node#15650
Refs: nodejs/node#13214 (comment)
Reviewed-By: Anatoli Papirovski <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
`Connection: keep-alive` is now properly supported when making client
connections to UNIX domain sockets so `request.abort()` should not
blindly destroy the underlying socket.

PR-URL: nodejs/node#15650
Refs: nodejs/node#13214 (comment)
Reviewed-By: Anatoli Papirovski <[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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants