From d93aa85c07d46f758308d099d98eb39b966377dc Mon Sep 17 00:00:00 2001 From: Bryan English Date: Thu, 25 May 2017 00:32:04 -0700 Subject: [PATCH 1/2] 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. --- lib/_http_agent.js | 7 ++++ lib/_http_client.js | 20 ++--------- .../test-http-unix-socket-keep-alive.js | 34 +++++++++++++++++++ 3 files changed, 43 insertions(+), 18 deletions(-) create mode 100644 test/parallel/test-http-unix-socket-keep-alive.js diff --git a/lib/_http_agent.js b/lib/_http_agent.js index ddd36c158ec3..e71f0cb441f8 100644 --- a/lib/_http_agent.js +++ b/lib/_http_agent.js @@ -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; + return name; }; @@ -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; @@ -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; diff --git a/lib/_http_client.js b/lib/_http_client.js index a0e4fb2507c2..36e4210019fe 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -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; - 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 @@ -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); if (newSocket && !called) { called = true; this.onSocket(newSocket); diff --git a/test/parallel/test-http-unix-socket-keep-alive.js b/test/parallel/test-http-unix-socket-keep-alive.js new file mode 100644 index 000000000000..33c65a88f199 --- /dev/null +++ b/test/parallel/test-http-unix-socket-keep-alive.js @@ -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, () => + asyncLoop(makeKeepAliveRequest, 10, () => + server.getConnections(common.mustCall((err, conns) => { + assert.ifError(err); + assert.strictEqual(1, conns); + server.close(); + })) + ) +); + +function asyncLoop(fn, times, cb) { + fn(function handler() { + if (--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)); +} From 2da5453d42d183259b79c75dedbc18aedd2f8d41 Mon Sep 17 00:00:00 2001 From: Bryan English Date: Thu, 1 Jun 2017 23:36:11 -0700 Subject: [PATCH 2/2] [squash] the nits --- lib/_http_client.js | 2 +- test/parallel/test-http-agent-getname.js | 12 +++++++++++- .../test-http-unix-socket-keep-alive.js | 17 ++++++++++------- 3 files changed, 22 insertions(+), 9 deletions(-) diff --git a/lib/_http_client.js b/lib/_http_client.js index 36e4210019fe..6eb7d34fc57d 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -258,7 +258,7 @@ function ClientRequest(options, cb) { this._last = true; this.shouldKeepAlive = false; if (typeof options.createConnection === 'function') { - var newSocket = options.createConnection(options, oncreate); + const newSocket = options.createConnection(options, oncreate); if (newSocket && !called) { called = true; this.onSocket(newSocket); diff --git a/test/parallel/test-http-agent-getname.js b/test/parallel/test-http-agent-getname.js index f84996ec013e..4b4e9ac26b44 100644 --- a/test/parallel/test-http-agent-getname.js +++ b/test/parallel/test-http-agent-getname.js @@ -1,8 +1,9 @@ 'use strict'; -require('../common'); +const common = require('../common'); const assert = require('assert'); const http = require('http'); +const path = require('path'); const agent = new http.Agent(); @@ -31,6 +32,15 @@ assert.strictEqual( '0.0.0.0:80:192.168.1.1' ); +// unix socket +const socketPath = path.join(common.tmpDir, 'foo', 'bar'); +assert.strictEqual( + agent.getName({ + socketPath + }), + `localhost:::${socketPath}` +); + for (const family of [0, null, undefined, 'bogus']) assert.strictEqual(agent.getName({ family }), 'localhost::'); diff --git a/test/parallel/test-http-unix-socket-keep-alive.js b/test/parallel/test-http-unix-socket-keep-alive.js index 33c65a88f199..668c440325e0 100644 --- a/test/parallel/test-http-unix-socket-keep-alive.js +++ b/test/parallel/test-http-unix-socket-keep-alive.js @@ -7,15 +7,15 @@ const server = http.createServer((req, res) => res.end()); common.refreshTmpDir(); -server.listen(common.PIPE, () => - asyncLoop(makeKeepAliveRequest, 10, () => +server.listen(common.PIPE, common.mustCall(() => + asyncLoop(makeKeepAliveRequest, 10, common.mustCall(() => server.getConnections(common.mustCall((err, conns) => { assert.ifError(err); - assert.strictEqual(1, conns); + assert.strictEqual(conns, 1); server.close(); })) - ) -); + )) +)); function asyncLoop(fn, times, cb) { fn(function handler() { @@ -29,6 +29,9 @@ function asyncLoop(fn, times, cb) { function makeKeepAliveRequest(cb) { http.get({ socketPath: common.PIPE, - headers: {connection: 'keep-alive'} - }, (res) => res.on('data', () => {}).on('error', assert.fail).on('end', cb)); + headers: { connection: 'keep-alive' } + }, (res) => res.on('data', common.mustNotCall()) + .on('error', assert.fail) + .on('end', cb) + ); }