Skip to content

http: client keep-alive for UNIX domain sockets #13214

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.

Sign up for GitHub

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

Closed
bengl wants to merge 2 commits into from
Closed

http: client keep-alive for UNIX domain sockets #13214

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d93aa85
http: client keep-alive for UNIX domain sockets
bengl May 25, 2017
2da5453
[squash] the nits
bengl Jun 2, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
http: client keep-alive for UNIX domain sockets 
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.
  • Loading branch information
@bengl
bengl committed Sep 23, 2017
commit d93aa85c07d46f758308d099d98eb39b966377dc
7 changes: 7 additions & 0 deletions lib/_http_agent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,9 @@ Agent.prototype.getName = function getName(options) {
if (options.family === 4 || options.family === 6)
name += ':' + options.family;

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

@lpinca lpinca May 26, 2017

Copy link
Copy Markdown
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.

@BridgeAR BridgeAR Aug 27, 2017

Copy link
Copy Markdown
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.


return name;
};

Expand All @@ -147,6 +150,8 @@ Agent.prototype.addRequest = function addRequest(req, options, port/*legacy*/,

options = util._extend({}, options);
util._extend(options, this.options);
if (options.socketPath)
options.path = options.socketPath;

if (!options.servername) {
options.servername = options.host;
Expand Down Expand Up @@ -199,6 +204,8 @@ Agent.prototype.createSocket = function createSocket(req, options, cb) {
var self = this;
options = util._extend({}, options);
util._extend(options, self.options);
if (options.socketPath)
options.path = options.socketPath;

if (!options.servername) {
options.servername = options.host;
Expand Down
20 changes: 2 additions & 18 deletions lib/_http_client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,23 +240,7 @@ function ClientRequest(options, cb) {
this._deferToConnect(null, null, () => this._flush());
};

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

@refack refack May 25, 2017

Copy link
Copy Markdown
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?

var optionsPath = {
path: this.socketPath,
timeout: this.timeout,
rejectUnauthorized: !!options.rejectUnauthorized
};
newSocket = this.agent.createConnection(optionsPath, oncreate);
if (newSocket && !called) {
called = true;
this.onSocket(newSocket);
} else {
return;
}
} else if (this.agent) {
if (this.agent) {
// If there is an agent we should default to Connection:keep-alive,
// but only if the Agent will actually reuse the connection!
// If it's not a keepAlive agent, and the maxSockets==Infinity, then
Expand All @@ -274,7 +258,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);

@refack refack May 25, 2017

Copy link
Copy Markdown
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

@bengl bengl May 25, 2017

Copy link
Copy Markdown
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.

if (newSocket && !called) {
called = true;
this.onSocket(newSocket);
Expand Down
34 changes: 34 additions & 0 deletions test/parallel/test-http-unix-socket-keep-alive.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const http = require('http');

const server = http.createServer((req, res) => res.end());

common.refreshTmpDir();

server.listen(common.PIPE, () =>

@jasnell jasnell Jun 2, 2017

Copy link
Copy Markdown
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?

@bengl bengl Jun 2, 2017

Copy link
Copy Markdown
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()?)

@Trott Trott Jun 2, 2017

Copy link
Copy Markdown
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).

@bengl bengl Jun 2, 2017

Copy link
Copy Markdown
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.

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

@lpinca lpinca May 26, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

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?

server.close();
}))
)
);

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

@lpinca lpinca May 26, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

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--.

fn(handler);
} else {
cb();
}
});
}
function makeKeepAliveRequest(cb) {
http.get({
socketPath: common.PIPE,
headers: {connection: 'keep-alive'}
}, (res) => res.on('data', () => {}).on('error', assert.fail).on('end', cb));

@Trott Trott May 25, 2017

Copy link
Copy Markdown
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.

}