Preventing race condition using the sync.Atomic package in Go-CodePudding

I have the following counter implemented in Go that I would like to use in a concurrently. I'm using the atomic package to store the state, but not sure if I could run into any race conditions. Do I need to add an additional mutex as well in order to protect against incrementing below zero for example or do the atomic operations provide enough safety? Thank you!

type Counter struct {
    counter  uint64
    finished uint32
    sync.Mutex
}

// Inc increments the counter by one
func (c *Counter) Inc() error {
    if c.isFinished() {
        return fmt.Errorf("counter is finished")
    }

    atomic.AddUint64(&c.counter, 1)
    return nil
}

// Dec decrements the counter by one, but prevents the counter from going to zero
func (c *Counter) Dec() {
    // prevent overflow
    if !c.isZero() {
        atomic.AddUint64(&c.counter, ^uint64(0))
    }
}

// Cancel sets the finished flag, and sets counter to zero
func (c *Counter) Cancel() {
    if !c.isFinished() {
        atomic.StoreUint32(&c.finished, 1)
        atomic.StoreUint64(&c.counter, 0)
    }
}

// isFinished returns true if finished
func (c *Counter) isFinished() bool {
    return atomic.LoadUint32(&c.finished) == uint32(1)
}

// isZero returns true if counter is zero
func (c *Counter) isZero() bool {
    return atomic.LoadUint64(&c.counter) == uint64(0)
}

Update: By running the code below with the -race flag, I was able to detect that I indeed need to include mutexes.

var counter *Counter = &Counter{}

func main() {
    wg := sync.WaitGroup{}

    numberOfLoops := 10
    wg.Add(numberOfLoops)

    for i := 0; i < numberOfLoops; i   {
        go incrementor(&wg)
    }

    wg.Wait()
    fmt.Println("Final Counter:", counter.counter)
}

func incrementor(wg *sync.WaitGroup) {
    rand.Seed(time.Now().UnixNano())
    for i := 0; i < 20; i   {
        counter.Inc()
        time.Sleep(time.Duration(rand.Intn(3)) * time.Millisecond)
    }
    wg.Done()
    counter.Cancel()
}

Playground link

CodePudding user response：

to avoid races as in "detectable by the race detector" :

As @caveman answered, the issue you encountered is linked to an issue in wg.Done() / wg.Wait() instructions ordering, and the fact that you don't use an atomic.Load() to access the value of counter.counter.

Your methods are "safe" with regards to this kind of race condition.

to avoid races as in "don't reach an incoherent state for counter" :

you do have an issue, because your methods inspect two distinct fields of your object, and you have no synchronization mechanism to check that the first one isn't changed before applying an operation to the second one.

By changing your incrementor function to :

func incrementor(wg *sync.WaitGroup) {
    for i := 0; i < 20000; i   {
        counter.Inc()
    }
    counter.Cancel()
    wg.Done()
}

and running your program several times, you should be able to see that "Final Counter:" does not always end with 0 (playground).

Here is an illustration on how this can occur :

suppose goroutine 1 executes counter.Cancel() while goroutine 2 executes counter.Inc(), the following sequence of execution may occur :

   goroutine 1                                 goroutine 2

                                            1. if c.isFinished() {
                                                   return fmt.Errorf("counter is finished")
                                               }
2. if !c.isFinished() {                         
3.     atomic.StoreUint32(&c.finished, 1)       
4.     atomic.StoreUint64(&c.counter, 0)        
   }                                            
                                            5. atomic.AddUint64(&c.counter, 1)
                                            6. return nil

the c.isFinished() instruction in .Inc() may happen before .Cancel() gets executed,
and the atomic.AddUint64(&c.counter, 1) may happen after .Cancel() has reset the counter to zero.

To avoid this kind of race, you need to choose a way to synchronize "both" values.

One common way to do that is to use a mutex :

type Counter struct {
    counter  uint64
    finished uint32
    m        sync.Mutex
}

// Inc increments the counter by one
func (c *Counter) Inc() error {
    c.m.Lock()
    defer c.m.Unlock()

    if c.finished != 0 {
        return fmt.Errorf("counter is finished")
    }

    c.counter  
    return nil
}

// Dec decrements the counter by one, but prevents the counter from going to zero
func (c *Counter) Dec() {
    c.m.Lock()
    defer c.m.Unlock()

    // prevent overflow
    if c.counter > 0 {
        c.counter--
    }
}

// Cancel sets the finished flag, and sets counter to zero
func (c *Counter) Cancel() {
    c.m.Lock()
    defer c.m.Unlock()

    if c.finished == 0 {
        c.finished = 1
        c.counter = 0
    }
}

playground

CodePudding user response：

You don't need an additional mutex, your main function is reading counter.counter without using an atomic load, while your incrementor calls wg.Done() before counter.Cancel(), thus you get a race condition.

By moving wg.Done() after counter.Cancel() the race condition is resolved:

func main() {
    wg := sync.WaitGroup{}

    numberOfLoops := 10
    wg.Add(numberOfLoops)

    for i := 0; i < numberOfLoops; i   {
        go incrementor(&wg)
    }

    wg.Wait()
    fmt.Println("Final Counter:", counter.counter)
}

func incrementor(wg *sync.WaitGroup) {
    rand.Seed(time.Now().UnixNano())
    for i := 0; i < 20; i   {
        counter.Inc()
        time.Sleep(time.Duration(rand.Intn(3)) * time.Millisecond)
    }
    counter.Cancel()
    wg.Done()
}