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 address already in use with webhooks input during reload #3206

Merged
merged 2 commits into from
Sep 11, 2017

Conversation

dsalbert
Copy link
Contributor

@dsalbert dsalbert commented Sep 7, 2017

Change HTTP listener by removing ListenAndServe() method and implementing Close() net.Listen() methods.
Change error reporting, by disabling adding it in case when http.ErrServerClosed occurred.

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

Change HTTP lister, to implement Close() method and avoid displaying error by adding condition based on `http.ErrServerClosed` error.

ln, err := net.Listen("tcp", fmt.Sprintf("%s", wb.ServiceAddress))
if err != nil {
acc.AddError(fmt.Errorf("E! Error starting server: %v", err))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should return this error, making it fatal and preventing Telegraf from starting. This probably indicates the port is in use or a privilege issue, and won't resolve, so I think exit is the right action even though it is technically a change in behavior.

err := http.ListenAndServe(fmt.Sprintf("%s", wb.ServiceAddress), r)
if err != nil {
acc.AddError(fmt.Errorf("E! Error starting server: %v", err))
func (wb *Webhooks) listen(ln net.Listener, acc telegraf.Accumulator) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets call this serve since we are already listening.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or just inline it in the Start func.

@danielnelson
Copy link
Contributor

cc @francois2metz

Copy link
Contributor

@francois2metz francois2metz left a comment

Choose a reason for hiding this comment

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

LGTM

@danielnelson danielnelson changed the title fix issue 3136 Fix address already in use with webhooks input during reload Sep 7, 2017
@danielnelson danielnelson modified the milestone: 1.5.0 Sep 8, 2017
Move content of listen() function to inline function inside Start() function.
Report fatal and return from Start() function (exit program) in case where net.Listen() has any problem.
@danielnelson danielnelson merged commit f62e543 into influxdata:master Sep 11, 2017
@francois2metz
Copy link
Contributor

@dankans thanks!

@dsalbert
Copy link
Contributor Author

Thanks @danielnelson and @francois2metz for help!

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.

3 participants