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 socket file handle leaks from old blocking queries upon consul re… #3195

Merged
merged 3 commits into from
Jun 27, 2017

Conversation

preetapan
Copy link
Contributor

…load. This fixes issue #3018

Wired up a context to pass through via QueryOptions to the underlying http request. When the watch plan is stopped, it calls cancelFunc on that context.

@preetapan preetapan requested a review from slackpad June 26, 2017 20:59
Copy link
Contributor

@slackpad slackpad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good - one nit and one question about the context usage.

watch/watch.go Outdated
@@ -5,6 +5,8 @@ import (
"io"
"sync"

"context"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be sorted into the list above.

@@ -41,7 +42,7 @@ func keyWatch(params map[string]interface{}) (WatcherFunc, error) {
}
fn := func(p *Plan) (uint64, interface{}, error) {
kv := p.client.KV()
opts := consulapi.QueryOptions{AllowStale: stale, WaitIndex: p.lastIndex}
opts := makeQueryOptionsWithContext(p, stale)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do all of these watch functions need a defer cancel() in here to clean up in case the request goes through?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc says that it should do that, will fix. Doc also said that go vet should catch it, but it didn't - The go vet tool checks that CancelFuncs are used on all control-flow paths.

@slackpad
Copy link
Contributor

LGTM

@preetapan preetapan merged commit 48983bd into master Jun 27, 2017
@slackpad slackpad deleted the issue-3018-filehandle-leaks branch August 8, 2017 18:36
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