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

Prevent deadlock during path reading / results populating #14

Closed
wants to merge 1 commit into from

Conversation

nick-jones
Copy link
Contributor

@nick-jones nick-jones commented May 27, 2020

Deadlock was possible before because workers were spun up prior to
the results channel being read. Using a buffered channel mitigates
this to some extent, but it's still possible to trigger on large
codebases.

Realistically this needs to be reading the results channel in
advance of the workers being spun up. This can end up getting quite
complicated, as this means spinning up and coordinating another
goroutine.

Wait groups are quite handy for this - in particular the error
handling errgroup is ideal. I've adjusted the code to spin up the
necessary goroutines in order. The workers operate on a separate
errgroup - this might seem quirky, but IMO it's the easiest way
deal with the need to close the results channel when the workers
are all done (if they were attached to the main group this would
be a logistical issue.. something would need to understand that
all workers have stopped).

NB I relocated the channels from struct level fields to variables
that are passed around. Happy to change this back, I just feel it's
a little more clear to work with, particularly given channels will
end up being closed (which would previously mean calling
`Impi.Verify()` twice would result in an error).

Resolves #13

Deadlock was possible before because workers were spun up prior to
the results channel being read. Using a buffered channel mitigates
this to some extent, but it's still possible to trigger on large
codebases.

Realistically this needs to be reading the results channel in
advance of the workers being spun up. This can end up getting quite
complicated, as this means spinning up and coordinating another
goroutine.

Wait groups are quite handy for this - in particular the error
handling errgroup is ideal. I've adjusted the code to spin up the
necessary goroutines in order. The workers operate on a separate
errgroup - this might seem quirky, but IMO it's the easiest way
deal with the need to close the results channel when the workers
are all done (if they were attached to the main group this would
be a logistical issue.. something would need to understand that
all workers have stopped).

NB I relocated the channels from struct level fields to variables
that are passed around. Happy to change this back, I just feel it's
a little more clear to work with, particularly given channels will
end up being closed (which would previously mean calling
`Impi.Verify()` twice would result in an error).

Resolves pavius#13
@pavius
Copy link
Owner

pavius commented Jun 3, 2020

Looks great. Will give this a spin when time allows and merge

@nick-jones nick-jones closed this Jan 31, 2022
@nick-jones nick-jones deleted the workers branch January 31, 2022 09:41
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