HTTP request setTimeout(0) does not remove timeout listener

@szmarczak

Version: v11.6.0
Platform: all
Subsystem: http, net

This was originally discovered by @szmarczak and reported as sindresorhus/got#690.

Unless we're mistaken, setTimeout(0) on an HTTP request (and then socket) can lead to a timeout listener not being removed. This is problematic because combined with a keepalive agent, it can lead to a memory leak.

This Gist by Szymon illustrates the issue. For convenience, I'm also quoting his inline explanation here:

request.setTimeout calls listenSocketTimeout:
https://github.com/nodejs/node/blob/v11.6.0/lib/_http_client.js#L736

listenSocketTimeout creates a proxy (timeout event) Socket -> ClientRequest: https://github.com/nodejs/node/blob/v11.6.0/lib/_http_client.js#L673

The proxy event is released on the response end event:
https://github.com/nodejs/node/blob/v11.6.0/lib/_http_client.js#L682

In this case this never happens, because we set the timeout on that particular event. The correct behaviour would be to prevent calling listenSocketTimeout if msecs is 0.

@nodejs/http

Is there any reason for using req.setTimeout(0) after response ends?

This re-adds the listener right after it is removed

node/lib/_http_client.js

Lines 591 to 594 in 968e901

    
           if (req.timeoutCb) { 
        
             socket.setTimeout(0, req.timeoutCb); 
        
             req.timeoutCb = null; 
        
           }

because the timeoutCb flag is cleared when response ends.

I think we can make req.setTimeout() a noop if used after the 'end' event is emitted on the response.

diff --git a/lib/_http_client.js b/lib/_http_client.js
index 95488bc5e7..2e9d22389d 100644
--- a/lib/_http_client.js
+++ b/lib/_http_client.js
@@ -731,6 +731,9 @@ function _deferToConnect(method, arguments_, cb) {
 }
 
 ClientRequest.prototype.setTimeout = function setTimeout(msecs, callback) {
+  if (this._ended)
+    return this;
+
   listenSocketTimeout(this);
   msecs = validateTimerDuration(msecs);
   if (callback) this.once('timeout', callback);

Is there any reason for using req.setTimeout(0) after response ends?

When the response ends (or the user cancels the request), got clears all timeouts to make sure no TimeoutError will be thrown. There are some custom timeouts - see this timed-out module.

@szmarczak

And even if @szmarczak didn't have a valid reason to do so, a bit of state checking that prevents the memory leak would be cool. 🙂

Yes I agree. If you have some spare time to open a PR please do.

Spare time is debatable but I've created #25536. (Sorry about the force-push spam.)

http: make ClientRequest#setTimeout() a no-op after end

http: make ClientRequest#setTimeout() noop at end

Navigation Menu

Repository navigation

HTTP request setTimeout(0) does not remove timeout listener #25499

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	if (req.timeoutCb) {
	socket.setTimeout(0, req.timeoutCb);
	req.timeoutCb = null;
	}

HTTP request setTimeout(0) does not remove timeout listener #25499

Description

Activity

addaleax commented on Jan 14, 2019

lpinca commented on Jan 16, 2019

szmarczak commented on Jan 16, 2019

timdp commented on Jan 16, 2019

lpinca commented on Jan 16, 2019

timdp commented on Jan 16, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions