Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fix memory leak if timeout is set #1

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

masontikhonov
Copy link
Collaborator

@masontikhonov masontikhonov commented May 20, 2024

Note

Corresponding PR to upstream: apocas#178

Issue

Currently, if timeout option is provided, one more event listener is added to the underlying socket on each request. This causes the number of timeout 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.

image

Moreover, it closures Request object within timeout handler preventing it from being garbage collected far beyond its lifecycle.

image

Resolution

This PR changes timeout implementation, by adding timeout event listener directly to the Request object instead of the Socket. Considering that request.setTimeout implicitly calls an appropriate socket.setTimeout() (docs); and timeout 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.

image
image

Also, this PR replaces deprecated request.abort() with request.destroy().

@masontikhonov masontikhonov self-assigned this May 20, 2024
@masontikhonov masontikhonov marked this pull request as ready for review May 20, 2024 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants