Preventing race condition using the sync.Atomic package in Go

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

• 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 run several successive instructions to inspect and update the object (e.g : if condition { update_fields }), and you have no synchronization mechanism to check that the condition is still true when you apply update_fields.

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 of 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 the inspect + update instructions.

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

(*) [edit] I initially wrote : "because your methods inspect two distinct fields of your object", but you could have a similar issue even with one single field.

For example, if two goroutines run the following code :

if c.LoadUint64(&c.counter) == 0 {
   atomic.AddUint64(&c.counter, 1)
}

you may have an end value of 2.

The issue lies with having several instructions, which aren't executed in an atomic way.

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()
}