Data race in big.Rat

Question

I used math/big.Rat to represent numbers for accuracy. Denom() returns the denominator of number and Cmp() to compare two numbers. Both of them seems to be a pure read only functions. But when I ran my code with data race enabled, my whole assumption went wrong. When these functions are called concurrently with the same Rat instance, system throws data race scenario. Are these functions not read-only?

my test case

package main

import (
    "math/big"
    "sync"
)

func main() {
    x := big.NewRat(5, 1)
    wg := new(sync.WaitGroup)

    // just for testing
    for i := 0; i < 10; i++ {
        go func() {
            wg.Add(1)
            defer wg.Done()
            if i%2 == 0 {
                x.Cmp(x)
            } else {
                x.Denom()
            }
        }()
    }
    wg.Wait()
}

When I check the source, every time Denom() function is called, it resets value in the same object. Is this an issue in the source? or I shouldn't take use Rat Denom() and Cmp() concurrently.

Denom() source from Golang for ref.

// Denom returns the denominator of x; it is always > 0.
   400  // The result is a reference to x's denominator; it
   401  // may change if a new value is assigned to x, and vice versa.
   402  func (x *Rat) Denom() *Int {
   403      x.b.neg = false // the result is always >= 0
   404      if len(x.b.abs) == 0 {
   405          x.b.abs = x.b.abs.set(natOne) // materialize denominator
   406      }
   407      return &x.b
   408  }

I am adding few more points based on the discussions below and I accept that I made a mistake in using the varibale 'i' for the intended purpose (But still it will work to show the Data race scenario).

My point here is the operation performed in the Denom() won't have modification in the value represented by Rat. This can be performed upon creation of Rat to represent a value or a new value is set in the Rat. My concern is the repeated computaion (not concurrent safe) of same value again and again unless the value represented by the Rat is changed. Then why can't this be done at the creation/modifcation part?

Answer 1

You have a clear race condition, in short a race condition is when more than two asynchronous routines (threads, process, co-routines. etc.) are trying to access(write or read) a resource(memory or i/o devices) and at least one of those routines has intentions to write.

In your case you're creating goroutines that are accessing a shared resource (var x Rat) and as you may note the method Denom() modifies its own value in line 403 and 405 and it seems that the Cmp() method is only reading. All we have to do is protect the memory with a RWMutex:

package main 

import (
    "math/big" 
    "sync"
)

func main() {
    x := big.NewRat(5, 1)
    wg := new(sync.WaitGroup)
    mutex := new(sync.RWMutex)

    wg.Add(10) // all goroutines

    for i := 0; i < 10; i++ {

        go func(index int) {
            defer wg.Done()

            if index%2 == 0 {

                mutex.RLock() // locks only for reading
                x.Cmp(x)
                mutex.RUnlock() // unlocks for reading

            } else {

                mutex.Lock() // locks for writing
                x.Denom()
                mutex.Unlock() // unlock for writing

            }
        }(i)
    }

    wg.Wait()
}

Note that we're using RLock and RUnlock for reading operation and Lock and Unlock() for write. Also if you know the number of goroutines to be created I always recommend do the wg.Add(n) in just one line because if you do after wg.Add(1) the go func(){...} you'll be in troubles.

And indeed you have a common mistake of using a for index inside a goroutine, always pass them as parameters.

Finally I recommend you to use the -race flag to go run or go build commands, like:

go run -race rat.go

In fact you will see the difference between your code and my solution just using -race

Answer 2

Rat.Denom appears not to be thread safe, as @JimB pointed out in the comment. The general rule with the standard library is that methods are not thread safe unless explicitly noted.

Your other issue is a common pitfall with closure loops.

See this article for a description of the issue. You must pass the variable into the closure, as seen below.

Working example:

package main

import (
    "math/big"
    "sync"
    "fmt"
)

func main() {
    x := big.NewRat(5, 1)
    wg := new(sync.WaitGroup)

    // just for testing
    for i := 0; i < 10; i++ {
    wg.Add(1)
        go func(i int) {
            fmt.Println("x: ", x)
            defer wg.Done()
            if i%2 == 0 {
                fmt.Printf("i: %d, x.Cmp(x): %+v", i, x.Cmp(x))
            } else {
                fmt.Println("x.Denom(): ", x.Denom())
            }
        }(i)
    }
    wg.Wait()
}

See: https://play.golang.org/p/aKo3gHuSeT for a live example.