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?
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 :)
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.
If you love us? You can donate to us via Paypal or buy me a coffee so we can maintain and grow! Thank you!
Donate Us With