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

Timeout for handshaken clients which never connect to fix memory leak #445

Closed
wants to merge 1 commit into from
Closed

Conversation

squidfunk
Copy link

Under some circumstances there seem to be clients which only perform one handshake and are then never seen again. I don't know the exact source of the problem, but it shouldn't affect the server. However, this problem introduces a memory leak in manager.js. The clients are not correctly removed from the "handshaken" member in manager.js, leading to a memory leak and frequent crashes of the server because of constantly increasing load and cpu (most probably because of operations on this member taking longer and longer).

Here is a graph to illustrate the problem:
http://img684.imageshack.us/img684/9707/handshakememleak.png

(The drop at around 18:00 was the result of a crash)

The fix introduces a timeout which fires after the "client store expiration" and checks if the client has connected within the given interval. If he hasn't, the client is deleted from the "handshaken" member. This fixes the problem of the evergrowing "handshaken" member.

@3rd-Eden
Copy link
Contributor

3rd-Eden commented Aug 4, 2011

It's a known issue (I also fixed it already: #408). But @guille is still planning some changes to the handshaking logic so the declined it.

Anyways, you are also missing

this.store.publish('handshake', data.id, handshaken);

in the patch, this causes the the changes to replicated to different processes.

@squidfunk
Copy link
Author

Ah okay, haven't seen your pull request. However, I can confirm that this is a serious issue in a production environment and hope this will be included in the next release.

@squidfunk squidfunk closed this Aug 4, 2011
@3rd-Eden
Copy link
Contributor

3rd-Eden commented Aug 5, 2011

A fix landed in master.

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.

2 participants