HTTP request setTimeout(0) does not remove timeout listener

[timdp]

• 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:

1. request.setTimeout calls listenSocketTimeout:
   https://github.com/nodejs/node/blob/v11.6.0/lib/_http_client.js#L736
2. listenSocketTimeout creates a proxy (timeout event) Socket -> ClientRequest: https://github.com/nodejs/node/blob/v11.6.0/lib/_http_client.js#L673
3. 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.

[addaleax]

@nodejs/http

[lpinca]

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);

[szmarczak]

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.

[timdp]

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. 🙂

[lpinca]

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

[timdp]

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