1

I had the following code as part of a test:

    expected := 10
    var wg sync.WaitGroup
    for i := 0; i < expected; i++ {
        go func(wg *sync.WaitGroup) {
            wg.Add(1)
            defer wg.Done()
            // do something
        }(&wg)
    }
    wg.Wait()

To my surprise, I got panic: Fail in goroutine after TestReadWrite has completed when running "go test". When running with "go test -race", I did not get a panic, but the test failed at a later point. In both cases, despite having a wg.Wait(), a goroutine did not finish executing.

I made the following change, and now the test works as expected:

    expected := 10
    var wg sync.WaitGroup
    wg.Add(expected)
    for i := 0; i < expected; i++ {
        go func(wg *sync.WaitGroup) {
            defer wg.Done()
            // do something
        }(&wg)
    }
    wg.Wait()

My doubts are:

  1. A lot of the code I have seen so far does wg.Add(1) inside the goroutine. Why does it behave unexpectedly in this specific case? What seems to be happening here is that some goroutines seem to finish running, and get past the wg.Wait(), before other goroutines even start to run. Is using wg.Add(1) inside the goroutine dangerous / to be avoided? If this is not a problem in general, what exactly is causing the problem here?
  2. Is adding wg.Add(expected) the correct way to address this problem?
learnlearnlearn
  • 946
  • 1
  • 8
  • 16
  • Your second approach is perfectly fine. However you can keep using `wg.Add(1)` if you move it just above the goroutine, i.e. keep it inside the loop but outside of the goroutine. – mkopriva Jul 22 '21 at 04:49
  • 1
    *A lot of the code I have seen so far does `wg.Add(1)` inside the goroutine* - it sounds like you are seeing a lot of bad code: if you do this inside the same goroutine that was just added, it's often too late. (It can be OK *provided* there's some reason that this goroutine *must* be started and run to the point where the `Add` finishes, before anyone outside this goroutine calls `wg.Wait`.) – torek Jul 22 '21 at 05:07
  • 1
    Thanks -- I just re-read the code, and noticed that it was my mistake-- all the samples indeed did do the wg.Add outside the goroutine. – learnlearnlearn Jul 22 '21 at 05:10

2 Answers2

3

According to the docs -

A WaitGroup waits for a collection of goroutines to finish. The main goroutine calls Add to set the number of goroutines to wait for. Then each of the goroutines runs and calls Done when finished. At the same time, Wait can be used to block until all goroutines have finished.

So Add() must be called by a goroutine which is initiating other goroutines which in your case is the main goroutine.

In the first code snippet, you are calling Add() inside other goroutines instead of the main goroutine which is causing problem -

expected := 10
var wg sync.WaitGroup
for i := 0; i < expected; i++ {
   go func(wg *sync.WaitGroup) {
       wg.Add(1) // Do not call Add() here
       defer wg.Done()
       // do something
   }(&wg)
}
wg.Wait()

The second snippet is working because you are calling Add() in the main goroutine -

expected := 10
var wg sync.WaitGroup
wg.Add(expected) // Okay
for i := 0; i < expected; i++ {
    go func(wg *sync.WaitGroup) {
       defer wg.Done()
       // do something
     }(&wg)
}
wg.Wait()

Is adding wg.Add(expected) the correct way to address this problem?

You can also call wg.Add(1) in the for loop -

expected := 10
var wg sync.WaitGroup
for i := 0; i < expected; i++ {
    wg.Add(1) // Okay
    go func(wg *sync.WaitGroup) {
       defer wg.Done()
       // do something
     }(&wg)
}
wg.Wait()
cod3rboy
  • 717
  • 7
  • 9
1

Your first approach panics coz (WaitGroup.Add):

Add adds delta, which may be negative, to the WaitGroup counter. If the counter becomes zero, all goroutines blocked on Wait are released. If the counter goes negative, Add panics.

...

...

Typically this means the calls to Add should execute before the statement creating the goroutine or other event to be waited for

When Wait() is called at the end of your code none of the goroutines may have started executing yet - hence the value held in WaitGroup is 0. Then later when your go-routine executes the calling go-routine has already been released. This will lead to unexpected behaviour, a panic in your case. Maybe you were using values from the calling go-routine in there.

Your second approach is absolutely fine. You can also call .Add(1) inside the loop - but outside the go func block

Kelsnare
  • 635
  • 5
  • 12