-
Notifications
You must be signed in to change notification settings - Fork 78
Fix data races #1
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
Conversation
As an outsider to the project, I am a bit curious to why a data race matters in a pseudo-random data genrator API... Supposedly it only makes the data more random. However, in #3 I propse allowing the API user to set a custom rand.Source -- my use case was to set a custom seed, but I belive your use case (providing a thread-safe rand.Source implementation) could be covered by the same function. Presumably it's marginally faster to leave the default rand.Source to be non-thread safe, and some users might prefer that. @jackc would you agree? |
I typically run tests with t.Parallel. So multiple tests can be running and requesting fake data concurrently. I often run my tests with the race detector. At the very least this would cause errors to be detected. But race conditions can never be considered safe, so other undefined results may occur. A custom rand source would solve one of the race conditions, but it would not solve the race for accessing the sample cache. |
Yeah, I guess you are right @jackc. And I read that golang do locking on all their root level methods, including rand. quote: " Go source: https://golang.org/src/math/rand/rand.go?s=5017:5032#L163 |
Is this not being merged into the master branch for some reason? |
@Adron, from the commit / PR history, this repo does not appear to be maintained. You might want to fork it if you want to use it. |
@smyrman good point. I looked and realized that after I'd left the comment that it's been a long while. Gonna fork like you mention and get things going. |
I'm running into the same issue. For now I manually updated it with the patch above and it works fine for me. |
Merged it with rebase, thank you! |
fake was not safe to call concurrently from multiple Go routines. This pull request contains two fixes.