Permalink
Browse files

http: Remove timeout handler when data arrives

  • Loading branch information...
1 parent 58a5bc1 commit 451ff1540ab536237e8d751d241d7fc3391a4087 @fredericgermain fredericgermain committed with isaacs Sep 9, 2012
Showing with 96 additions and 0 deletions.
  1. +7 −0 lib/http.js
  2. +89 −0 test/simple/test-http-client-timeout-agent.js
View
@@ -1474,6 +1474,10 @@ function responseOnEnd() {
assert(!socket.writable);
} else {
debug('AGENT socket keep-alive');
+ if (req.timeoutCb) {
+ socket.setTimeout(0, req.timeoutCb);
+ req.timeoutCb = null;
+ }
socket.removeListener('close', socketCloseListener);
socket.removeListener('error', socketErrorListener);
socket.emit('free');
@@ -1554,6 +1558,9 @@ ClientRequest.prototype.setTimeout = function(msecs, callback) {
}
if (this.socket && this.socket.writable) {
+ if (this.timeoutCb)
+ this.socket.setTimeout(0, this.timeoutCb);
+ this.timeoutCb = emitTimeout;
this.socket.setTimeout(msecs, emitTimeout);
return;
}
@@ -0,0 +1,89 @@
+// 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 request_number = 0;
+var requests_sent = 0;
+var requests_done = 0;
+var options = {
+ method: 'GET',
+ port: common.PORT,
+ host: '127.0.0.1',
+};
+
+//http.globalAgent.maxSockets = 15;
+
+var server = http.createServer(function(req, res) {
+ var m = /\/(.*)/.exec(req.url),
+ reqid = parseInt(m[1], 10);
+ if ( reqid % 2 ) {
+ // do not reply the request
+ } else {
+ res.writeHead(200, {'Content-Type': 'text/plain'});
+ res.write(reqid.toString());
+ res.end();
+ }
+ request_number+=1;
+});
+
+server.listen(options.port, options.host, function() {
+ var req;
+
+ for (requests_sent = 0; requests_sent < 30; requests_sent+=1) {
+ options.path = '/' + requests_sent;
+ req = http.request(options);
+ req.id = requests_sent;
+ req.on('response', function(res) {
+ res.on('data', function(data) {
+ console.log('res#'+this.req.id+' data:'+data);
+ });
+ res.on('end', function(data) {
+ console.log('res#'+this.req.id+' end');
+ requests_done += 1;
+ });
+ });
+ req.on('close', function() {
+ console.log('req#'+this.id+' close');
+ });
+ req.on('error', function() {
+ console.log('req#'+this.id+' error');
+ this.destroy();
+ });
+ req.setTimeout(50, function () {
+ var req = this;
+ console.log('req#'+this.id + ' timeout');
+ req.abort();
+ requests_done += 1;
+ });
+ req.end();
+ }
+ setTimeout(function() {
+ server.close();
+ }, 150);
+});
+
+process.on('exit', function() {
+ console.error('done=%j sent=%j', requests_done, requests_sent);
+ assert.ok(requests_done == requests_sent, 'timeout on http request called too much');
+});

2 comments on commit 451ff15

@bnoordhuis
Member

The test is consistently failing for me.

$ python tools/test.py --mode=release simple/test-http-client-timeout-agent
=== release test-http-client-timeout-agent ===                    
Path: simple/test-http-client-timeout-agent
res#0 data:0
res#0 end
res#2 data:2
res#2 end
res#4 data:4
res#4 end
res#6 data:6
res#6 end
res#8 data:8
req#3 timeout
req#1 timeout
req#3 close
req#3 error
req#1 close
req#1 error
req#7 timeout
req#5 timeout
req#7 close
req#7 error
req#5 close
req#5 error
res#10 data:10
res#10 end
res#12 data:12
res#12 end
res#14 data:14
res#8 end
res#14 end
res#16 data:16
req#9 timeout
req#9 close
req#9 error
req#11 timeout
req#11 close
req#11 error
res#18 data:18
res#18 end
req#13 timeout
req#13 close
req#13 error
res#20 data:20
res#20 end
req#15 timeout
req#15 close
req#15 error
res#22 data:22
res#22 end
res#16 end
res#24 data:24
req#17 timeout
req#17 close
req#17 error
req#25 error
req#25 close
req#26 error
req#26 close
req#19 timeout
req#21 timeout
req#19 close
req#19 error
req#21 close
req#21 error
req#29 error
req#29 close
req#28 error
req#28 close
req#27 error
req#27 close
res#24 end
req#23 timeout
req#23 close
req#23 error
done=25 sent=30

assert.js:102
  throw new assert.AssertionError({
        ^
AssertionError: timeout on http request called too much
    at process.<anonymous> (/home/bnoordhuis/src/nodejs/v0.8/test/simple/test-http-client-timeout-agent.js:88:10)
    at process.EventEmitter.emit (events.js:115:20)
Command: out/Release/node /home/bnoordhuis/src/nodejs/v0.8/test/simple/test-http-client-timeout-agent.js
[00:00|% 100|+   0|-   1]: Done                                  
@langpavel

+1

Please sign in to comment.