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: make EventuallyWithT concurrency safe #1395
Conversation
4ce8b36
to
7767496
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.
I was thinking whether this can be fixed by enforcing that condition funcs can never overlap but the documentation is clear that it is checked every tick, so that would be a breaking change. The test relies on the race detector, I don't see a better way to do it.
7767496
to
c790f3f
Compare
So while this effectively serializes calls to CollectT methods (#1418), I'm not convinced this really fixes the core issue. This implementation doesn't fix the real problem which that we may have multiple instances of the I think we will merge this (with |
b93dd46
to
596062c
Compare
@dolmen Thanks for the comment, you're right! I've taken a look into that again and came up with a solution that doesn't involve a mutex. It also doesn't allow a situation where we end up with an empty |
596062c
to
41b483d
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.
This looks very good!
This is the solution I want!
Also it will allow us to get rid of CollectT
which was a design error of the original implementation (a separate work for which I am studying plans since a week).
As the |
@czeslavo Note for the future, that completely replacing your original implementation on which at least 2 other people worked too (reviewers) with a completely different one, is not a good practice. It would have been better to open a separate PR and close this one. Anyway, in this case, the original implementation can be forgotten. |
5178d6f
to
81a641d
Compare
"address review comments" is never a good commit message. It is completely useless when looking at the Git log or with "git blame". A good commit message must give a summary of what changes, and, if necessary, why. A commit that ONLY says the why is never good. In the present case squashing the commits is the best thing to do. |
Summary
In
assert.EventuallyWithT
,assert.CollectT
is used in a concurrent context which may lead to races as explained in #1391.Changes
We create a
CollectT
per every condition tick and send its errors over a channel. On the reader side of the channel, we cache thelastTickErrs
so they can be copied over to t once the timeout is fired. The only access tolastTickErrs
happens in the reader case and the timeout case which both run in the same goroutine.Motivation
We cannot use
EventuallyWithT
family of functions/methods in our codebase due to data races it causes.Related issues
Closes #1391.