Best way of using sync.WaitGroup with external function

Question

I have some issues with the following code:

package main

import (
"fmt"
"sync"
)
// This program should go to 11, but sometimes it only prints 1 to 10.
func main() {
    ch := make(chan int)
    var wg sync.WaitGroup
    wg.Add(2)
    go Print(ch, wg) //
    go func(){

        for i := 1; i <= 11; i++ {
            ch <- i
        }

        close(ch) 
        defer wg.Done()


    }()

    wg.Wait() //deadlock here
}

// Print prints all numbers sent on the channel.
// The function returns when the channel is closed.
func Print(ch <-chan int, wg sync.WaitGroup) {
    for n := range ch { // reads from channel until it's closed
        fmt.Println(n)
    }
    defer wg.Done()
}

I get a deadlock at the specified place. I have tried setting wg.Add(1) instead of 2 and it solves my problem. My belief is that I'm not successfully sending the channel as an argument to the Printer function. Is there a way to do that? Otherwise, a solution to my problem is replacing the go Print(ch, wg)line with:

go func() {
Print(ch)
defer wg.Done()
}

and changing the Printer function to:

func Print(ch <-chan int) {
    for n := range ch { // reads from channel until it's closed
        fmt.Println(n)
    }

}

What is the best solution?

Jhvh

from golang.org/pkg/sync it notes: "Values containing the types defined in this package should not be copied" Therefore, only pass sync.WaitGroup by pointer reference. – Ruinous

Answer 1

Well, first your actual error is that you're giving the Print method a copy of the sync.WaitGroup, so it doesn't call the Done() method on the one you're Wait()ing on.

Try this instead:

package main

import (
    "fmt"
    "sync"
)

func main() {    
    ch := make(chan int)

    var wg sync.WaitGroup
    wg.Add(2)    

    go Print(ch, &wg)

    go func() {  
        for i := 1; i <= 11; i++ {
            ch <- i
        }
        close(ch)
        defer wg.Done()
    }()          

    wg.Wait() //deadlock here
}                

func Print(ch <-chan int, wg *sync.WaitGroup) {
    for n := range ch { // reads from channel until it's closed
        fmt.Println(n)
    }            
    defer wg.Done()
}

Now, changing your Print method to remove the WaitGroup of it is a generally good idea: the method doesn't need to know something is waiting for it to finish its job.

Venture

Got it, I didn't know you needed the address to be sent to Print instead of the WaitGroup itself. I agree that the method doesn't need to know about the WaitGroup, but suppose I want this anyway. What then, does the star do? Does it pick out the ACTUAL WaitGrooup I have defined in main? And why is this one not a copy? – Jhvh

The *sync.WaitGroup tells the compiler you want a pointer to a WaitGroup instead of a WaitGroup. So the compiler expects you to call the method by giving it an address, hence the &wg. – Venture

Don't think you can remove print's waitgroup as main's wg.Wait could fall through before the last value is printed if you only waited for the sender to complete. Also, no reason to end a function with a defer wg.Done(), defer basically means, run this at the end. drop the defer – Outrigger

This answer doesn't show the what I think is the correct pattern that Elwinar posted in the comments of an answer below at play.golang.org/p/lOopY0bYHT. Print is wrapped in an anonymous goroutine in main and wg.Done() is called at that scope. – Stefaniastefanie

Answer 2

I agree with @Elwinar's solution, that the main problem in your code caused by passing a copy of your Waitgroup to the Print function.

This means the wg.Done() is operated on a copy of wg you defined in the main. Therefore, wg in the main could not get decreased, and thus a deadlock happens when you wg.Wait() in main.

Since you are also asking about the best practice, I could give you some suggestions of my own:

• Don't remove defer wg.Done() in Print. Since your goroutine in main is a sender, and print is a receiver, removing wg.Done() in receiver routine will cause an unfinished receiver. This is because only your sender is synced with your main, so after your sender is done, your main is done, but it's possible that the receiver is still working. My point is: don't leave some dangling goroutines around after your main routine is finished. Close them or wait for them.

• Remember to do panic recovery everywhere, especially anonymous goroutine. I have seen a lot of golang programmers forgetting to put panic recovery in goroutines, even if they remember to put recover in normal functions. It's critical when you want your code to behave correctly or at least gracefully when something unexpected happened.

• Use defer before every critical calls, like sync related calls, at the beginning since you don't know where the code could break. Let's say you removed defer before wg.Done(), and a panic occurrs in your anonymous goroutine in your example. If you don't have panic recover, it will panic. But what happens if you have a panic recover? Everything's fine now? No. You will get deadlock at wg.Wait() since your wg.Done() gets skipped because of panic! However, by using defer, this wg.Done() will be executed at the end, even if panic happened. Also, defer before close is important too, since its result also affects the communication.

So here is the code modified according to the points I mentioned above:

package main

import (
    "fmt"
    "sync"
)

func main() {
    ch := make(chan int)
    var wg sync.WaitGroup
    wg.Add(2)
    go Print(ch, &wg)
    go func() {

        defer func() {
            if r := recover(); r != nil {
                println("panic:" + r.(string))
            }
        }()

        defer func() {
            wg.Done()
        }()

        for i := 1; i <= 11; i++ {
            ch <- i

            if i == 7 {
                panic("ahaha")
            }
        }

        println("sender done")
        close(ch)
    }()

    wg.Wait()
}

func Print(ch <-chan int, wg *sync.WaitGroup) {
    defer func() {
        if r := recover(); r != nil {
            println("panic:" + r.(string))
        }
    }()

    defer wg.Done()

    for n := range ch {
        fmt.Println(n)
    }
    println("print done")
}

Hope it helps :)

Harpist

I completely disagree with the Don't remove the wg from Print. But maybe my proposal wasn't understood correctly, what I advise is to do something like that : play.golang.org/p/lOopY0bYHT That way, your routine doesn't know that is is synchronized, which makes it simpler. As a rule of thumb, unless synchronization is part of the algorithm, your code should never be aware of it. Meaning, a method that print something to the screen should never need any synchronization awareness. – Venture

Its a good idea to recover from panics, but in this example it is overdoing it. The only thing that can panic is sending to a closed channel, and you are the one that close it. Don't fall in the trap of checking everything every time, that will just bloat your code with unnecessary checks and make it hard to read. – Venture

@Venture I said I disagree with you because you only mentioned to remove the wg from print, but never said it should be added somewhere else. And what I want to demo in my post is stated as bold: never leave a goroutine dangling around. Also, the reason why I add recover for all functions is, you never know when you will add in some logic that causes panic, it may not panic now, but it may in the future. – Harpist

In the end of the OP's question is an example where he show how he wants to remove the wg from the function… that's the solution I was agreeing with (as shown in my playground example). I agree with the dangling routines warning, but obviously this is a teaching example about routines. And adding recovers everywhere because one day someone might add code that cause panics is an unnecessary precaution that just makes the coe unreadable now, and as such is likely to cause errors (that will, ironically, trigger the panics you're protecting yourself from). – Venture