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

proposal: math/rand: Expose constructor for `lockedSource` #24121

Open
tpudlik opened this issue Feb 25, 2018 · 10 comments

Comments

@tpudlik
Copy link

commented Feb 25, 2018

The math/rand library defines the Source interface to represent a source of pseudorandom numbers, and a constructor returning a concrete implementation, NewSource. However, the implementation returned by NewSource is not safe for concurrent use by multiple goroutines. The package also defines a thread-safe lockedSource implementation, but this implementation is not exposed.

The proposal is to expose a constructor for the lockedSource by adding the following function to math/rand:

// NewLockedSource returns a new pseudo-random Source seeded with the given value
// that is safe for concurrent use by multiple goroutines.
NewLockedSource(seed int64) Source

Benefits

Today, users who need a thread-safe implementation must duplicate the lockedSource implementation in their own code. Apparently over 1,000 GitHub projects have done so (search). Exposing the standard library implementation would avoid this duplicate code.

Why do users need a thread-safe source? One reason is that such a source is necessary when implementing any object that (1) exposes thread-safe methods which use random numbers in their implementation and (2) supports dependency injection.

Incidentally, the proposed rand package for Go 2 (#21835) goes even further than this proposal and makes LockedSource a public type.

References

This proposal was originally raised in 2015 on golang-dev, but it appears not to have been resolved. The lack of a constructor for a lockedSource was raised as issue #21393.

@gopherbot gopherbot added this to the Proposal milestone Feb 25, 2018

@gopherbot gopherbot added the Proposal label Feb 25, 2018

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2018

Isn't this basically the same as #21393? I guess the difference is that you have a specific proposal?

@tpudlik

This comment has been minimized.

Copy link
Author

commented Feb 26, 2018

Right, this is basically a proposal that would fix #21393 if given the go-ahead.

@josharian

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2018

I think that we should consider all Go 2 math/rand proposals together to get a coherent picture. The home for that should probably be #21835. I think we should backlink here from #21835 and close this.

If we do something like this, I think a better API would be:

func NewLockedSource(src Source) Source

Then it could be usefully used with any input source.

@tpudlik

This comment has been minimized.

Copy link
Author

commented Feb 26, 2018

One issue with considering all the math/rand proposals together is that some of them (like this one) are fully backwards compatible, while others (like switching to PCG) are not, and so have to wait for Go 2.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2018

On hold for eventual consideration as part of "what do we do about math/rand", as @josharian said.

For now you can always put a lock around a source yourself.

@FiloSottile

This comment has been minimized.

Copy link
Member

commented Apr 25, 2018

@ChimeraCoder makes an articulate argument in #25057 for how exposing a locked source could lead to less contention issues by encouraging developers not to use the global one.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2018

A locked source is literally a source plus a lock. It's not difficult. There's no need for this in the rand API. We don't published locked other types.

@ChimeraCoder

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2018

It's not a matter of whether it's difficult or not, but what the defaults encourage. At present, math/rand is more convenient than crypto/rand, which means that the set of available choices encourage people to choose a default that is neither cryptographically secure nor performant at scale.

And because there's neither a constructor function in the standard library for a locked source nor an interface definition, there isn't really a way for different libraries to standardize around a common interface for requiring a threadsafe source even if they want to. Either one by itself would suffice, but without either one, people reach for the most convenient default, which has led to the problems we're currently observing:

a) a lot of packages duplicate the lockedSource implementation verbatim, which is a sign that people would find it valuable if it were exported

b) too many packages rely on the default Source

where I'm using "too many" in this context to mean "enough to provide noticeable mutex contention for applications which use these packages, even under modest workloads".

@josharian

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2018

A locked source is literally a source plus a lock. It's not difficult. There's no need for this in the rand API.

As I wrote over in #21393, there may be some subtleties and/or optimizations that just throwing a mutex at won't help with, like batching core PRNG calls for rand.Read. But yes, it doesn't seem like a high priority.

josharian added a commit to josharian/go that referenced this issue May 8, 2018
math/rand: reduce global mutex contention for default source
* By using a sync.Pool, we can preserve the current behavior for
the single-threaded, serialized case (seeded with 1), but also
allow concurrent callers the ability to generate random numbers
with the default source without blocking on the same global mutex

golang#24121 (comment)

Change-Id: Icf10fdf10f96775ea64bc52202c41bf023d299de
@orian

This comment has been minimized.

Copy link

commented Dec 17, 2018

A rand.Rand having hardcoded lockedSource type assertion just to implement default methods on package seems like interesting design choice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.