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

math/rand: document that math.Rand is not safe for concurrent use #3611

Closed
gopherbot opened this issue May 11, 2012 · 7 comments

Comments

Projects
None yet
4 participants
@gopherbot
Copy link

commented May 11, 2012

by gar3ts:

I have a project that as an aside calls rand.Intn() many times with different values. My
project frequently causes rng.Int63() to panic due to an out of range index:

math/rand.(*rngSource).Int63(0xf84006a400, 0xf8412cc2a0, 0xf840050ab0, 0xf84006a400)
        C:/Go/src/pkg/math/rand/rng.go:243 +0x7e
math/rand.(*Rand).Int63(0xf84003d5e0, 0x200027bb78, 0x42e0eb, 0xf84003d5e0)
        C:/Go/src/pkg/math/rand/rand.go:37 +0x46
math/rand.(*Rand).Int31(0xf84003d5e0, 0x414b1b, 0x444369, 0x27bb60)
        C:/Go/src/pkg/math/rand/rand.go:43 +0x28
math/rand.(*Rand).Int31n(0xf84003d5e0, 0x3, 0x27bb60, 0x43c9c1, 0x20, ...)
        C:/Go/src/pkg/math/rand/rand.go:72 +0x79
math/rand.(*Rand).Intn(0xf84003d5e0, 0xf800000003, 0xf84104a5c8, 0xf841e9b160,
0xf84244e300, ...)
        C:/Go/src/pkg/math/rand/rand.go:86 +0x70

Which compiler are you using (5g, 6g, 8g, gccgo)?

go run

Which operating system are you using?

Windows7

Which version are you using?  (run 'go version')

1.0.1 but 1.0 had the same problem

Please provide any additional information below.

it only happens when I'm using multiple procs, i.e.:

runtime.GOMAXPROCS(2)

The calls to rand are made from various of 1000's of goroutine instances.

I instrumented Int63() to capture useful values in a recover() then downloaded MinGW and
compiled Go for amd64. Here's some of the captured state:

seed: 1336714659800372000  
count: 1003446 // number of calls to Int63()
rng.feed: 330
rng.tap: 1212

another:

seed: 1336715957940621400
count: 726597
rng.feed: 333
rng.tap: 1212

another:

seed: 1336716097220587700
count: 478936
rng.feed: 327
rng.tap:  1212


note that len(rng.vec) is 607
rng.fee and rng.tap are used as indexes into that array so for the above examples it is
clear that rng.tap is the problem. What is unclear is how it manages to get out of range.

The following are captures from an index out of range panic but without the values being
out of range somehow:

rng.seed: 1336716288342519300
count: 390314
rng.feed: 337
rng.tap: 604

and:

rng.seed: 1336717800504010000
count: 143252
rng.feed: 334
rng.tap: 606

This looks like a thread safety problem but http://golang.org/pkg/math/rand/ doesn't
mention it. Also, I don't get a panic in my project when I substitute a cryptographic
random number generator.
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented May 11, 2012

Comment 1:

If 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.
So I think this is a documentation issue.

Labels changed: added priority-soon, removed priority-triage.

@robpike

This comment has been minimized.

Copy link
Contributor

commented May 11, 2012

Comment 2:

Labels changed: added documentation.

Status changed to Accepted.

@rsc

This comment has been minimized.

Copy link
Contributor

commented May 15, 2012

Comment 3:

In general the rule is this: top-level functions like strings.Split or
fmt.Printf or rand.Int63 may be called from any goroutine at any time
(otherwise programming with them would be too restrictive), but
objects you create (like a new bytes.Buffer or rand.Rand) must only be
used by one goroutine at a time unless otherwise noted (like
net.Conn's documentation does).
There are not enough stack frames to say for sure, but it sounds like
you are calling Int63 on your own allocated rand.Rand from multiple
goroutines.  That is not promised to work, and it turns out does not.
If you are actually calling the top-level function rand.Int63 and it
is crashing, then that's a bug on our part and we should investigate
further.  Please let us know which it is.
Thanks.

Status changed to WaitingForReply.

@gopherbot

This comment has been minimized.

Copy link
Author

commented May 16, 2012

Comment 4 by gar3ts:

I'm calling rand.Intn(int) from a rand that I created.  Is the best practice to use a
mutex to control access to the rand I create?
Additionally, couldn't the code in rng.go be made goroutine-safe (preventing tap and
feed from jumping off the end of the vector) by doing the decrement and range fix
against a local variable?
    tap := rng.tap -1
    if tap < 0 {
        tap += _LEN
    }
    rng.tap = tap
    feed := rng.feed -1
    if feed < 0 {
        feed += _LEN
    }
    rng.feed = feed
Thanks.
@rsc

This comment has been minimized.

Copy link
Contributor

commented May 16, 2012

Comment 5:

: I'm calling rand.Intn(int) from a rand that I created.  Is the best practice
: to use a mutex to control access to the rand I create?
Yes, using a mutex is the right solution, or call the package's
top-level Intn function.
: Additionally, couldn't the code in rng.go be made goroutine-safe (preventing
: tap and feed from jumping off the end of the vector) by doing the decrement
: and range fix against a local variable?
The rewrite would probably work, but you still have the problem that
simultaneous calls to Intn are returning the same value.  Also, an
optimizing compiler would be allowed to rewrite your new code into the
old code.
Russ
@gopherbot

This comment has been minimized.

Copy link
Author

commented May 16, 2012

Comment 6 by gar3ts:

OK thanks. I'm creating different rands in order to reduce contention for the top-level
rand.
Please consider a revision along the lines I suggested. I'd think different rands
getting the same value occasionally would be preferable to an out-of-range exception -
at least where the compiler doesn't optimize out the temp variables.
@rsc

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2012

Comment 7:

I thought about this, and I think it's okay for buggy code to crash.

Status changed to Unfortunate.

@drewlanenga drewlanenga referenced this issue Dec 15, 2014

Merged

Add rnd mutex #3

smallnest added a commit to smallnest/C1000K-Servers that referenced this issue Jul 15, 2015

tsenart pushed a commit to mesosphere/mesos-dns that referenced this issue Oct 8, 2015

Tomás Senart
resolver: Synchronize access to RNG
Only the global package level rand.Source is safe for concurrent use.
Instantiated ones need their own synchronization mechanisms.
See: golang/go#3611

tsenart pushed a commit to mesosphere/mesos-dns that referenced this issue Oct 8, 2015

Tomás Senart
resolver: Synchronize access to RNG
Only the global package level rand.Source is safe for concurrent use.
Instantiated ones need their own synchronization mechanisms.
See: golang/go#3611

jkodumal added a commit to launchdarkly/go-client that referenced this issue May 14, 2016

@golang golang locked and limited conversation to collaborators Jun 24, 2016

This issue was closed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.