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

Closed
tpudlik opened this issue Feb 25, 2018 · 13 comments
Closed

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

tpudlik opened this issue Feb 25, 2018 · 13 comments
Labels
Milestone

Comments

@tpudlik
Copy link

tpudlik 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
@ianlancetaylor
Copy link
Contributor

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

@tpudlik
Copy link
Author

tpudlik commented Feb 26, 2018

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

@josharian
Copy link
Contributor

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
Copy link
Author

tpudlik 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
Copy link
Contributor

rsc 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
Copy link
Contributor

@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
Copy link
Contributor

rsc 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
Copy link
Contributor

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
Copy link
Contributor

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 pushed a commit to josharian/go that referenced this issue May 8, 2018
* 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
Copy link

orian commented Dec 17, 2018

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

@valderrama
Copy link

valderrama commented Apr 24, 2020

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.

I think this is more serious than just subtleties. Anyone who copies the lockedSource implementation introduces the same race conditions fixed here and here:

This demonstration in the go playground shows that a locked source copied from the standard library can interleave Rand.Seed() and Rand.Read() resulting in duplicate generated sets of bytes: https://play.golang.org/p/2PrUkJUSw-M

As @orian pointed out:

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

IMO this choice prevents anyone from creating a completely thread safe rand.Source implementation. Instead developers must combine a lock with a rand.Rand similar to this:

type LockedRand struct {
	lk  sync.Mutex
	*rand.Rand
}

func (r *LockedRand) Seed(seed int64) {
	r.lk.Lock()
	r.Rand.Seed(seed)
	r.lk.Unlock()
}

func (r *LockedRand) Read(p []byte) (n int, err error) {
	r.lk.Lock()
	n, err = r.Rand.Read(p)
	r.lk.Unlock()
	return n, err
}

So solving this is clearly not just a rand.Source and a lock, and it is also not at all evident as seen by the fact that many people on Github and even Amazon AWS get it wrong: https://github.com/aws/aws-sdk-go/blob/v1.30.13/internal/sdkrand/locked_source.go#L28-L29

However, FWIW there are comments on both rand.Read and rand.Seed claiming they cannot be used concurrently so perhaps users should start creating locked rand.Rand types instead of locked rand.Seed types.

@rsc
Copy link
Contributor

rsc commented Jun 7, 2023

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc removed the Proposal-Hold label Jun 7, 2023
@rsc
Copy link
Contributor

rsc commented Jun 14, 2023

No change in consensus, so declined.
— rsc for the proposal review group

@rsc rsc closed this as completed Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Declined
Development

No branches or pull requests

9 participants