-
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
socket_listener: clean up unix socket file on start & stop #2618
socket_listener: clean up unix socket file on start & stop #2618
Conversation
@@ -40,7 +39,6 @@ func TestSocketListener_udp(t *testing.T) { | |||
} | |||
|
|||
func TestSocketListener_unix(t *testing.T) { | |||
defer os.Remove("/tmp/telegraf_test.sock") |
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.
Dunno how I forgot about the issue when I explicitly put the removal of the file in the test code...
sl := newSocketListener() | ||
sl.ServiceAddress = "unixgram:///tmp/telegraf_test.sock" | ||
|
||
acc := &testutil.Accumulator{} | ||
err := sl.Start(acc) | ||
require.NoError(t, err) | ||
defer sl.Stop() |
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.
This was added so that the goroutines are shut down and the unix socket file removed. Otherwise they just get killed when go exits (finishes the tests), and the file is left around.
listener, err := net.Listen("unix", "/tmp/telegraf_test.sock") | ||
require.NoError(t, err) | ||
defer os.Remove("/tmp/telegraf_test.sock") |
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.
I think the defer needs to move upwards to ensure it is ran even if the require.NoError fails.
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.
Well if it errored, then the listen failed and the file wasn't created. Though it won't hurt to put it above.
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.
Yeah true
Includes 2 fixes for the
socket_listener
:This is the critical fix. When writing, I forgot about this step :-(
On shutdown, we kill off the listener & reader goroutines by just shutting down the sockets they're using. But this was causing them to log the result as an error.
No change log entry since the plugin isn't in a released version yet.
Required for all PRs: