pkg/pwalk: fix data race with err #109
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Go race detector found a race in the code wrt err read/write: > $ cd pkg/pwalk && go test -run Many -race . > === RUN TestWalkManyErrors > ================== > WARNING: DATA RACE > Read at 0x00c0000a0510 by goroutine 7: > github.com/opencontainers/selinux/pkg/pwalk.WalkN() > /home/kir/go/src/github.com/opencontainers/selinux/pkg/pwalk/pwalk.go:91 +0x1f8 > github.com/opencontainers/selinux/pkg/pwalk.Walk() > /home/kir/go/src/github.com/opencontainers/selinux/pkg/pwalk/pwalk.go:35 +0x321 > ... > Previous write at 0x00c0000a0510 by goroutine 8: > github.com/opencontainers/selinux/pkg/pwalk.WalkN.func1() > /home/kir/go/src/github.com/opencontainers/selinux/pkg/pwalk/pwalk.go:53 +0xa6 Indeed, we wait for all runner goroutines but do not wait for the filetree reading goroutine (the one that calls filepath.Walk). One way to fix it would be to not assign value to global variable err in the filetree reading goroutine, but do that in a runner. Unfortunately, that way we might mix up and not return the first error. Fix by using waitgroup for the reading goroutine as well. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
|
LGTM |
|
lgtm and verified locally |
|
@thaJeztah you have to add the *which maybe we should ditch as we did in runc, opencontainers/runc#2388 |
|
ooooh, so it doesn't look at "reviews", only "comments"? |
|
LGTM |
|
oh, wow, that's "silly" |
|
Looks like there's support for github reviews in the v2 syntax that we're using (https://v2-docs.pullapprove.com/groups/github_reviews/), but v3 is the current version of the config format (not sure if that allows approval through comments), but yes, I think GitHub's native reviews should work fine for this |
except for people who use email to LGTM (I'm not sure we have anyone doing that in here) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #108
Go race detector found a race in the code wrt err read/write:
Indeed, we wait for all runner goroutines but do not wait for the
filetree reading goroutine (the one that calls filepath.Walk).
One way to fix it would be to not assign value to global variable err in
the filetree reading goroutine, but do that in a runner. Unfortunately,
that way we might mix up and not return the first error.
Fix by using waitgroup for the reading goroutine as well. Slightly
more complex than not assiging err in there, but guarantee we'll
return the first error.