Skip to content
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

Fixed a race condition when shutting down Agg #5

Merged
merged 1 commit into from
Feb 21, 2019

Conversation

wblakecaldwell
Copy link
Contributor

When the Agg is stopped, the done channel is written to, which signals
its aggregate() goroutine loop to exit. This is fine, except this done
channel is also returned to the caller via Done(). The intended flow
is for aggregate() to catch the done channel write, and then wrap
up, close the output channel that the caller is reading from, and write
again to the done channel for the caller. However, if the caller
catches the first message before Agg does, then Agg never shuts down.

The fix here is to have two channels - one to signal that we're shutting
down (quitting), and one to signal that we've shut down (done). The
first one is private, just for communication with its own goroutines.
The second is exposed to the caller. We're also now closing channels,
rather than writing to them, for good measure, since we're only using
them as quit channels.

When the Agg is stopped, the `done` channel is written to, which signals
its `aggregate()` goroutine loop to exit. This is fine, except this `done`
channel is also returned to the caller via `Done()`. The intended flow
is for `aggregate()` to catch the `done` channel write, and then wrap
up, close the output channel that the caller is reading from, and write
again to the `done` channel for the caller. However, if the caller
catches the first message before Agg does, then Agg never shuts down.

The fix here is to have two channels - one to signal that we're shutting
down (`quitting`), and one to signal that we've shut down (`done`). The
first one is private, just for communication with its own goroutines.
The second is exposed to the caller. We're also now closing channels,
rather than writing to them, for good measure, since we're only using
them as quit channels.
@wg
Copy link
Contributor

wg commented Feb 21, 2019

LGTM

@wblakecaldwell
Copy link
Contributor Author

thanks

@wblakecaldwell wblakecaldwell merged commit a3940a0 into master Feb 21, 2019
@wblakecaldwell wblakecaldwell deleted the wbc-stop-fix branch March 23, 2019 00:19
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.

2 participants