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

Make upstream.Close() wait for shutdown #1015

Merged
merged 1 commit into from
Mar 20, 2018
Merged

Conversation

squaremo
Copy link
Member

@squaremo squaremo commented Mar 20, 2018

We let main() exit, and block on graceful shutdown in a defered
procedure. We also defer a call to upstream.Close(), which stops
the upstream reconnecting and closes it.

The two deferred procedure calls used to be in such an order that by
coincidence, upstream.Close() was also blocked on graceful
shutdown. But this order was shuffled in PR #962.

This commit makes the upstream.Close() explicitly wait for shutdown.

Fixes weaveworks/service-conf#2053.

We let main() exit, and block on graceful shutdown in a `defer`ed
procedure. We _also_ `defer` a call to `upstream.Close()`, which stops
the upstream reconnecting and closes it.

The two deferred procedure calls used to be in such an order that by
coincidence, `upstream.Close()` was _also_ blocked on graceful
shutdown. But this order was shuffled in PR #962.

This commit makes the `upstream.Close()` explicitly wait for shutdown.
Copy link
Contributor

@samb1729 samb1729 left a comment

Choose a reason for hiding this comment

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

I did briefly wonder what'd happen if main exited before upstream.Close() were called. Seems fine to assume the runtime will clean up for us in that case.

LGTM

@squaremo
Copy link
Member Author

I did briefly wonder what'd happen if main exited before upstream.Close() were called. Seems fine to assume the runtime will clean up for us in that case.

Yeah; this is just a last-chance cleanup. (Better would be to use the same shutdown protocol for the upstream; let's save that for a bigger refactor)

@squaremo squaremo merged commit f220230 into master Mar 20, 2018
@squaremo squaremo deleted the issue/no-reconnect branch March 20, 2018 15:50
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.

2 participants