5
var wg sync.WaitGroup
var v int32 = 0 
for i = 0; i < 100; i++{
   go func(){
       wg.Add(1) // wrong place
       atomic.AddInt32(&v,1)
       wg.Done()
   } 
}
wg.Wait()
fmt.Println(v)

Here is a piece of code I see from this video https://subscription.packtpub.com/video/application_development/9781788994880/97598/97608/goroutines

but the v is always less than 100, I think the reason could be the wg.Wait() will over earlier than expectation because we put wg.Add(1) inside the anonymous function and in the same goroutine wg.Done() will be called immediately, thus main goroutine resume execution from blocked state.

But if we put the wg.Add(1) into the for loop, v will always be 100.

var wg sync.WaitGroup
var v int32 = 0 
for i = 0; i < 100; i++{
   wg.Add(1)
   go func(){
       atomic.AddInt32(&v,1)
       wg.Done()
   } 
}
wg.Wait()
fmt.Println(v)

My question is why we can guarantee that main goroutine will always block here such that v will equal to 100 finally. Is that possible if before the for loop add one task to wg, and main goroutine resume execution here since there is no task at that moment.

icza
  • 389,944
  • 63
  • 907
  • 827
  • 2
    Put it immediately before `go` – Jonathan Hall Dec 09 '20 at 09:10
  • And please copy-paste your _actual_ code. The code you've pasted is invalid, since you're trying to access symbols that don't exist (i.e. `wg.wait()` instead of `wg.Wait()`) – Jonathan Hall Dec 09 '20 at 09:14
  • 2
    No, it has nothing to do with time. It has to do with order. Imagine a goroutine is a messenger, sent on a journey, and you need to wait for the messenger to return. Will you tell the messenger to sign the log book before leaving on his journey, or after? If he signs after leaving, you have no way of knowing how many messengers you've sent, since you may have sent some of them that have not yet signed in. – Jonathan Hall Dec 09 '20 at 09:18
  • @Flimzy Sorry about that. This code is in the video, I type it line by line. Sorry again –  Dec 09 '20 at 09:21
  • Presumably you're copying it into your editor, and compiling it. Copy/paste that version into your question. – Jonathan Hall Dec 09 '20 at 09:22
  • If you know how long the loop is (in this case 100) you can also put the `wg.Add(100)` before the loop -- making it a bit faster. – TehSphinX Dec 09 '20 at 09:25
  • 1
    @TehSphinX Yes, that's an alternative but I'd advise against it (explained in my answer). – icza Dec 09 '20 at 09:31
  • Ah, didn't see that. Yes it does require care. I'd still do it though. – TehSphinX Dec 09 '20 at 09:35
  • 1
    @TehSphinX You'd do it for "performance", but when you're using `WaitGroup` and you launch goroutines, it won't result in noticeable speedup. – icza Dec 09 '20 at 09:43
  • Also I'd do it for readability. It keeps the loop clean(er). – TehSphinX Dec 09 '20 at 09:48
  • 1
    @TehSphinX There's a singe `wg.Add()` call in both cases, I don't think it's cleaner, especially since moving it out of the loop it gets farther from where the goroutine are launched. – icza Dec 09 '20 at 10:08
  • Your issue "v will always be 100" has nothing to do with the waitgroup; it's because you're closing over the loop variable which isn't thread-safe. You want to pass it as an argument to your anonymous function, or create a local copy in the loop. – Adrian Dec 09 '20 at 14:37

2 Answers2

15

You should always call wg.Add() before you launch the goroutine that will call wg.Done().

In your corrected example the main goroutine can only reach wg.Wait() after the for loop, which guarantees you call wg.Add() a hundred times, so wg.Wait() will block until wg.Done() is called 100 times.

When wg.Add() calls are in the new goroutines, there is no guarantee that any of the wg.Add() calls will be executed before the main goroutine reaches wg.Wait() since they run concurrently (without synchronization up until this point). The behavior in this case is non-deterministic (depends on the goroutine scheduler which is non-deterministic without explicit synchronization).

Note that if you know your loop does 100 iterations, another alternative is to call wg.Add(100) before the loop. I advise against this practice though because this requires care when the loop contains break or continue operations, which might result in less goroutines launched and thus ultimately your main goroutine getting stuck. Yes, in your case this might be trivial, but if this code evolves in time, it might become less obvious and might result in future errors. Saying it's faster is irrelevant when launching goroutines is involved in the scenario. If you only ever call wg.Add(1) before launching a goroutine, it won't matter if later you conditionally skip this part, because you'll be skipping wg.Add() along with launching the goroutine, and your code will remain correct.

Simple "rules" to follow when using sync.WaitGroup: (quoted from this answer)

  • call WaitGroup.Add() in the "original" goroutine (that starts a new) before the go statement
  • recommended to call WaitGroup.Done() deferred, so it gets called even if the goroutine panics
  • if you want to pass WaitGroup to other functions (and not use a package level variable), you must pass a pointer to it, else the WaitGroup (which is a struct) would be copied, and the Done() method called on the copy wouldn't be observed on the original
icza
  • 389,944
  • 63
  • 907
  • 827
  • The stated issue "v will always be 100" has nothing to do with the WaitGroups, it's because it's closing over the loop variable. – Adrian Dec 09 '20 at 15:41
2

Like others have mentioned wg.add() should be called before calling any go routine. So inside the main thread:

It's Good practice to defer at the beginning so that you don't forget as it is regarded as clean-up action.

var wg sync.WaitGroup
var v int32 = 0 
for i = 0; i < 100; i++{
   wg.Add(1) //right place to put wg.add(1)
   go func(){
       defer wg.Done()
       atomic.AddInt32(&v,1)
   } 
}
wg.Wait()
fmt.Println(v)
Nouru Muneza
  • 57
  • 6