Skip to content

Remove unneeded stopping of nil Dispatcher#2897

Merged
roidelapluie merged 1 commit intomainfrom
remove-noop-disp-stop
May 3, 2022
Merged

Remove unneeded stopping of nil Dispatcher#2897
roidelapluie merged 1 commit intomainfrom
remove-noop-disp-stop

Conversation

@juliusv
Copy link
Member

@juliusv juliusv commented Apr 29, 2022

The function value and parameters of a defer statement are immediately
evaluated, so this "disp" value is always nil, and calling Stop() on a nil
dispatcher is a no-op, so this does nothing, but is confusing.

Signed-off-by: Julius Volz julius.volz@gmail.com

Copy link
Member

@gotjosh gotjosh left a comment

Choose a reason for hiding this comment

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

LGTM.

Took a while to review this one, I wasn't sure I understood why didn't we need to move this one but after a few rounds of testing and a re-read, this is safe to remove.

I think that we need to add is a way to explicitly stop all components similarly, as an example both silence and nflog maintenance are governed by the stopc but mem.NewAlerts nor Dispatcher aren't - would good to make sure all components are cleanly shutting down as we exit.

Let me know if you'd like to create a follow issue+PR to make sure we don't forget as I think this can be separate from this PR.

@juliusv
Copy link
Member Author

juliusv commented May 2, 2022

This PR just resulted from brief tourism into the AM codebase for the purpose of refreshing my memory about it :) I filed an issue (#2906), but can't promise I'll get to it myself.

Now actually looking at this PR here again, I noticed that indeed, the Dispatcher is also not shut down any other way when main.go exits (only upon config reload), so the right short-term fix is probably not to remove this ineffectual line, but to fix it by wrapping the disp.Stop() in a deferred closure. Look again :)

The function value and parameters of a defer statement are immediately
evaluated, so this "disp" value is always nil, and calling Stop() on a nil
dispatcher is a no-op, so this does nothing, but wrapping it in a closure
that refers to "disp" fixes it.

Signed-off-by: Julius Volz <julius.volz@gmail.com>
@juliusv juliusv force-pushed the remove-noop-disp-stop branch from 71adb5a to 649fdee Compare May 2, 2022 20:43
@gotjosh
Copy link
Member

gotjosh commented May 3, 2022

but can't promise I'll get to it myself.

I'm happy to take it for a spin.

disp.Stop() in a deferred closure

That also works 😄

Copy link
Member

@gotjosh gotjosh left a comment

Choose a reason for hiding this comment

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

LGTM

@roidelapluie roidelapluie merged commit a38c5b8 into main May 3, 2022
@roidelapluie roidelapluie deleted the remove-noop-disp-stop branch May 3, 2022 09:01
@roidelapluie
Copy link
Member

Thanks!

qinxx108 pushed a commit to qinxx108/alertmanager that referenced this pull request Dec 13, 2022
The function value and parameters of a defer statement are immediately
evaluated, so this "disp" value is always nil, and calling Stop() on a nil
dispatcher is a no-op, so this does nothing, but wrapping it in a closure
that refers to "disp" fixes it.

Signed-off-by: Julius Volz <julius.volz@gmail.com>
Signed-off-by: Yijie Qin <qinyijie@amazon.com>
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