-
Notifications
You must be signed in to change notification settings - Fork 5.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
[http_listener_v2] Stop() succeeds even if fails to start #8502
[http_listener_v2] Stop() succeeds even if fails to start #8502
Conversation
In cases where the http_listener_v2 plugin config is invalid, when the agent attempts to cleanup by stopping all the inputs, the Stop method here panics as it tries to call listener.Stop() when no listener has been set. This also masks the error message returned from the Start method. ``` > telegraf --test 2020-10-27T12:21:45Z I! Starting Telegraf 1.16.0 2020-10-27T12:21:45Z I! Using config file: /etc/telegraf/telegraf.conf ... panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x28 pc=0x1245130] goroutine 45 [running]: github.com/influxdata/telegraf/plugins/inputs/http_listener_v2.(*HTTPListenerV2).Stop(0xc00043e000) /go/src/github.com/influxdata/telegraf/plugins/inputs/http_listener_v2/http_listener_v2.go:178 +0x30 github.com/influxdata/telegraf/agent.stopServiceInputs(0xc00045e480, 0x5, 0x8) /go/src/github.com/influxdata/telegraf/agent/agent.go:445 +0x82 github.com/influxdata/telegraf/agent.(*Agent).testRunInputs(0xc000288080, 0x32be8c0, 0xc0000f1f00, 0x0, 0xc00000f480, 0x0, 0x0) /go/src/github.com/influxdata/telegraf/agent/agent.go:434 +0x1b7 github.com/influxdata/telegraf/agent.(*Agent).test.func4(0xc000057b70, 0xc000288080, 0x32be8c0, 0xc0000f1f00, 0x0, 0xc00000f480) /go/src/github.com/influxdata/telegraf/agent/agent.go:977 +0x8b created by github.com/influxdata/telegraf/agent.(*Agent).test /go/src/github.com/influxdata/telegraf/agent/agent.go:975 +0x352 ``` This fixes this issue by checking if the listener has been set before calling listener.Stop. ``` > ./telegraf --config test.conf --test 2020-10-27T12:43:25Z I! Starting Telegraf 2020-10-27T12:43:25Z E! [agent] Starting input inputs.http_listener_v2: listen tcp: address address_without_port: missing port in address ```
I'm not sure it should even be calling stop if Start failed. |
Thanks for the review @ssoroka . It seems useful in a general sense for Stop to be called so the plugin can close any resources it did manage to open, but that said, the outcome of this is to exit anyway so that would only be useful for persistent resources. In any case, changing whether or not Stop is called would be a bigger change affecting all plugins. Do you know if it is normal that only AppVeyor CI ran? Seems like it only tests whether things work on Windows, while other PRs have CircleCI runs for the other platforms. Also, I had to retry the AppVeyor CI run because there was a transient error (not sure what, but it seems to have affected master too, e.g. 73986ac). Lastly, what are the next steps for this PR? Do I just wait for someone with write access to merge? Sorry, I'm new to this repo. |
* [http_listener_v2] Stop() succeeds even if fails to start In cases where the http_listener_v2 plugin config is invalid, when the agent attempts to cleanup by stopping all the inputs, the Stop method here panics as it tries to call listener.Stop() when no listener has been set. This also masks the error message returned from the Start method. ``` > telegraf --test 2020-10-27T12:21:45Z I! Starting Telegraf 1.16.0 2020-10-27T12:21:45Z I! Using config file: /etc/telegraf/telegraf.conf ... panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x28 pc=0x1245130] goroutine 45 [running]: github.com/influxdata/telegraf/plugins/inputs/http_listener_v2.(*HTTPListenerV2).Stop(0xc00043e000) /go/src/github.com/influxdata/telegraf/plugins/inputs/http_listener_v2/http_listener_v2.go:178 +0x30 github.com/influxdata/telegraf/agent.stopServiceInputs(0xc00045e480, 0x5, 0x8) /go/src/github.com/influxdata/telegraf/agent/agent.go:445 +0x82 github.com/influxdata/telegraf/agent.(*Agent).testRunInputs(0xc000288080, 0x32be8c0, 0xc0000f1f00, 0x0, 0xc00000f480, 0x0, 0x0) /go/src/github.com/influxdata/telegraf/agent/agent.go:434 +0x1b7 github.com/influxdata/telegraf/agent.(*Agent).test.func4(0xc000057b70, 0xc000288080, 0x32be8c0, 0xc0000f1f00, 0x0, 0xc00000f480) /go/src/github.com/influxdata/telegraf/agent/agent.go:977 +0x8b created by github.com/influxdata/telegraf/agent.(*Agent).test /go/src/github.com/influxdata/telegraf/agent/agent.go:975 +0x352 ``` This fixes this issue by checking if the listener has been set before calling listener.Stop. ``` > ./telegraf --config test.conf --test 2020-10-27T12:43:25Z I! Starting Telegraf 2020-10-27T12:43:25Z E! [agent] Starting input inputs.http_listener_v2: listen tcp: address address_without_port: missing port in address ``` * retry CI (cherry picked from commit 35a2210)
…#8502) * [http_listener_v2] Stop() succeeds even if fails to start In cases where the http_listener_v2 plugin config is invalid, when the agent attempts to cleanup by stopping all the inputs, the Stop method here panics as it tries to call listener.Stop() when no listener has been set. This also masks the error message returned from the Start method. ``` > telegraf --test 2020-10-27T12:21:45Z I! Starting Telegraf 1.16.0 2020-10-27T12:21:45Z I! Using config file: /etc/telegraf/telegraf.conf ... panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x28 pc=0x1245130] goroutine 45 [running]: github.com/influxdata/telegraf/plugins/inputs/http_listener_v2.(*HTTPListenerV2).Stop(0xc00043e000) /go/src/github.com/influxdata/telegraf/plugins/inputs/http_listener_v2/http_listener_v2.go:178 +0x30 github.com/influxdata/telegraf/agent.stopServiceInputs(0xc00045e480, 0x5, 0x8) /go/src/github.com/influxdata/telegraf/agent/agent.go:445 +0x82 github.com/influxdata/telegraf/agent.(*Agent).testRunInputs(0xc000288080, 0x32be8c0, 0xc0000f1f00, 0x0, 0xc00000f480, 0x0, 0x0) /go/src/github.com/influxdata/telegraf/agent/agent.go:434 +0x1b7 github.com/influxdata/telegraf/agent.(*Agent).test.func4(0xc000057b70, 0xc000288080, 0x32be8c0, 0xc0000f1f00, 0x0, 0xc00000f480) /go/src/github.com/influxdata/telegraf/agent/agent.go:977 +0x8b created by github.com/influxdata/telegraf/agent.(*Agent).test /go/src/github.com/influxdata/telegraf/agent/agent.go:975 +0x352 ``` This fixes this issue by checking if the listener has been set before calling listener.Stop. ``` > ./telegraf --config test.conf --test 2020-10-27T12:43:25Z I! Starting Telegraf 2020-10-27T12:43:25Z E! [agent] Starting input inputs.http_listener_v2: listen tcp: address address_without_port: missing port in address ``` * retry CI
In cases where the http_listener_v2 plugin config is invalid, when the agent attempts to cleanup by stopping all the inputs, the Stop method here panics as it tries to call listener.Stop() when no listener has been set. This also masks the error message returned from the Start method.
This PR fixes this issue by checking if the listener has been set before calling listener.Stop.
Required for all PRs: