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

clientv3: close watcher stream once all watchers detach #6136

Merged
merged 3 commits into from
Aug 9, 2016

Conversation

heyitsanthony
Copy link
Contributor

Fixes #6134

@xiang90
Copy link
Contributor

xiang90 commented Aug 9, 2016

#6134 also mentioned that there might be a server side memory leak. No?

@xiang90
Copy link
Contributor

xiang90 commented Aug 9, 2016

So we leak go routines perviously?

@xiang90
Copy link
Contributor

xiang90 commented Aug 9, 2016

LGTM. But I am not sure if this fixes all issues with #6134. I am going to test it out later. (Or you might already test it out?)

@heyitsanthony
Copy link
Contributor Author

The grpc stream wasn't being canceled when the last watcher on the stream was canceled, causing the watcher to stay open on the server; the watcher total metric wasn't decreasing before.

@xiang90
Copy link
Contributor

xiang90 commented Aug 9, 2016

@heyitsanthony Let's say we hard kill the client. The server will still experience the same issue? Or the total will decrease? I think I did do some experiments a while ago, it was fine. #6134 mentioned that he killed the process, but the server still has the memory there. I assume it is because of GC. But it is good to confirm.

@heyitsanthony
Copy link
Contributor Author

Haven't tested on hard kill. I think that path will be a little slower since the server won't know if the client is gone until it tries to send a message / keepalive and times out.

@xiang90
Copy link
Contributor

xiang90 commented Aug 9, 2016

@heyitsanthony I just worry about there might be a leak on the server side as well.

@heyitsanthony
Copy link
Contributor Author

OK, I tested with a small program that opens 10k watchers and sleeps forever. Metrics show 10k watchers while it's running. The 10k watchers vanish from the metrics when I kill -9 the program.

@xiang90
Copy link
Contributor

xiang90 commented Aug 9, 2016

LGTM. Thanks.

@heyitsanthony heyitsanthony merged commit 88a77f3 into etcd-io:master Aug 9, 2016
@heyitsanthony heyitsanthony deleted the fix-watcher-leak branch August 10, 2016 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants