Repository navigation
HTTP request setTimeout(0) does not remove timeout listener #25499
Description
Activity
@nodejs/http
Is there any reason for using
req.setTimeout(0)after response ends?This re-adds the listener right after it is removed
Lines 591 to 594 in 968e901
because the
timeoutCbflag 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);
Reacted by Szymon MarczakIs there any reason for using req.setTimeout(0) after response ends?
When the response ends (or the user cancels the request),
gotclears all timeouts to make sure no TimeoutError will be thrown. There are some custom timeouts - see thistimed-outmodule.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.
Reacted by Szymon Marczak- added a commit that references this issue
on Jan 17, 2019 - added a commit that references this issue
on Jan 23, 2019 - added a commit that references this issue
on Apr 29, 2019 - added a commit that references this issue
on May 10, 2019 - added a commit that references this issue
on May 16, 2019 - added a commit that references this issue
on Jul 27, 2026
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 atimeoutlistener 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: