Skip to content

Commit c92f5b5

Browse files
fengmk2fengmk2
committed
fix socket does not timeout bug, it will hang on life
1 parent 13eb5b7 commit c92f5b5

File tree

6 files changed

+166
-15
lines changed

6 files changed

+166
-15
lines changed

‎README.md‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,9 @@ var Agent = require('agentkeepalive');
1818

1919
var keepaliveAgent = new Agent({
2020
maxSockets: 10,
21-
maxKeepAliveRequests: 0, // max requests per keepalive socket, default is 0, no limit.
22-
maxKeepAliveTime: 30000 // keepalive for 30 seconds
21+
maxFreeSockets: 10,
22+
keepAlive: true,
23+
keepAliveMsecs: 30000 // keepalive for 30 seconds
2324
});
2425

2526
var options = {

‎benchmark/proxy.js‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,9 @@ var maxSockets = parseInt(process.argv[2], 10) || 10;
1717
var SERVER = process.argv[3] || '127.0.0.1';
1818

1919
var agentKeepalive = new Agent({
20+
keepAlive: true,
2021
maxSockets: maxSockets,
22+
maxFreeSockets: maxSockets,
2123
maxKeepAliveTime: 30000,
2224
});
2325
var agentHttp = new http.Agent({
@@ -63,14 +65,14 @@ var rtNormals = {
6365
setInterval(function () {
6466
var name = SERVER + ':1984';
6567
console.log('----------------------------------------------------------------');
66-
console.log('[proxy.js:%d] keepalive, %d created, %d requestFinished, %d req/socket, %s requests, %s sockets, %s unusedSockets, %d timeout\n%j',
68+
console.log('[proxy.js:%d] keepalive, %d created, %d requestFinished, %d req/socket, %s requests, %s sockets, %s freeSockets, %d timeout\n%j',
6769
count,
6870
agentKeepalive.createSocketCount,
6971
agentKeepalive.requestFinishedCount,
7072
(agentKeepalive.requestFinishedCount / agentKeepalive.createSocketCount || 0).toFixed(2),
7173
agentKeepalive.requests[name] && agentKeepalive.requests[name].length || 0,
7274
agentKeepalive.sockets[name] && agentKeepalive.sockets[name].length || 0,
73-
agentKeepalive.unusedSockets[name] && agentKeepalive.unusedSockets[name].length || 0,
75+
agentKeepalive.freeSockets[name] && agentKeepalive.freeSockets[name].length || 0,
7476
agentKeepalive.timeoutSocketCount,
7577
rtKeepalives
7678
);
@@ -109,6 +111,7 @@ http.createServer(function (req, res) {
109111
method: method,
110112
agent: agent
111113
};
114+
req.on('data', function () {});
112115
req.on('end', function () {
113116
var timer = null;
114117
var start = Date.now();
@@ -160,4 +163,4 @@ http.createServer(function (req, res) {
160163

161164
}).listen(1985);
162165

163-
console.log('proxy start, listen on 1985');
166+
console.log('proxy start, listen on 1985');

‎benchmark/start.sh‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ sudo ulimit -n 100000
88
sysctl -n machdep.cpu.brand_string
99

1010
SERVER=127.0.0.1
11-
NUM=1000
11+
NUM=500
1212
CONCURRENT=60
1313
maxSockets=50
1414
DELAY=5

‎lib/agent.js‎

Lines changed: 120 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,17 +20,131 @@ var http = require('http');
2020
var https = require('https');
2121
var util = require('util');
2222

23-
if (typeof http.Agent.request === 'function') {
24-
// node >= 0.11, default Agent is keepavlie
25-
module.exports = http.Agent;
26-
module.exports.HttpsAgent = https.Agent;
27-
return;
23+
var debug;
24+
if (process.env.NODE_DEBUG && /agentkeepalive/.test(process.env.NODE_DEBUG)) {
25+
debug = function (x) {
26+
console.error.apply(console, arguments);
27+
};
28+
} else {
29+
debug = function () { };
2830
}
2931

32+
function Agent(options) {
33+
options = options || {};
34+
options.keepAliveMsecs = options.keepAliveMsecs || options.maxKeepAliveTime;
35+
http.Agent.call(this, options);
36+
37+
var self = this;
38+
// max requests per keepalive socket, default is 0, no limit.
39+
self.maxKeepAliveRequests = parseInt(options.maxKeepAliveRequests, 10) || 0;
40+
// max keep alive time, default 60 seconds.
41+
// if set `keepAliveMsecs = 0`, will disable keepalive feature.
42+
self.createSocketCount = 0;
43+
self.timeoutSocketCount = 0;
44+
self.requestFinishedCount = 0;
45+
46+
// override the `free` event listener
47+
self.removeAllListeners('free');
48+
self.on('free', function (socket, options) {
49+
self.requestFinishedCount++;
50+
socket._requestCount++;
51+
52+
var name = self.getName(options);
53+
debug('agent.on(free)', name);
54+
55+
if (!socket.destroyed &&
56+
self.requests[name] && self.requests[name].length) {
57+
self.requests[name].shift().onSocket(socket);
58+
if (self.requests[name].length === 0) {
59+
// don't leak
60+
delete self.requests[name];
61+
}
62+
} else {
63+
// If there are no pending requests, then put it in
64+
// the freeSockets pool, but only if we're allowed to do so.
65+
var req = socket._httpMessage;
66+
if (req &&
67+
req.shouldKeepAlive &&
68+
!socket.destroyed &&
69+
self.options.keepAlive) {
70+
var freeSockets = self.freeSockets[name];
71+
var freeLen = freeSockets ? freeSockets.length : 0;
72+
var count = freeLen;
73+
if (self.sockets[name])
74+
count += self.sockets[name].length;
75+
76+
if (count >= self.maxSockets || freeLen >= self.maxFreeSockets) {
77+
self.removeSocket(socket, options);
78+
socket.destroy();
79+
} else {
80+
freeSockets = freeSockets || [];
81+
self.freeSockets[name] = freeSockets;
82+
socket.setKeepAlive(true, self.keepAliveMsecs);
83+
socket.unref && socket.unref();
84+
socket._httpMessage = null;
85+
self.removeSocket(socket, options);
86+
freeSockets.push(socket);
87+
88+
// Avoid duplicitive timeout events by removing timeout listeners set on
89+
// socket by previous requests. node does not do this normally because it
90+
// assumes sockets are too short-lived for it to matter. It becomes a
91+
// problem when sockets are being reused. Steps are being taken to fix
92+
// this issue upstream in node v0.10.0.
93+
//
94+
// See https://github.com/joyent/node/commit/451ff1540ab536237e8d751d241d7fc3391a4087
95+
if (self.keepAliveMsecs && socket._events && Array.isArray(socket._events.timeout)) {
96+
socket.removeAllListeners('timeout');
97+
// Restore the socket's setTimeout() that was remove as collateral
98+
// damage.
99+
socket.setTimeout(self.keepAliveMsecs, socket._maxKeepAliveTimeout);
100+
}
101+
}
102+
} else {
103+
self.removeSocket(socket, options);
104+
socket.destroy();
105+
}
106+
}
107+
});
108+
}
30109

31-
var Agent = require('./_http_agent').Agent;
110+
util.inherits(Agent, http.Agent);
32111
module.exports = Agent;
33112

113+
Agent.prototype.createSocket = function (req, options) {
114+
var self = this;
115+
var socket = http.Agent.prototype.createSocket.call(this, req, options);
116+
socket._requestCount = 0;
117+
if (self.keepAliveMsecs) {
118+
socket._maxKeepAliveTimeout = function () {
119+
debug('maxKeepAliveTimeout, socket destroy()');
120+
socket.destroy();
121+
self.timeoutSocketCount++;
122+
};
123+
socket.setTimeout(self.keepAliveMsecs, socket._maxKeepAliveTimeout);
124+
// Disable Nagle's algorithm: http://blog.caustik.com/2012/04/08/scaling-node-js-to-100k-concurrent-connections/
125+
socket.setNoDelay(true);
126+
}
127+
this.createSocketCount++;
128+
return socket;
129+
};
130+
131+
Agent.prototype.removeSocket = function (s, options) {
132+
http.Agent.prototype.removeSocket.call(this, s, options);
133+
var name = this.getName(options);
134+
debug('removeSocket', name, 'destroyed:', s.destroyed);
135+
136+
if (s.destroyed && this.freeSockets[name]) {
137+
var index = this.freeSockets[name].indexOf(s);
138+
if (index !== -1) {
139+
this.freeSockets[name].splice(index, 1);
140+
if (this.freeSockets[name].length === 0) {
141+
// don't leak
142+
delete this.freeSockets[name];
143+
}
144+
}
145+
}
146+
};
147+
34148
function HttpsAgent(options) {
35149
Agent.call(this, options);
36150
this.defaultPort = 443;

‎package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@
3232
"coveralls": "*",
3333
"mocha-lcov-reporter": "*"
3434
},
35-
"engines": { "node": ">= 0.10.0" },
35+
"engines": { "node": ">= 0.11.8" },
3636
"author": "fengmk2 <[email protected]> (http://fengmk2.github.com)",
3737
"license": "MIT"
3838
}

‎test/agent.test.js‎

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,10 @@ describe('agent.test.js', function () {
3333
} else if (req.url === '/hang') {
3434
// Wait forever.
3535
return;
36+
} else if (req.url === '/remote_close') {
37+
setTimeout(function () {
38+
req.connection.end();
39+
}, 500);
3640
}
3741
var info = urlparse(req.url, true);
3842
if (info.query.timeout) {
@@ -183,7 +187,7 @@ describe('agent.test.js', function () {
183187
}).on('error', done);
184188
});
185189

186-
it.skip('should use new socket when hit the max keepalive time: 1000ms', function (done) {
190+
it('should use new socket when hit the max keepalive time: 1000ms', function (done) {
187191
// socket._handle will not timeout ...
188192
var name = 'localhost:' + port + '::';
189193
agentkeepalive.sockets.should.have.not.key(name);
@@ -255,7 +259,6 @@ describe('agent.test.js', function () {
255259
it('should not keepalive when client.abort()', function (done) {
256260
var name = 'localhost:' + port + '::';
257261
agentkeepalive.sockets.should.have.not.key(name);
258-
agentkeepalive.freeSockets.should.have.not.key(name);
259262
var client = agentkeepalive.get({
260263
port: port,
261264
path: '/',
@@ -329,6 +332,36 @@ describe('agent.test.js', function () {
329332
agent.requests[name].should.length(1);
330333
});
331334

335+
it('should request /remote_close 200 status, after 500ms free socket close', function (done) {
336+
var name = 'localhost:' + port + '::';
337+
agentkeepalive.sockets.should.not.have.key(name);
338+
agentkeepalive.get({
339+
port: port,
340+
path: '/remote_close'
341+
}, function (res) {
342+
res.should.status(200);
343+
res.on('data', function (data) {
344+
});
345+
res.on('end', function () {
346+
agentkeepalive.sockets.should.have.key(name);
347+
agentkeepalive.freeSockets.should.not.have.key(name);
348+
setTimeout(function () {
349+
agentkeepalive.sockets.should.not.have.key(name);
350+
agentkeepalive.freeSockets.should.have.key(name);
351+
agentkeepalive.freeSockets[name].should.length(1);
352+
setTimeout(function () {
353+
agentkeepalive.sockets.should.not.have.key(name);
354+
agentkeepalive.freeSockets.should.not.have.key(name);
355+
done();
356+
}, 510);
357+
}, 10);
358+
});
359+
});
360+
agentkeepalive.sockets.should.have.key(name);
361+
agentkeepalive.sockets[name].should.length(1);
362+
agentkeepalive.freeSockets.should.not.have.key(name);
363+
});
364+
332365
// it('should maxKeepAliveRequests work with 1 and 10', function (done) {
333366
// var name = 'localhost:' + port + '::';
334367
// function request(agent, checkCount, callback) {

0 commit comments

Comments
 (0)