http: fix request with option timeout and agent

When request with both timeout and agent, timeout not work. This patch will fix it, socket timeout will set to request timeout before socket is connected, and socket timeout will reset to agent timeout after response end. Fixes: #21185 PR-URL: #21204 Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>

Author: killagu

doc/api/http.md

@@ -126,6 +126,8 @@ added: v0.3.4
 *`maxFreeSockets`{number} Maximum number of sockets to leave open
  in a free state. Only relevant if `keepAlive` is set to `true`.
 **Default:**`256`.
+*`timeout`{number} Socket timeout in milliseconds.
+ This will set the timeout after the socket is connected.
 
 The default [`http.globalAgent`][] that is used by [`http.request()`][] has all
 of these values set to their respective defaults.

lib/_http_agent.js

@@ -66,7 +66,8 @@ function Agent(options){
 
 if(socket.writable&&
 this.requests[name]&&this.requests[name].length){
-this.requests[name].shift().onSocket(socket);
+constreq=this.requests[name].shift();
+setRequestSocket(this,req,socket);
 if(this.requests[name].length===0){
 // don't leak
 deletethis.requests[name];
@@ -176,12 +177,12 @@ Agent.prototype.addRequest = function addRequest(req, options, port/* legacy */,
 deletethis.freeSockets[name];
 
 this.reuseSocket(socket,req);
-req.onSocket(socket);
+setRequestSocket(this,req,socket);
 this.sockets[name].push(socket);
 }elseif(sockLen<this.maxSockets){
 debug('call onSocket',sockLen,freeLen);
 // If we are under maxSockets create a new one.
-this.createSocket(req,options,handleSocketCreation(req,true));
+this.createSocket(req,options,handleSocketCreation(this,req,true));
 }else{
 debug('wait for socket');
 // We are over limit so we'll add it to the queue.
@@ -305,9 +306,10 @@ Agent.prototype.removeSocket = function removeSocket(s, options){
 
 if(this.requests[name]&&this.requests[name].length){
 debug('removeSocket, have a request, make a socket');
-varreq=this.requests[name][0];
+constreq=this.requests[name][0];
 // If we have pending requests and a socket gets closed make a new one
-this.createSocket(req,options,handleSocketCreation(req,false));
+constsocketCreationHandler=handleSocketCreation(this,req,false);
+this.createSocket(req,options,socketCreationHandler);
 }
 };
 
@@ -337,19 +339,36 @@ Agent.prototype.destroy = function destroy(){
 }
 };
 
-functionhandleSocketCreation(request,informRequest){
+functionhandleSocketCreation(agent,request,informRequest){
 returnfunctionhandleSocketCreation_Inner(err,socket){
 if(err){
 process.nextTick(emitErrorNT,request,err);
 return;
 }
 if(informRequest)
-request.onSocket(socket);
+setRequestSocket(agent,request,socket);
 else
 socket.emit('free');
 };
 }
 
+functionsetRequestSocket(agent,req,socket){
+req.onSocket(socket);
+constagentTimeout=agent.options.timeout||0;
+if(req.timeout===undefined||req.timeout===agentTimeout){
+return;
+}
+socket.setTimeout(req.timeout);
+// reset timeout after response end
+req.once('response',(res)=>{
+res.once('end',()=>{
+if(socket.timeout!==agentTimeout){
+socket.setTimeout(agentTimeout);
+}
+});
+});
+}
+
 functionemitErrorNT(emitter,err){
 emitter.emit('error',err);
 }

lib/_http_client.js

@@ -639,18 +639,35 @@ function tickOnSocket(req, socket){
 socket.on('end',socketOnEnd);
 socket.on('close',socketCloseListener);
 
-if(req.timeout){
-constemitRequestTimeout=()=>req.emit('timeout');
-socket.once('timeout',emitRequestTimeout);
-req.once('response',(res)=>{
-res.once('end',()=>{
-socket.removeListener('timeout',emitRequestTimeout);
-});
-});
+if(req.timeout!==undefined){
+listenSocketTimeout(req);
 }
 req.emit('socket',socket);
 }
 
+functionlistenSocketTimeout(req){
+if(req.timeoutCb){
+return;
+}
+constemitRequestTimeout=()=>req.emit('timeout');
+// Set timeoutCb so it will get cleaned up on request end.
+req.timeoutCb=emitRequestTimeout;
+// Delegate socket timeout event.
+if(req.socket){
+req.socket.once('timeout',emitRequestTimeout);
+}else{
+req.on('socket',(socket)=>{
+socket.once('timeout',emitRequestTimeout);
+});
+}
+// Remove socket timeout listener after response end.
+req.once('response',(res)=>{
+res.once('end',()=>{
+req.socket.removeListener('timeout',emitRequestTimeout);
+});
+});
+}
+
 ClientRequest.prototype.onSocket=functiononSocket(socket){
 process.nextTick(onSocketNT,this,socket);
 };
@@ -700,42 +717,29 @@ function _deferToConnect(method, arguments_, cb){
 }
 
 ClientRequest.prototype.setTimeout=functionsetTimeout(msecs,callback){
+listenSocketTimeout(this);
 msecs=validateTimerDuration(msecs);
 if(callback)this.once('timeout',callback);
 
-constemitTimeout=()=>this.emit('timeout');
-
-if(this.socket&&this.socket.writable){
-if(this.timeoutCb)
-this.socket.setTimeout(0,this.timeoutCb);
-this.timeoutCb=emitTimeout;
-this.socket.setTimeout(msecs,emitTimeout);
-returnthis;
-}
-
-// Set timeoutCb so that it'll get cleaned up on request end
-this.timeoutCb=emitTimeout;
 if(this.socket){
-varsock=this.socket;
-this.socket.once('connect',function(){
-sock.setTimeout(msecs,emitTimeout);
-});
-returnthis;
+setSocketTimeout(this.socket,msecs);
+}else{
+this.once('socket',(sock)=>setSocketTimeout(sock,msecs));
 }
 
-this.once('socket',function(sock){
-if(sock.connecting){
-sock.once('connect',function(){
-sock.setTimeout(msecs,emitTimeout);
-});
-}else{
-sock.setTimeout(msecs,emitTimeout);
-}
-});
-
 returnthis;
 };
 
+functionsetSocketTimeout(sock,msecs){
+if(sock.connecting){
+sock.once('connect',function(){
+sock.setTimeout(msecs);
+});
+}else{
+sock.setTimeout(msecs);
+}
+}
+
 ClientRequest.prototype.setNoDelay=functionsetNoDelay(noDelay){
 this._deferToConnect('setNoDelay',[noDelay]);
 };

lib/net.js

@@ -408,6 +408,7 @@ function writeAfterFIN(chunk, encoding, cb){
 }
 
 Socket.prototype.setTimeout=function(msecs,callback){
+this.timeout=msecs;
 // Type checking identical to timers.enroll()
 msecs=validateTimerDuration(msecs);
 

test/parallel/test-http-client-set-timeout.js

@@ -0,0 +1,46 @@
+'use strict';
+constcommon=require('../common');
+
+// Test that `req.setTimeout` will fired exactly once.
+
+constassert=require('assert');
+consthttp=require('http');
+
+constHTTP_CLIENT_TIMEOUT=2000;
+
+constoptions={
+method: 'GET',
+port: undefined,
+host: '127.0.0.1',
+path: '/',
+timeout: HTTP_CLIENT_TIMEOUT,
+};
+
+constserver=http.createServer(()=>{
+// Never respond.
+});
+
+server.listen(0,options.host,function(){
+doRequest();
+});
+
+functiondoRequest(){
+options.port=server.address().port;
+constreq=http.request(options);
+req.setTimeout(HTTP_CLIENT_TIMEOUT/2);
+req.on('error',()=>{
+// This space is intentionally left blank.
+});
+req.on('close',common.mustCall(()=>server.close()));
+
+lettimeout_events=0;
+req.on('timeout',common.mustCall(()=>{
+timeout_events+=1;
+}));
+req.end();
+
+setTimeout(function(){
+req.destroy();
+assert.strictEqual(timeout_events,1);
+},common.platformTimeout(HTTP_CLIENT_TIMEOUT));
+}

test/parallel/test-http-client-timeout-option-with-agent.js

@@ -0,0 +1,55 @@
+'use strict';
+constcommon=require('../common');
+
+// Test that when http request uses both timeout and agent,
+// timeout will work as expected.
+
+constassert=require('assert');
+consthttp=require('http');
+
+constHTTP_AGENT_TIMEOUT=1000;
+constHTTP_CLIENT_TIMEOUT=3000;
+
+constagent=newhttp.Agent({timeout: HTTP_AGENT_TIMEOUT});
+constoptions={
+method: 'GET',
+port: undefined,
+host: '127.0.0.1',
+path: '/',
+timeout: HTTP_CLIENT_TIMEOUT,
+ agent,
+};
+
+constserver=http.createServer(()=>{
+// Never respond.
+});
+
+server.listen(0,options.host,function(){
+doRequest();
+});
+
+functiondoRequest(){
+options.port=server.address().port;
+constreq=http.request(options);
+conststart=Date.now();
+req.on('error',()=>{
+// This space is intentionally left blank.
+});
+req.on('close',common.mustCall(()=>server.close()));
+
+lettimeout_events=0;
+req.on('timeout',common.mustCall(()=>{
+timeout_events+=1;
+constduration=Date.now()-start;
+// The timeout event cannot be precisely timed. It will delay
+// some number of milliseconds, so test it in second units.
+assert.strictEqual(duration/1000|0,HTTP_CLIENT_TIMEOUT/1000);
+}));
+req.end();
+
+setTimeout(function(){
+req.destroy();
+assert.strictEqual(timeout_events,1);
+// Ensure the `timeout` event fired only once.
+},common.platformTimeout(HTTP_CLIENT_TIMEOUT*2));
+}