4

Like here I created a go playground sample: sGgxEh40ev, but cannot get it work.

quit := make(chan bool)
res := make(chan int)

go func() {
    idx := 0
    for {
        select {
        case <-quit:
            fmt.Println("Detected quit signal!")
            return
        default:
            fmt.Println("goroutine is doing stuff..")
            res <- idx
            idx++
        }
    }

}()

for r := range res {
    if r == 6 {
        quit <- true
    }
    fmt.Println("I received: ", r)
}

Output:

goroutine is doing stuff..
goroutine is doing stuff..
I received:  0
I received:  1
goroutine is doing stuff..
goroutine is doing stuff..
I received:  2
I received:  3
goroutine is doing stuff..
goroutine is doing stuff..
I received:  4
I received:  5
goroutine is doing stuff..
goroutine is doing stuff..
fatal error: all goroutines are asleep - deadlock!

Is this possible? Where am I wrong

icza
  • 389,944
  • 63
  • 907
  • 827
lnshi
  • 2,310
  • 3
  • 19
  • 42
  • You must end your main for loop. As the error tells you: Nothing arrives on res. – Volker Sep 11 '17 at 05:29
  • @Volker but even i do `break` the main for loop, then do `quit <- true`, still deadlock, can you help to create a works playground example? – lnshi Sep 11 '17 at 05:31
  • In your program, `res <- idx` and `quit <- true` are both waiting for their values to be received which causes the deadlock. A better pattern to use channels can be found [here](https://play.golang.org/p/uf94rfXkva) – John S Perayil Sep 11 '17 at 06:43
  • @John S Perayil That's a good and simple example, but doesn't have consumer signalling to the producer to exit. – Ravi R Sep 11 '17 at 06:51
  • @Ravi I was just linking to a simple example on how to quit using a channel properly. I should have been more clear in the comment. [Reference](http://www.golangbootcamp.com/book/concurrency) – John S Perayil Sep 11 '17 at 07:00

3 Answers3

7

The problem is that in the goroutine you use a select to check if it should abort, but you use the default branch to do the work otherwise.

The default branch is executed if no communications (listed in case branches) can proceed. So in each iteration quit channel is checked, but if it cannot be received from (no need to quit yet), default branch is executed, which unconditionally tries to send a value on res. Now if the main goroutine is not ready to receive from it, this will be a deadlock. And this is exactly what happens when the sent value is 6, because then the main goroutine tries to send a value on quit, but if the worker goroutine is in the default branch trying to send on res, then both goroutines try to send a value, and none is trying to receive! Both channels are unbuffered, so this is a deadlock.

In the worker goroutine you must send the value on res using a proper case branch, and not in the default branch:

select {
case <-quit:
    fmt.Println("Detected quit signal!")
    return
case res <- idx:
    fmt.Println("goroutine is doing stuff..")
    idx++
}

And in the main goroutine you must break out from the for loop so the main goroutine can end and so the program can end as well:

if r == 6 {
    quit <- true
    break
}

Output this time (try it on the Go Playground):

goroutine is doing stuff..
I received:  0
I received:  1
goroutine is doing stuff..
goroutine is doing stuff..
I received:  2
I received:  3
goroutine is doing stuff..
goroutine is doing stuff..
I received:  4
I received:  5
goroutine is doing stuff..
goroutine is doing stuff..
icza
  • 389,944
  • 63
  • 907
  • 827
  • 1
    Lesser traffic on channel, better solution :) – Ravi R Sep 11 '17 at 07:03
  • @icza, thank u very much for the very detailed explanation, put `res <- idx` on one case branch works fine for this case, but if i have some logic: in some iteration will send back one value, but in some iteration no value need to be sent back, then how to handle that properly? – lnshi Sep 11 '17 at 07:27
  • @lnshi What is "iteration"? The iteration should be when something needs to be sent back, which will result in no iterations where you don't need to send back anything. You should structure your loop that way. Please show something more concrete, maybe as a new question. – icza Sep 11 '17 at 07:30
  • @icza like [Paq01v3ScM](https://play.golang.org/p/Paq01v3ScM), i only want to return the number in the `arbitraryNums` slice, or else do nothing, and quit upon first negative value is encountered. – lnshi Sep 11 '17 at 07:40
  • @lnshi Then here's your solution: [Go Playground](https://play.golang.org/p/p89Ay46lIh). Basically the "iteration" here is the subsequent numbers in that slice, and not an increasing index. Just make sure the numbers you send trigger the `quit` branch in the main goroutine, else you need to send a negative number after the loop manually. – icza Sep 11 '17 at 07:48
  • @icza no, not generic enough, i describe my more complex logic here: [e-TqgatUJb](https://play.golang.org/p/e-TqgatUJb), basically u have no way to know when and what need to be passed back, only can go through the logic, to decide this iteration only need to traverse forward the data, that iteration need to pass back some value or error, and the main goroutine received the wanted data will indicate this goroutine to stop. – lnshi Sep 11 '17 at 08:12
  • @lnshi Then the logic that produces the next value to be sent should be separated. When the next value is ready, that may be used in a `case` statement. If producing the next value may take long time or may even block, that logic must also monitor the `quit` channel. – icza Sep 11 '17 at 08:15
0

The fundamental issue is that producer must always check in between sending values if the consumer (main in your case) has decided to quit reading (in your code this is optional). What's happening is even before the value of quit is sent (and received), the producer goes ahead and sends the next value on res which the consumer never is able to read - the consumer is in fact trying to send the value on the quit channel expecting the producer to read. Added a debug statement which can help you understand : https://play.golang.org/p/mP_4VYrkZZ, - producer is trying to send 7 on res and blocking, and then consumer trying to send value on quit and blocking. Deadlock!

One possible solution is as follows (using a Waitgroup is optional, needed only if you need a clean exit from producer side before return):

package main

import (
    "fmt"
    "sync"
)

func main() {
    //WaitGroup is needed only if need a clean exit for producer
    //that is the producer should have exited before consumer (main)
    //exits - the code works even without the WaitGroup
    var wg sync.WaitGroup
    quit := make(chan bool)
    res := make(chan int)

    go func() {
        idx := 0
        for {

            fmt.Println("goroutine is doing stuff..", idx)
            res <- idx
            idx++
            if <-quit {
                fmt.Println("Producer quitting..")
                wg.Done()
                return
            }

            //select {
            //case <-quit:
            //fmt.Println("Detected quit signal!")
            //time.Sleep(1000 * time.Millisecond)
            //  return
            //default:
            //fmt.Println("goroutine is doing stuff..", idx)
            //res <- idx
            //idx++
            //}
        }


    }()
    wg.Add(1)
    for r := range res {
        if r == 6 {
            fmt.Println("Consumer exit condition met: ", r)
            quit <- true
            break
        }
        quit <- false
        fmt.Println("I received: ", r)
    }
    wg.Wait()

}

Output:

goroutine is doing stuff.. 0
I received:  0
goroutine is doing stuff.. 1
I received:  1
goroutine is doing stuff.. 2
I received:  2
goroutine is doing stuff.. 3
I received:  3
goroutine is doing stuff.. 4
I received:  4
goroutine is doing stuff.. 5
I received:  5
goroutine is doing stuff.. 6
Consumer exit condition met:  6
Producer quitting..

On playground : https://play.golang.org/p/N8WSPvnqqM

Ravi R
  • 1,692
  • 11
  • 16
  • haha, thank u, nice, like this then i need to explicitly quit the goroutine unconditionally – lnshi Sep 11 '17 at 08:57
0

As @icza's answer is pretty clean, which @Ravi's goes to the synchronised way.

But coz I don't want to spend that much effort to restructure the code, and also I don't want to go to the synchronised way, so eventually went to the defer panic recover flow control, as below:

func test(ch chan<- int, data []byte) {
    defer func() {
        recover()
    }()
    defer close(ch)

    // do your logic as normal ...
    // send back your res as normal `ch <- res`
}

// Then in the caller goroutine

ch := make(chan int)
data := []byte{1, 2, 3}
go test(ch, data)

for res := range ch {
    // When you want to terminate the test goroutine:
    //     deliberately close the channel
    //
    // `go -race` will report potential race condition, but it is fine
    //
    // then test goroutine will be panic due to try sending on the closed channel,
    //     then recover, then quit, perfect :) 
    close(ch)
    break
}

any potential risk with this approach?

lnshi
  • 2,310
  • 3
  • 19
  • 42
  • If you have a data race, no, it's not fine. Read [Is it safe to read a function pointer concurrently without a lock?](https://stackoverflow.com/questions/41406501/is-it-safe-to-read-a-function-pointer-concurrently-without-a-lock/41407827#41407827) Moreover you're misusing panic-recover which was created for exceptional cases, so I don't recommend ever doing this. – icza Sep 12 '17 at 07:00
  • @icza I made this playground [TYo_zMaEAU](https://play.golang.org/p/TYo_zMaEAU) to explain what exactly i was doing with this approach, want to hear your insight if in this case u think there still is potential risk, thank u very much – lnshi Sep 12 '17 at 13:50
  • Have you read the answer I linked? There is no point in talking about the correctness or usefulness of an app that contains data race. Period. It might be it runs correctly for you a hundred thousand times, then crash in the next run. It's unpredictable. – icza Sep 12 '17 at 15:21