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

Error handling for docker swarm mode #1533

Merged
merged 1 commit into from
Jun 20, 2017

Conversation

tanyadegurechaff
Copy link
Contributor

Description

Fixes #1100

Copy link
Member

@emilevauge emilevauge left a comment

Choose a reason for hiding this comment

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

@tanyadegurechaff
You introduced a memory leak (you need to close the new channel).
Could you explain in details how your code fixes the issue? Isn't this issue Swarm related instead ?
Ping @vdemeester

@tanyadegurechaff
Copy link
Contributor Author

This makes backoff operation return error when an error occurs, and the operation will be retried.

@tanyadegurechaff
Copy link
Contributor Author

Could you explain in detail how a memory leak comes about?

@beldpro-ci
Copy link

AFAIK not closing a channel does not introduce a memory leak by itself (as one could worry in regards to files). As I see it's just a mechanism for communicating that nothing more will be sent there, right? (I'd really like to know if I got something wrong).

@emilevauge
Copy link
Member

emilevauge commented Jun 12, 2017

@tanyadegurechaff @beldpro-ci After reading more in details, indeed, it does not introduce any memory leak in this case. But it is still better to defer close the channel, especially because it's inside a go routine. We had many leaks in the past due to opened channels. Other than that, seems great :)

@tanyadegurechaff
Copy link
Contributor Author

Thank you for your feedbacks. @beldpro-ci @emilevauge

// TODO: This need to be change. Linked to Swarm events docker/docker#23827
ticker := time.NewTicker(SwarmDefaultWatchTime)
pool.Go(func(stop chan bool) {
defer close(errChan)
Copy link
Contributor

Choose a reason for hiding this comment

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

why you put the defer here?
I think you must move this line under the line 156.

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 receiver at line 185 blocks the execution until the channel is closed or received an error.

Copy link
Member

@emilevauge emilevauge Jun 14, 2017

Choose a reason for hiding this comment

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

I agree with @tanyadegurechaff on this. But it would be better to use this syntax instead:

if err, ok := <-errChan; ok {
  return err
} else {
  // channel closed
  ...
}

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but I got an error from golint:

provider/docker/docker.go:187:13: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)

Copy link
Contributor

@ldez ldez Jun 14, 2017

Choose a reason for hiding this comment

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

just do:

if err, ok := <-errChan; ok {
  return err
}
// channel closed
// ...

@ldez
Copy link
Contributor

ldez commented Jun 14, 2017

Due to SemaphoreCI, I close and reopen the PR.

@ldez ldez closed this Jun 14, 2017
@ldez ldez reopened this Jun 14, 2017
@ldez ldez force-pushed the fix-swarm-watch branch 2 times, most recently from 01127f9 to f5e878a Compare June 19, 2017 18:34
Copy link
Member

@emilevauge emilevauge left a comment

Choose a reason for hiding this comment

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

Thanks @tanyadegurechaff
LGTM !

@ldez ldez added the kind/enhancement a new or improved feature. label Jun 20, 2017
@ldez ldez merged commit 984ea10 into traefik:master Jun 20, 2017
@ldez ldez modified the milestone: 1.4 Jun 20, 2017
@tanyadegurechaff tanyadegurechaff deleted the fix-swarm-watch branch June 20, 2017 23:43
@ldez ldez added kind/bug/fix a bug fix and removed kind/enhancement a new or improved feature. labels Aug 14, 2017
@ldez ldez changed the title Fix error handling for docker swarm mode Error handling for docker swarm mode Aug 14, 2017
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