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

hubble: Fix data races in pkg/hubble.TestRingReader_NextFollow_WithEmptyRing #17397

Merged
merged 2 commits into from Sep 15, 2021

Conversation

gandro
Copy link
Member

@gandro gandro commented Sep 14, 2021

This fixes two unit test data races reported in #17255 - see commit messages for details. Both races were easy to reproduce locally via go test -race -count 10 . inside pkg/hubble/container. I have verified that the races are fixed by running go test -race -count 1000 .

Fixes: #17255

The NextFollow method writes to `r.followChan` variable concurrently
from a `go func() {..}` statement when the context is cancelled. That
write may be executed concurrently to the other accesses in NextFollow.
This commit fixes that issue by protecting the access with a mutex.

Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
The ring reader unit tests rely on wait groups to ensure all go routines
spawned by `NextFollow` have exited. However, correct use requires us to
ensure that there are no concurrent spawns while we wait, i.e. it is an
error to invoke `Close` concurrently to `NextFollow`, one must wait for
`NextFollow` to return and then wait for any still lingering Go routines
to finish. See the documentation of WaitGroup.Add for details:
https://pkg.go.dev/sync#WaitGroup.Add

This fixes an issue reported by the Go race detector by ensuring
NextFollow has exited before we wait on any lingering go routines.

Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
@gandro gandro added release-note/misc This PR makes changes that have no direct user impact. sig/hubble Impacts hubble server or relay kind/bug/race-detector labels Sep 14, 2021
@gandro gandro requested review from rolinh and a team September 14, 2021 17:42
Copy link
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for the fixes!

@gandro
Copy link
Member Author

gandro commented Sep 15, 2021

test-me-please

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 15, 2021
@aanm aanm merged commit a147585 into cilium:master Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug/race-detector ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/hubble Impacts hubble server or relay
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Race Detector] data race in pkg/hubble.TestRingReader_NextFollow_WithEmptyRing
3 participants