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

Add String method to ContextImpl to fix a race #2879

Merged
merged 2 commits into from
May 21, 2022
Merged

Conversation

dnr
Copy link
Member

@dnr dnr commented May 20, 2022

What changed?
Add a String method (fmt.Stringer) to ContextImpl.

Also simplify contextMatcher implementation and move getOrCreateShardRequestMatcher to the bottom of the file just so the matchers are in one place.

Why?
This is a weird one: in some tests, we use a gomock matcher to differentiate between Contexts in CreateEngine, to return the right value. Internally, gomock evaluates all its matchers, but for the failing ones (and there will always be failing ones if there's more than one), it constructs an error message using Sprintf("%v") and then throws it away when it finds a match. The default "%v" formatter reads private fields of the value being matched. Some of those private fields (e.g. state) are protected by a mutex, but it doesn't know that and reads them anyway. This triggers the race detector. We can just provide a String to bypass the default "%v" behavior.

How did you test it?
ran test with -race -count 1000

@dnr dnr requested a review from a team as a code owner May 20, 2022 20:18
@dnr dnr merged commit b530a35 into temporalio:master May 21, 2022
@dnr dnr deleted the fixmockrace branch May 21, 2022 00:10
Sushisource pushed a commit to Sushisource/temporal that referenced this pull request Jun 7, 2022
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.

None yet

2 participants