Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Commit

Permalink
http: fix resource leak
Browse files Browse the repository at this point in the history
Fixes #2069
  • Loading branch information
koichik committed Dec 26, 2011
1 parent 0de6ec5 commit 7aa5924
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 21 deletions.
12 changes: 8 additions & 4 deletions lib/http.js
Original file line number Diff line number Diff line change
Expand Up @@ -896,6 +896,10 @@ function Agent(options) {
var name = host + ':' + port;
if (self.requests[name] && self.requests[name].length) {
self.requests[name].shift().onSocket(socket);
if (self.requests[name].length === 0) {
// don't leak
delete this.requests[name];

This comment has been minimized.

Copy link
@windyrobin

windyrobin Jan 5, 2012

should use 'self.requests[name]' instead of 'this.requests[name]' ?

This comment has been minimized.

Copy link
@koichik

koichik Jan 5, 2012

Author

Thanks, good catch. Fortunately, this and self are the same objects here, but self is clearly better.

This comment has been minimized.

Copy link
@fengmk2

fengmk2 Jan 6, 2012

support self.requests[name] too, use this would let me to think what is this point to.

This comment has been minimized.

Copy link
@koichik

koichik Jan 6, 2012

Author

Fixed in baebd30

}
} else {
// If there are no pending requests just destroy the
// socket and it will get removed from the pool. This
Expand Down Expand Up @@ -963,11 +967,11 @@ Agent.prototype.removeSocket = function(s, name, host, port) {
var index = this.sockets[name].indexOf(s);
if (index !== -1) {
this.sockets[name].splice(index, 1);
if (this.sockets[name].length === 0) {
// don't leak
delete this.sockets[name];
}
}
} else if (this.sockets[name] && this.sockets[name].length === 0) {
// don't leak
delete this.sockets[name];
delete this.requests[name];
}
if (this.requests[name] && this.requests[name].length) {
// If we have pending requests and a socket gets closed a new one
Expand Down
9 changes: 6 additions & 3 deletions test/simple/test-http-client-agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,12 @@ function request(i) {
var socket = req.socket;
socket.on('close', function() {
++count;
assert.equal(http.globalAgent.sockets[name].length, max - count);
assert.equal(http.globalAgent.sockets[name].indexOf(socket), -1);
if (count === max) {
if (count < max) {
assert.equal(http.globalAgent.sockets[name].length, max - count);
assert.equal(http.globalAgent.sockets[name].indexOf(socket), -1);
} else {
assert(!http.globalAgent.sockets.hasOwnProperty(name));
assert(!http.globalAgent.requests.hasOwnProperty(name));
server.close();
}
});
Expand Down
36 changes: 24 additions & 12 deletions test/simple/test-http-keep-alive.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,8 @@
var common = require('../common');
var assert = require('assert');
var http = require('http');
var util = require('util');

var body = 'hello world\n';
var headers = {'connection': 'keep-alive'};

var server = http.createServer(function(req, res) {
res.writeHead(200, {'Content-Length': body.length});
Expand All @@ -34,23 +32,37 @@ 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'};

server.listen(common.PORT, function() {
var agent = new http.Agent({maxSockets: 1});
var request = http.request({method: 'GET', path: '/', headers: headers, port: common.PORT, agent: agent}, function() {
assert.equal(1, agent.sockets['localhost:' + common.PORT].length);
http.get({
path: '/', headers: headers, port: common.PORT, agent: agent
}, function(response) {
assert.equal(agent.sockets[name].length, 1);
assert.equal(agent.requests[name].length, 2);
});
request.end();

request = http.request({method: 'GET', path: '/', headers: headers, port: common.PORT, agent: agent}, function() {
assert.equal(1, agent.sockets['localhost:' + common.PORT].length);
http.get({
path: '/', headers: headers, port: common.PORT, agent: agent
}, function(response) {
assert.equal(agent.sockets[name].length, 1);
assert.equal(agent.requests[name].length, 1);
});
request.end();
request = http.request({method: 'GET', path: '/', headers: headers, port: common.PORT, agent: agent}, function(response) {

http.get({
path: '/', headers: headers, port: common.PORT, agent: agent
}, function(response) {
response.on('end', function() {
assert.equal(1, agent.sockets['localhost:' + common.PORT].length);
assert.equal(agent.sockets[name].length, 1);
assert(!agent.requests.hasOwnProperty(name));
server.close();
});
});
request.end();
});

process.on('exit', function() {
assert(!agent.sockets.hasOwnProperty(name));
assert(!agent.requests.hasOwnProperty(name));
});
5 changes: 3 additions & 2 deletions test/simple/test-http-upgrade-agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ srv.listen(common.PORT, '127.0.0.1', function() {
'upgrade': 'websocket'
}
};
var name = options.host + ':' + options.port;

var req = http.request(options);
req.end();
Expand All @@ -73,11 +74,11 @@ srv.listen(common.PORT, '127.0.0.1', function() {
'connection': 'upgrade',
'upgrade': 'websocket' };
assert.deepEqual(expectedHeaders, res.headers);
assert.equal(http.globalAgent.sockets[options.host + ':' + options.port].length, 1);
assert.equal(http.globalAgent.sockets[name].length, 1);

process.nextTick(function() {
// Make sure this request got removed from the pool.
assert.equal(http.globalAgent.sockets[options.host + ':' + options.port].length, 0);
assert(!http.globalAgent.sockets.hasOwnProperty(name));
socket.end();
srv.close();

Expand Down

0 comments on commit 7aa5924

Please sign in to comment.