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

Ensure input services can be safely opened and closed #7463

Merged
merged 2 commits into from
Oct 18, 2016
Merged

Conversation

e-dard
Copy link
Contributor

@e-dard e-dard commented Oct 13, 2016

Required for all non-trivial PRs
  • Rebased/mergable
  • Tests pass

This PR ensures that input services (udp, graphite, opentsdb and collectd) can be idempotently opened and closed.

It also fixes a goroutine leak where a listener was not being closed properly.

@e-dard e-dard force-pushed the er-services branch 2 times, most recently from 6cab812 to d1f64da Compare October 14, 2016 13:11
@e-dard e-dard changed the title [WIP] Ensure input services can be safely opened and closed Ensure input services can be safely opened and closed Oct 14, 2016
@e-dard e-dard force-pushed the er-services branch 3 times, most recently from 701201c to fa4cb0b Compare October 17, 2016 14:17
if s.ln != nil {
return s.ln.Close()
if s.closed() {
s.Logger.Println("Service already closed.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove? Not sure this really provides any value.

@e-dard
Copy link
Contributor Author

e-dard commented Oct 18, 2016

@jwilder ready for approval

Copy link
Contributor

@jwilder jwilder left a comment

Choose a reason for hiding this comment

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

One nit. LGTM

@@ -225,7 +257,7 @@ func (s *Service) Statistics(tags map[string]string) []models.Statistic {
}

// Err returns a channel for fatal errors that occur on the listener.
func (s *Service) Err() <-chan error { return s.err }
// func (s *Service) Err() <-chan error { return s.err }
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eurgh sorry I meant to remove this function. Nothing calls it. I don't think we need it on the services do we @jwilder?

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's not used by anything, then it could be removed.

@e-dard e-dard merged commit d3f3e02 into master Oct 18, 2016
@e-dard e-dard deleted the er-services branch October 18, 2016 15:49
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.

None yet

2 participants