-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[v*] Healthcheck goroutine leaks #543
Conversation
That looks interesting. Thanks for reporting/fixing it. BTW: Tests fail for Go 1.7 because |
Hmm. Stumbled upon something re: golang/go#20668. |
@utrack That works as expected here: Context is canceled.
|
@olivere on Go 1.8 it works OK, but fails on 1.7. |
@utrack Oh, I see. |
I've also prepared v2 backport, should I open up another PR? |
@olivere Fixed tests for all versions. It's not possible to monitor leaks directly on Go pre-1.8, so I'm using side effects to monitor stuck connections on pre-1.8. Seems Context is never canceled on 1.7 on server's side because it was not implemented then (but context itself is present in the request). Huh. |
@utrack Okay, thanks for figuring this out. Can you do three more things for me to merge this into v5:
Then we can put together PRs for v2 and v3, or I'll do this in a batch. |
Sure, done @olivere |
Merged and will be published soon. Thanks again! |
@utrack Do you mind to review #545 (and this commit)? It's this PR backported into elastic.v3. I will also backport to elastic.v2 later with #546, after merging #503 first. |
Reviewed everything :) Looks good, thank you for the client! |
@utrack Thanks. Have a nice weekend. cu. |
Looks like this breaks Go 1.6.2... I'm using elastic.v3... Upgrading Go to 1.8.3 resolved it...
|
@wtfiwtz I see. However, 1.6 is no longer supported. I try to support the current and previous versions of Go (while Go itself even only supports the current version). From my experience, updating Go is nowhere as critical as in other environments. |
Note that this bit me as well. I've updated to 1.8 and everything compiled. Thanks, -c |
This is a backport of olivere#543 for elastic.v3 (see also olivere#545).
Hi! This PR fixes goroutine leaks caused by healthcheck not using ctx properly.
I've managed to reproduce it via tests.