Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

http: Proper KeepAlive behavior

Instead of destroying sockets when there are no pending requests, put
them in a freeSockets list, and unref() them so that they do not keep
the event loop open.

Also, set the default max sockets to Infinity, to prevent the awful
surprising deadlocks that happen when more connections are made.
  • Loading branch information...
commit 9fc9b87472806147b83c24d85b303c4f75d3021c 1 parent 6176e49
@isaacs isaacs authored
View
63 lib/_http_agent.js
@@ -36,13 +36,20 @@ var EventEmitter = require('events').EventEmitter;
// concerned with managing a connection pool.
function Agent(options) {
+ if (!(this instanceof Agent))
+ return new Agent(options);
+
EventEmitter.call(this);
var self = this;
self.options = options || {};
self.requests = {};
self.sockets = {};
+ self.freeSockets = {};
+ self.keepAliveMsecs = self.options.keepAliveMsecs || 1000;
+ 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) {
@@ -57,11 +64,31 @@ function Agent(options) {
delete self.requests[name];
}
} else {
- // If there are no pending requests just destroy the
- // socket and it will get removed from the pool. This
- // gets us out of timeout issues and allows us to
- // default to Connection:keep-alive.
- socket.destroy();
+ // If there are no pending requests, then put it in
+ // the freeSockets pool, but only if we're allowed to do so.
+ var req = socket._httpMessage;
+ if (req &&
+ req.shouldKeepAlive &&
+ !socket.destroyed &&
+ self.options.keepAlive) {
@mmoskal
mmoskal added a note

Shouldn't this be self.keepAlive? It's copied in line 50, and it would allow setting on existing instances.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ var freeSockets = self.freeSockets[name];
+ var count = freeSockets ? freeSockets.length : 0;
+ if (self.sockets[name])
+ count += self.sockets[name].length;
+
+ if (count > self.maxSockets) {
+ socket.destroy();
+ } else {
+ freeSockets = freeSockets || [];
+ self.freeSockets[name] = freeSockets;
+ socket.setKeepAlive(true, self.keepAliveMsecs);
+ socket.unref();
+ socket._httpMessage = null;
+ freeSockets.push(socket);
+ }
+ } else {
+ socket.destroy();
+ }
}
});
self.createConnection = net.createConnection;
@@ -70,7 +97,7 @@ function Agent(options) {
util.inherits(Agent, EventEmitter);
exports.Agent = Agent;
-Agent.defaultMaxSockets = 5;
+Agent.defaultMaxSockets = Infinity;
@erewh0n
erewh0n added a note

Hi there! Regarding this comment: "set the default max sockets to Infinity, to prevent the awful surprising deadlocks that happen when more connections are made."

I'm wondering if either @isaacs or @domenic could comment on the deadlock problem and whether the above maximum sockets change is really just a mitigation of some underlying core issue?

I ask because we've seen a persistent problem in production where one of our services using the HTTP client from node 0.10 stops issuing requests to our upstream servers. Investigation of this issue led us to the max sockets configuration. Is increasing the value of defaultMaxSockets to Infinity just masking an underlying problem related deadlock behaviour?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Agent.prototype.defaultPort = 80;
Agent.prototype.addRequest = function(req, host, port, localAddress) {
@@ -81,7 +108,18 @@ Agent.prototype.addRequest = function(req, host, port, localAddress) {
if (!this.sockets[name]) {
this.sockets[name] = [];
}
- if (this.sockets[name].length < this.maxSockets) {
+
+ if (this.freeSockets[name] && this.freeSockets[name].length) {
+ // we have a free socket, so use that.
+ var socket = this.freeSockets[name].shift();
+
+ // don't leak
+ if (!this.freeSockets[name].length)
+ delete this.freeSockets[name];
+
+ socket.ref();
+ req.onSocket(socket);
+ } else if (this.sockets[name].length < this.maxSockets) {
// If we are under maxSockets create a new one.
req.onSocket(this.createSocket(name, host, port, localAddress, req));
} else {
@@ -158,5 +196,16 @@ Agent.prototype.removeSocket = function(s, name, host, port, localAddress) {
}
};
+Agent.prototype.destroy = function() {
+ var sets = [this.freeSockets, this.sockets];
+ sets.forEach(function(set) {
+ Object.keys(set).forEach(function(name) {
+ set[name].forEach(function(socket) {
+ socket.destroy();
+ });
+ });
+ });
+};
+
var globalAgent = new Agent();
exports.globalAgent = globalAgent;
View
2  test/simple/test-http-client-abort.js
@@ -50,7 +50,7 @@ var server = http.Server(function(req, res) {
});
var responses = 0;
-var N = http.Agent.defaultMaxSockets - 1;
+var N = 16;
var requests = [];
server.listen(common.PORT, function() {
View
92 test/simple/test-http-keepalive-client.js
@@ -0,0 +1,92 @@
+// Copyright Joyent, Inc. and other Node contributors.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to permit
+// persons to whom the Software is furnished to do so, subject to the
+// following conditions:
+//
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+var common = require('../common');
+var assert = require('assert');
+
+var http = require('http');
+
+
+var serverSocket = null;
+var server = http.createServer(function(req, res) {
+ // They should all come in on the same server socket.
+ if (serverSocket) {
+ assert.equal(req.socket, serverSocket);
+ } else {
+ serverSocket = req.socket;
+ }
+
+ res.end(req.url);
+});
+server.listen(common.PORT);
+
+var agent = http.Agent({ keepAlive: true });
+
+
+var clientSocket = null;
+var expectRequests = 10;
+var actualRequests = 0;
+
+
+makeRequest(expectRequests);
+function makeRequest(n) {
+ if (n === 0) {
+ server.close();
+ agent.destroy();
+ return;
+ }
+
+ var req = http.request({
+ port: common.PORT,
+ agent: agent,
+ path: '/' + n
+ });
+
+ req.end();
+
+ req.on('socket', function(sock) {
+ if (clientSocket) {
+ assert.equal(sock, clientSocket);
+ } else {
+ clientSocket = sock;
+ }
+ });
+
+ req.on('response', function(res) {
+ var data = '';
+ res.setEncoding('utf8');
+ res.on('data', function(c) {
+ data += c;
+ });
+ res.on('end', function() {
+ assert.equal(data, '/' + n);
+ setTimeout(function() {
+ actualRequests++;
+ makeRequest(n - 1);
+ }, 1);
+ });
+ });
+}
+
+process.on('exit', function() {
+ assert.equal(actualRequests, expectRequests)
+ console.log('ok');
+});

2 comments on commit 9fc9b87

@domenic

\o/

@OrangeDog

What happens when the socket selected from the pool has been hung up since the last request?

Please sign in to comment.
Something went wrong with that request. Please try again.