Skip to content

fix memory leak, which appears in case of multiple parallel requests #13

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

Merged
merged 5 commits into from
Aug 19, 2014
Merged

fix memory leak, which appears in case of multiple parallel requests #13

merged 5 commits into from
Aug 19, 2014

Conversation

philip1986
Copy link

In the case of simultaneous request the number event listeners on res.connection 'end' increases, depending by the amount of traffic. This patch will avoid this behavior.

@utvara
Copy link

utvara commented Aug 14, 2014

To clarify, this problem is caused by underlying socket not closing if http is using keep-alive. I believe that the solution does not change the current behaviour of the lib too much.

@pocesar
Copy link
Owner

pocesar commented Aug 14, 2014

Node.js uses a pool of sockets internally, and you can't enforce the client behavior like @utvara said, unless we implement a timeout config for idle sockets (can happen in all endpoints, when a connection is made but no requests sent, it's usually a slowloris attack)

@utvara
Copy link

utvara commented Aug 18, 2014

@pocesar I'm not sure what the intention of the emitting end was. Only reference I found in the code was for streaming over http. The PR does not change behavior for use case I just mentioned.

We are using json-rpc2 in production under some stress (currently 1k req per sec) and we need to push it 10 fold. It would be appreciated if you would accept the PR or propose how we can make it better.

Tnx in advance,
utvara

@pocesar
Copy link
Owner

pocesar commented Aug 19, 2014

@utvara it's just a matter to forward events that happen on the abstraction. for example, if you are holding the connected client list to a third party, like redis, you need to do a cleanup by your own, as far as I can think about it. I'll merge the PR, but it's not the solution for this, the best would be set a sensible timeout on endpoints after a couple of seconds/minutes of inactivity.

@pocesar pocesar merged commit 1fe3e71 into pocesar:master Aug 19, 2014
pocesar added a commit that referenced this pull request Aug 19, 2014
@pocesar
Copy link
Owner

pocesar commented Aug 19, 2014

released on npm as 0.8.0, thanks

@philip1986
Copy link
Author

thx

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