fix: fix memory leak if timeout
is set
#178
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue
Currently, if
timeout
option is provided, one more event listener is added to the underlying socket on each request. This causes the number oftimeout
event listeners for each socket to grow over time until the socket is destroyed. In my tests, the number of event listeners for single socket can exceed 400.Moreover, it closures Request object within timeout handler preventing it from being garbage collected far beyond its lifecycle.
Resolution
This PR changes timeout implementation, by adding
timeout
event listener directly to the Request object instead of the Socket. Considering thatrequest.setTimeout
implicitly calls an appropriatesocket.setTimeout()
(docs); andtimeout
event is propagated from the underlying socket to the request itself (docs); the original behavior will remain unchanged.At the same time this will prevent Socket event listeners count from growing and will also allow garbage collecting Request objects.
Also, this PR replaces deprecated
request.abort()
withrequest.destroy()
.