-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Conversation
6cab812
to
d1f64da
Compare
701201c
to
fa4cb0b
Compare
if s.ln != nil { | ||
return s.ln.Close() | ||
if s.closed() { | ||
s.Logger.Println("Service already closed.") |
There was a problem hiding this comment.
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.
@jwilder ready for approval |
There was a problem hiding this 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 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Required for all non-trivial PRs
This PR ensures that input services (
udp
,graphite
,opentsdb
andcollectd
) can be idempotently opened and closed.It also fixes a goroutine leak where a listener was not being closed properly.