-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
fix data race in dynamic plugin registry tests #12554
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving because even if my comments are valid I don't need to re-review after any fixes.
Thanks for doing this!
for { | ||
select { | ||
case <-ctx.Done(): | ||
t.Fail() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps use t.Errorf
here? It's still safe for concurrent use (unlike Fatalf and FailNow) but would allow you to output a hint as to what went wrong.
At a glance I'm not sure what a failure here means other than "an unexpected thing happened," but I'm also not familiar with these tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. The original tests were just "we didn't finish" and that's not very helpful to repeat when fixing the test 😁
rcv1 = true | ||
if rcv1 { | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rcv1
seems like a noop. Can this be replaced with return
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, yup. Copypasta from the previous test. Will fix.
These tests have a data race where the test assertion is reading a value that's being set in the `listenFunc` goroutines that are subscribing to registry update events. Move the assertion into the subscribing goroutine to remove the race. This bug was discovered in #12098 but does not impact production Nomad code.
1bb0958
to
ae1b029
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! (and sorry I missed this 😞 )
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
These tests have a data race where the test assertion is reading a
value that's being set in the
listenFunc
goroutines that aresubscribing to registry update events. Move the assertion into the
subscribing goroutine to remove the race. This bug was discovered
in #12098 but does not impact production Nomad code.