From 49519f121787d51394f00c871f854794e409bdda Mon Sep 17 00:00:00 2001 From: isaacs Date: Wed, 22 May 2013 18:44:24 -0700 Subject: [PATCH] http: Reuse more http/https Agent code --- lib/_http_agent.js | 100 +++++++++++++++------ lib/_http_client.js | 21 ++--- lib/https.js | 69 +++++++------- test/simple/test-http-agent-destroyed-socket.js | 2 +- test/simple/test-http-client-agent.js | 2 +- .../simple/test-http-keep-alive-close-on-header.js | 7 +- test/simple/test-http-keep-alive.js | 2 +- test/simple/test-regress-GH-1531.js | 7 +- test/simple/test-regress-GH-877.js | 4 +- 9 files changed, 128 insertions(+), 86 deletions(-) diff --git a/lib/_http_agent.js b/lib/_http_agent.js index 20fa1b6..07e8f4c 100644 --- a/lib/_http_agent.js +++ b/lib/_http_agent.js @@ -24,6 +24,7 @@ var url = require('url'); var util = require('util'); var EventEmitter = require('events').EventEmitter; var ClientRequest = require('_http_client').ClientRequest; +var debug = util.debuglog('http'); // New Agent code. @@ -44,7 +45,12 @@ function Agent(options) { EventEmitter.call(this); var self = this; + + self.defaultPort = 80; + self.protocol = 'http:'; + self.options = util._extend({}, options); + // don't confuse net and make it think that we're connecting to a pipe self.options.path = null; self.requests = {}; @@ -54,11 +60,9 @@ function Agent(options) { self.keepAlive = self.options.keepAlive || false; self.maxSockets = self.options.maxSockets || Agent.defaultMaxSockets; - self.on('free', function(socket, host, port, localAddress) { - var name = host + ':' + port; - if (localAddress) { - name += ':' + localAddress; - } + self.on('free', function(socket, options) { + var name = self.getName(options); + debug('agent.on(free)', name); if (!socket.destroyed && self.requests[name] && self.requests[name].length) { @@ -103,18 +107,38 @@ exports.Agent = Agent; Agent.defaultMaxSockets = Infinity; Agent.prototype.createConnection = net.createConnection; -Agent.prototype.defaultPort = 80; -Agent.prototype.protocol = 'http:'; -Agent.prototype.addRequest = function(req, host, port, localAddress) { - var name = host + ':' + port; - if (localAddress) { - name += ':' + localAddress; - } + +// Get the key for a given set of request options +Agent.prototype.getName = function(options) { + var name = ''; + + if (options.host) + name += options.host; + else + name += 'localhost'; + + name += ':'; + if (options.port) + name += options.port; + name += ':'; + if (options.localAddress) + name += options.localAddress; + name += ':'; + return name; +}; + +Agent.prototype.addRequest = function(req, options) { + var host = options.host; + var port = options.port; + var localAddress = options.localAddress; + + var name = this.getName(options); if (!this.sockets[name]) { this.sockets[name] = []; } if (this.freeSockets[name] && this.freeSockets[name].length) { + debug('have free socket'); // we have a free socket, so use that. var socket = this.freeSockets[name].shift(); @@ -125,9 +149,11 @@ Agent.prototype.addRequest = function(req, host, port, localAddress) { socket.ref(); req.onSocket(socket); } else if (this.sockets[name].length < this.maxSockets) { + debug('call onSocket'); // If we are under maxSockets create a new one. - req.onSocket(this.createSocket(name, host, port, localAddress, req)); + req.onSocket(this.createSocket(req, options)); } else { + debug('wait for socket'); // We are over limit so we'll add it to the queue. if (!this.requests[name]) { this.requests[name] = []; @@ -136,14 +162,12 @@ Agent.prototype.addRequest = function(req, host, port, localAddress) { } }; -Agent.prototype.createSocket = function(name, host, port, localAddress, req) { +Agent.prototype.createSocket = function(req, options) { var self = this; - var options = util._extend({}, self.options); - options.port = port; - options.host = host; - options.localAddress = localAddress; + options = util._extend({}, options); + options = util._extend(options, self.options); - options.servername = host; + options.servername = options.host; if (req) { var hostHeader = req.getHeader('host'); if (hostHeader) { @@ -151,30 +175,36 @@ Agent.prototype.createSocket = function(name, host, port, localAddress, req) { } } + var name = self.getName(options); + + debug('createConnection', name, options); var s = self.createConnection(options); if (!self.sockets[name]) { self.sockets[name] = []; } this.sockets[name].push(s); + debug('sockets', name, this.sockets[name].length); function onFree() { - self.emit('free', s, host, port, localAddress); + self.emit('free', s, options); } s.on('free', onFree); function onClose(err) { + debug('CLIENT socket onClose'); // This is the only place where sockets get removed from the Agent. // If you want to remove a socket from the pool, just close it. // All socket errors end in a close event anyway. - self.removeSocket(s, name, host, port, localAddress); + self.removeSocket(s, options); } s.on('close', onClose); function onRemove() { // We need this function for cases like HTTP 'upgrade' - // (defined by WebSockets) where we need to remove a socket from the pool - // because it'll be locked up indefinitely - self.removeSocket(s, name, host, port, localAddress); + // (defined by WebSockets) where we need to remove a socket from the + // pool because it'll be locked up indefinitely + debug('CLIENT socket onRemove'); + self.removeSocket(s, options); s.removeListener('close', onClose); s.removeListener('free', onFree); s.removeListener('agentRemove', onRemove); @@ -183,7 +213,9 @@ Agent.prototype.createSocket = function(name, host, port, localAddress, req) { return s; }; -Agent.prototype.removeSocket = function(s, name, host, port, localAddress) { +Agent.prototype.removeSocket = function(s, options) { + var name = this.getName(options); + debug('removeSocket', name); if (this.sockets[name]) { var index = this.sockets[name].indexOf(s); if (index !== -1) { @@ -195,9 +227,10 @@ Agent.prototype.removeSocket = function(s, name, host, port, localAddress) { } } if (this.requests[name] && this.requests[name].length) { + debug('removeSocket, have a request, make a socket'); var req = this.requests[name][0]; - // If we have pending requests and a socket gets closed a new one - this.createSocket(name, host, port, localAddress, req).emit('free'); + // If we have pending requests and a socket gets closed make a new one + this.createSocket(req, options).emit('free'); } }; @@ -216,6 +249,10 @@ Agent.prototype.request = function(options, cb) { if (typeof options === 'string') { options = url.parse(options); } + // don't try to do dns lookups of foo.com:8080, just foo.com + if (options.hostname) { + options.host = options.hostname; + } if (options && options.path && / /.test(options.path)) { // The actual regex is more like /[^A-Za-z0-9\-._~!$&'()*+,;=/:@]/ @@ -229,11 +266,16 @@ Agent.prototype.request = function(options, cb) { throw new Error('Protocol:' + options.protocol + ' not supported.'); } - options = util._extend({ agent: this, keepAlive: false }, options); + options = util._extend({ + agent: this, + keepAlive: this.keepAlive + }, options); + // if it's false, then make a new one, just like this one. if (options.agent === false) - options.agent = new Agent(options); + options.agent = new this.constructor(options); + debug('agent.request', options); return new ClientRequest(options, cb); }; diff --git a/lib/_http_client.js b/lib/_http_client.js index 0f55148..0bc78ab 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -99,33 +99,26 @@ function ClientRequest(options, cb) { self._storeHeader(self.method + ' ' + self.path + ' HTTP/1.1\r\n', self._renderHeaders()); } + if (self.socketPath) { self._last = true; self.shouldKeepAlive = false; - if (options.createConnection) { - self.onSocket(options.createConnection(self.socketPath)); - } else { - self.onSocket(net.createConnection(self.socketPath)); - } + var conn = self.agent.createConnection({ path: self.socketPath }); + self.onSocket(conn); } else if (self.agent) { // If there is an agent we should default to Connection:keep-alive. self._last = false; self.shouldKeepAlive = true; - self.agent.addRequest(self, host, port, options.localAddress); + self.agent.addRequest(self, options); } else { // No agent, default to Connection:close. self._last = true; self.shouldKeepAlive = false; if (options.createConnection) { - options.port = port; - options.host = host; var conn = options.createConnection(options); } else { - var conn = net.createConnection({ - port: port, - host: host, - localAddress: options.localAddress - }); + debug('CLIENT use net.createConnection', options); + var conn = net.createConnection(options); } self.onSocket(conn); } @@ -134,8 +127,8 @@ function ClientRequest(options, cb) { self._flush(); self = null; }); - } + util.inherits(ClientRequest, OutgoingMessage); exports.ClientRequest = ClientRequest; diff --git a/lib/https.js b/lib/https.js index 18789ad..afbdef1 100644 --- a/lib/https.js +++ b/lib/https.js @@ -24,6 +24,7 @@ var http = require('http'); var util = require('util'); var url = require('url'); var inherits = require('util').inherits; +var debug = util.debuglog('https'); function Server(opts, requestListener) { if (!(this instanceof Server)) return new Server(opts, requestListener); @@ -77,56 +78,58 @@ function createConnection(port, host, options) { options.host = host; } + debug('createConnection', options); return tls.connect(options); } function Agent(options) { http.Agent.call(this, options); + this.defaultPort = 443; + this.protocol = 'https:'; } inherits(Agent, http.Agent); -Agent.prototype.defaultPort = 443; -Agent.prototype.protocol = 'https:'; Agent.prototype.createConnection = createConnection; +Agent.prototype.getName = function(options) { + var name = http.Agent.prototype.getName.call(this, options); + + name += ':'; + if (options.ca) + name += options.ca; + + name += ':'; + if (options.cert) + name += options.cert; + + name += ':'; + if (options.ciphers) + name += options.ciphers; + + name += ':'; + if (options.key) + name += options.key; + + name += ':'; + if (options.pfx) + name += options.pfx; + + name += ':'; + if (options.rejectUnauthorized !== undefined) + name += options.rejectUnauthorized; + + return name; +}; + var globalAgent = new Agent(); exports.globalAgent = globalAgent; exports.Agent = Agent; exports.request = function(options, cb) { - if (typeof options === 'string') { - options = url.parse(options); - } - - if (options.protocol && options.protocol !== 'https:') { - throw new Error('Protocol:' + options.protocol + ' not supported.'); - } - - options = util._extend({ - createConnection: createConnection, - defaultPort: 443 - }, options); - - if (typeof options.agent === 'undefined') { - if (typeof options.ca === 'undefined' && - typeof options.cert === 'undefined' && - typeof options.ciphers === 'undefined' && - typeof options.key === 'undefined' && - typeof options.passphrase === 'undefined' && - typeof options.pfx === 'undefined' && - typeof options.rejectUnauthorized === 'undefined') { - options.agent = globalAgent; - } else { - options.agent = new Agent(options); - } - } - - return new http.ClientRequest(options, cb); + return globalAgent.request(options, cb); }; exports.get = function(options, cb) { - var req = exports.request(options, cb); - req.end(); - return req; + return globalAgent.get(options, cb); }; diff --git a/test/simple/test-http-agent-destroyed-socket.js b/test/simple/test-http-agent-destroyed-socket.js index 960b3a3..be90bc6 100644 --- a/test/simple/test-http-agent-destroyed-socket.js +++ b/test/simple/test-http-agent-destroyed-socket.js @@ -43,7 +43,7 @@ var requestOptions = { var request1 = http.get(requestOptions, function(response) { // assert request2 is queued in the agent - var key = 'localhost:' + common.PORT; + var key = agent.getName(requestOptions); assert(agent.requests[key].length === 1); console.log('got response1'); request1.socket.on('close', function() { diff --git a/test/simple/test-http-client-agent.js b/test/simple/test-http-client-agent.js index aedf64b..ad5c149 100644 --- a/test/simple/test-http-client-agent.js +++ b/test/simple/test-http-client-agent.js @@ -23,7 +23,7 @@ var common = require('../common'); var assert = require('assert'); var http = require('http'); -var name = 'localhost:' + common.PORT; +var name = http.globalAgent.getName({ port: common.PORT }); var max = 3; var count = 0; diff --git a/test/simple/test-http-keep-alive-close-on-header.js b/test/simple/test-http-keep-alive-close-on-header.js index 53c73ae..4318bd9 100644 --- a/test/simple/test-http-keep-alive-close-on-header.js +++ b/test/simple/test-http-keep-alive-close-on-header.js @@ -38,6 +38,7 @@ var connectCount = 0; server.listen(common.PORT, function() { var agent = new http.Agent({ maxSockets: 1 }); + var name = agent.getName({ port: common.PORT }); var request = http.request({ method: 'GET', path: '/', @@ -45,7 +46,7 @@ server.listen(common.PORT, function() { port: common.PORT, agent: agent }, function(res) { - assert.equal(1, agent.sockets['localhost:' + common.PORT].length); + assert.equal(1, agent.sockets[name].length); res.resume(); }); request.on('socket', function(s) { @@ -62,7 +63,7 @@ server.listen(common.PORT, function() { port: common.PORT, agent: agent }, function(res) { - assert.equal(1, agent.sockets['localhost:' + common.PORT].length); + assert.equal(1, agent.sockets[name].length); res.resume(); }); request.on('socket', function(s) { @@ -79,7 +80,7 @@ server.listen(common.PORT, function() { agent: agent }, function(response) { response.on('end', function() { - assert.equal(1, agent.sockets['localhost:' + common.PORT].length); + assert.equal(1, agent.sockets[name].length); server.close(); }); response.resume(); diff --git a/test/simple/test-http-keep-alive.js b/test/simple/test-http-keep-alive.js index 4e8a6e8..75049c9 100644 --- a/test/simple/test-http-keep-alive.js +++ b/test/simple/test-http-keep-alive.js @@ -32,9 +32,9 @@ var server = http.createServer(function(req, res) { }); var connectCount = 0; -var name = 'localhost:' + common.PORT; var agent = new http.Agent({maxSockets: 1}); var headers = {'connection': 'keep-alive'}; +var name = agent.getName({ port: common.PORT }); server.listen(common.PORT, function() { http.get({ diff --git a/test/simple/test-regress-GH-1531.js b/test/simple/test-regress-GH-1531.js index 60cd949..53bc2a3 100644 --- a/test/simple/test-regress-GH-1531.js +++ b/test/simple/test-regress-GH-1531.js @@ -42,22 +42,25 @@ var server = https.createServer(options, function(req, res) { }); server.listen(common.PORT, function() { + console.error('listening'); https.get({ agent: false, path: '/', port: common.PORT, rejectUnauthorized: false }, function(res) { - console.error(res.statusCode); + console.error(res.statusCode, res.headers); gotCallback = true; + res.resume(); server.close(); }).on('error', function(e) { - console.error(e.message); + console.error(e.stack); process.exit(1); }); }); process.on('exit', function() { assert.ok(gotCallback); + console.log('ok'); }); diff --git a/test/simple/test-regress-GH-877.js b/test/simple/test-regress-GH-877.js index 1cdca9f..30e1f80 100644 --- a/test/simple/test-regress-GH-877.js +++ b/test/simple/test-regress-GH-877.js @@ -35,7 +35,7 @@ var server = http.createServer(function(req, res) { res.end('Hello World\n'); }); -var addrString = '127.0.0.1:' + common.PORT; +var addrString = agent.getName({ host: '127.0.0.1', port: common.PORT }); server.listen(common.PORT, '127.0.0.1', function() { for (var i = 0; i < N; i++) { @@ -55,7 +55,7 @@ server.listen(common.PORT, '127.0.0.1', function() { console.log('Socket: ' + agent.sockets[addrString].length + '/' + agent.maxSockets + ' queued: ' + (agent.requests[addrString] ? - agent.requests['127.0.0.1:' + common.PORT].length : 0)); + agent.requests[addrString].length : 0)); var agentRequests = agent.requests[addrString] ? agent.requests[addrString].length : 0; -- 2.7.4