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: export functions for creating locked sources #25057

Closed
ChimeraCoder opened this issue Apr 24, 2018 · 2 comments
Closed

proposal: math/rand: export functions for creating locked sources #25057

ChimeraCoder opened this issue Apr 24, 2018 · 2 comments

Comments

@ChimeraCoder
Copy link
Contributor

@ChimeraCoder ChimeraCoder commented Apr 24, 2018

The default Source provided by math/rand is safe for concurrent use by multiple goroutines, but Sources that are created with NewSource are not. Sharing a Source between concurrent goroutines can cause a runtime panic. In itself, this isn't a problem, because a given library has three options:

  1. Use the default Source
  2. Create a new Source and a separate structure (e.g. mutex) to manage locking and prevent concurrent use
  3. Create a new Source with logic to ensure that it cannot be shared between goroutines (such as a sync.Pool).

In practice, most libraries seem to choose option 1, because it's by far the most convenient - it's much easier to call rand.Int() and not think about the tradeoffs than it is to implement options 2 or 3.

Unfortunately, because option 1 is so convenient, and because so many libraries default to the default, applications which use multiple libraries that import math/rand are likely to run into contention around the internal *lockedSource. Third-party dependencies which appear to be completely independent and disconnected are all contending around the same mutex from the standard library.

It's easy to spot this sort of contention with pprof if you know to look for it (which is not always a given). However, it's actually not particularly straightforward to fix. If a library relies on the default source, it cannot also easily provide a way for callers to specify a different source, since the top-level functions don't expose the default source in any way (rand.Seed does not provide sufficient functionality here). In addition, the function signatures don't match, so it's cumbersome for libraries to write functions which can readily use either the thread-safe default source or a non-default source.

For callers who are running into contention problems, it's not easy to retrofit this functionality into existing libraries either. Even if I'm willing to fork one of my dependencies to rely on a non-default source, the lack of either a common interface type or common functions for generating thread-safe sources means it's not a straightforward find-and-replace.

As an example of what this retrofitting would look like, I've switched our top-level application here to use a non-default Source, but with a sync.Pool for thread-safety. The performance gains are significant, but it'd be more painful to apply this change to the other dependencies in the project which still use the default source:
stripe/veneur#466

If instead we had common functions in the standard library for initializing thread-safe RNGs, it would be easier to establish a community practice of expecting libraries to allow callers to specify the Source, while also guaranteeing that the Source is in fact still safe for concurrent usage.

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Apr 24, 2018

@ChimeraCoder Is it fair to say this is a duplicate of #24121?

@FiloSottile FiloSottile changed the title math/rand: export functions for creating locked sources proposal: math/rand: export functions for creating locked sources Apr 24, 2018
@gopherbot gopherbot added this to the Proposal milestone Apr 24, 2018
@gopherbot gopherbot added the Proposal label Apr 24, 2018
@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Apr 25, 2018

Duplicate of #24121

@FiloSottile FiloSottile marked this as a duplicate of #24121 Apr 25, 2018
@golang golang locked and limited conversation to collaborators Apr 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.