search

icrowley / fake

Fake data generator for Go (Golang)
MIT License
591 stars 77 forks source link

Fix data races #1

Closed jackc closed 7 years ago

jackc commented 8 years ago

fake was not safe to call concurrently from multiple Go routines. This pull request contains two fixes.

  1. Use concurrency safe random number generator
  2. Use mutex for lookup
smyrman commented 8 years ago

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 https://github.com/icrowley/fake/issues/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?

jackc commented 8 years ago

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.

smyrman commented 8 years ago

Yeah, I guess you are right @jackc. And I read that golang do locking on all their root level methods, including rand.

quote: " f you use your own Rand object, you must provide your own locking.The global Rand object used by Rand.Int31, et. al., does do locking itself. " Sources: -https://code.google.com/p/go/issues/detail?id=3611

Go source: https://golang.org/src/math/rand/rand.go?s=5017:5032#L163

Adron commented 7 years ago

Is this not being merged into the master branch for some reason?

smyrman commented 7 years ago

@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.

Adron commented 7 years ago

@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.

kazamatzuri commented 7 years ago

I'm running into the same issue. For now I manually updated it with the patch above and it works fine for me.

corpix commented 7 years ago

Merged it with rebase, thank you!