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 k8s panic #900

Merged
merged 2 commits into from
Nov 28, 2016
Merged

Fix k8s panic #900

merged 2 commits into from
Nov 28, 2016

Conversation

emilevauge
Copy link
Member

@emilevauge emilevauge commented Nov 24, 2016

Fixes #877
Fixes #869
DO NOT merge before validation made in #877

Copy link
Contributor

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐯

Signed-off-by: Emile Vauge <emile@vauge.com>
@jonaz
Copy link
Contributor

jonaz commented Nov 25, 2016

Should'nt this be enought? Whats the motives to use "third" party libs in stead of just channels and goroutines directly?

@@ -270,9 +271,9 @@ func (c *clientImpl) watch(url string, labelSelector string, stopCh <-chan bool)
 
        go func() {
                defer close(watchCh)
-               defer close(errCh)
                go func() {
                        defer res.Body.Close()
+                       defer close(errCh)
                        for {
                                var eventList interface{}
                                if err := json.NewDecoder(res.Body).Decode(&eventList); err != nil {

@emilevauge
Copy link
Member Author

@jonaz I don't know what you are calling "third party libs" here.
safe.Go is taken from Traefik directly. The goal is to avoid Traefik to panic and stop. So it is not useless ;)

@jonaz
Copy link
Contributor

jonaz commented Nov 25, 2016

I mean some external package thats not part of go. Just thought the fix was a bit to complicated for what was needed. But i'll admit i did not thoroughly read the safe packge from traefik.

@jonaz
Copy link
Contributor

jonaz commented Nov 25, 2016

Is'nt there a possible goroutine leak aswell?

	safe.Go(func() {
		EndCh := make(chan bool, 1)
		defer close(watchCh)
		defer close(errCh)
		defer close(EndCh)
		safe.Go(func() {
			defer res.Body.Close()
			defer func() {
				EndCh <- true
			}()
			for {
				var eventList interface{}
				if err := json.NewDecoder(res.Body).Decode(&eventList); err != nil {
					if !strings.Contains(err.Error(), "net/http: request canceled") {
						errCh <- fmt.Errorf("failed to decode watch event: GET %q : %v", url, err)
					}
					return
				}
				watchCh <- eventList
			}
		})
		<-stopCh **// Wont we listen here forever in case of error in Decode?** 
		safe.Go(func() {
			cancel() // cancel watch request
		})
		<-EndCh
	})

provider/kubernetes.go seems to check for error channel and send stop to it. But i dont think a client should be dependant on the usage not to leak goroutines.

DISCLAMER: This maybe should belong in another issue on github.

@emilevauge
Copy link
Member Author

@jonaz goroutine leak has been tested quite intensively during the RC phase.
Besides, we are in the process of moving to the official k8s client in the next release.

@jonaz
Copy link
Contributor

jonaz commented Nov 25, 2016

Good. Official client should be sufficient! :D

Copy link
Contributor

@errm errm left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants