Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Run signal listener in a goroutine instead of deferring. #1680

Merged
merged 2 commits into from
Feb 13, 2019

Conversation

captncraig
Copy link
Contributor

Fixes #1679

@squaremo
Copy link
Member

The existing code is probably ported from fluxd, where it is deferred so that it can appear in the context of setting up the shutdown machinery, but run last -- the idea is to grant the various other goroutines, told to shut down by closing the channel, a chance to gracefully do so before exiting.

The reason is doesn't work in helm-operator is that the last thing to happen in main() is synchronous, so the deferred procedure is not reached in the usual course of operation. Running it in a goroutine means it will be reached -- but waiting on the waitgroup in a goroutine will not have the effect of allowing the other tasks to complete.

Since opr is given a shutdown channel and waitgroup, I think you could run it in a goroutine, and keep the deferred procedure to handle the graceful shutdown. Or, if you prefer, inline that bit at the end of main.

@2opremio
Copy link
Contributor

2opremio commented Feb 1, 2019

@captncraig Are you interested in finishing up the PR following @squaremo 's advice?

@captncraig
Copy link
Contributor Author

Sorry, I got sidetracked and lost my dev environment on this one. It would be best if someone else finish this up, or just close it. My bad.

@squaremo
Copy link
Member

squaremo commented Feb 7, 2019

Sorry, I got sidetracked and lost my dev environment

No worries -- spotting the problem and suggesting a plausible solution gets us most of the way there, so thank you 👍

The general scheme in main.go is to start a bunch of goroutines,
giving them a shutdown channel; then wait until there's a reason to
exit, close the shutdown channel, and wait until the goroutines exit.

This wasn't working, because the last stanza in main.go was _not_ run
in a goroutine. This commit fixes that, and moves the graceful
shutdown bit to the end, for clarity.
@squaremo squaremo merged commit b9ec3e7 into fluxcd:master Feb 13, 2019
@squaremo
Copy link
Member

Thanks @2opremio 🍍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants