-
Notifications
You must be signed in to change notification settings - Fork 5
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
Data race? #21
Comments
I could see this being an intentional performance thing if setting a boolean field compiles down to a single instruction 🤔 but it makes using this library in any codebase that runs tests with the race detector enabled impossible |
@richardartoul Thank you for reporting. I've researched one thing that can cause this.
I think that there are two possible solutions here
Hot FixesThis is a critical bug, I'll handle it with a hotfix and conduct further researchs for this. |
I also want to hear @richardartoul opinions about this. |
Your hot fix looks good to me, but I don’t understand the two points you made above. The issue is just that there are two concurrent goroutines loading the same cache value at the same time and they both end up modifying the visited field because they’re not holding an exclusive lock. another alternative would be to add a sub-lock inside the entry struct and acquire that before marking visited as true. For now though I’d probably merge your existing hot fix since the more complex solutions probably require detailed concurrent benchmarking. |
My test suite caught this:
Looks like concurrent writes while holding an RLock?
The text was updated successfully, but these errors were encountered: