Golang channels using select doesn't stop

Question

Go-lang newbie here. I am trying out Go's Tour of Go, and came across an exercise about channels (https://tour.golang.org/concurrency/7). The idea is to walk two trees and then evaluate if the trees are equivalent.

I wanted to solve this exercise using a select waiting for results from both channels. When both would finish I evaluate the resulting slice. Unfortunately the method goes on an infinite loop. I added some output to see what was happening and noticed that only one of the channels was being closed, and then opened again.

I am clearly doing something wrong, but I can't see what. My question is what am I doing wrong? What assumption am I making regarding the closing of channels that makes the code below go into an infinite loop?

package main

import (
    "golang.org/x/tour/tree"
    "fmt"
)

// Walk walks the tree t sending all values
// from the tree to the channel ch.
func Walk(t *tree.Tree, ch chan int) {
    _walk(t, ch)
    close(ch)
}

func _walk(t *tree.Tree, ch chan int) {
    if (t.Left != nil) {
        _walk(t.Left, ch)
    }
    ch <- t.Value
    if (t.Right != nil) {
        _walk(t.Right, ch)
    }
}

// Same determines whether the trees
// t1 and t2 contain the same values.
func Same(t1, t2 *tree.Tree) bool {
    ch1 := make(chan int)
    ch2 := make(chan int)
    go Walk(t1, ch1)
    go Walk(t2, ch2)

    var out1 []int
    var out2 []int

    var tree1open, tree2open bool
    var tree1val, tree2val int
    for {
        select {
        case tree1val, tree1open = <- ch1:
            out1 = append(out1, tree1val)
        case tree2val, tree2open = <- ch2:
            out2 = append(out2, tree2val)
        default:
            if (!tree1open && !tree2open) {
                break
            } else {
                fmt.Println("Channel open?", tree1open, tree2open)
            }
        }
    }

    if (len(out1) != len(out2)) {
        return false
    }

    for i := 0 ; i < len(out1) ; i++ {
        if (out1[i] != out2[i]) {
            return false
        }
    }

    return true
}

func main() {
    ch := make(chan int)
    go Walk(tree.New(1), ch)

    for i := range ch {
        fmt.Println(i)
    }

    fmt.Println(Same(tree.New(1), tree.New(1)))
    fmt.Println(Same(tree.New(1), tree.New(2)))
}

Answer 1

A "break" statement terminates execution of the innermost "for", "switch" or "select" statement.
see: http://golang.org/ref/spec#Break_statements
the break statement in your example terminates the select statement, the "innermost" statement.
so add label: ForLoop before for loop and add break ForLoop

ForLoop:
    for {
        select {
        case tree1val, tree1open = <-ch1:
            if tree1open {
                out1 = append(out1, tree1val)
            } else if !tree2open {
                break ForLoop
            }
        case tree2val, tree2open = <-ch2:
            if tree2open {
                out2 = append(out2, tree2val)
            } else if !tree1open {
                break ForLoop
            }
        }
    }

don't read the rest if you want to solve that problem yourself, and come back when you are done: solution 1 (similar to yours):

package main

import "fmt"
import "golang.org/x/tour/tree"

// Walk walks the tree t sending all values
// from the tree to the channel ch.
func Walk(t *tree.Tree, ch chan int) {
    _walk(t, ch)
    close(ch)
}

func _walk(t *tree.Tree, ch chan int) {
    if t.Left != nil {
        _walk(t.Left, ch)
    }
    ch <- t.Value
    if t.Right != nil {
        _walk(t.Right, ch)
    }
}

// Same determines whether the trees
// t1 and t2 contain the same values.
func Same(t1, t2 *tree.Tree) bool {
    ch1, ch2 := make(chan int), make(chan int)
    go Walk(t1, ch1)
    go Walk(t2, ch2)

    tree1open, tree2open := false, false
    tree1val, tree2val := 0, 0
    out1, out2 := make([]int, 0, 10), make([]int, 0, 10)
ForLoop:
    for {
        select {
        case tree1val, tree1open = <-ch1:
            if tree1open {
                out1 = append(out1, tree1val)
            } else if !tree2open {
                break ForLoop
            }
        case tree2val, tree2open = <-ch2:
            if tree2open {
                out2 = append(out2, tree2val)
            } else if !tree1open {
                break ForLoop
            }
        }
    }
    if len(out1) != len(out2) {
        return false
    }
    for i, v := range out1 {
        if v != out2[i] {
            return false
        }
    }
    return true
}

func main() {
    ch := make(chan int)
    go Walk(tree.New(1), ch)
    for i := range ch {
        fmt.Println(i)
    }
    fmt.Println(Same(tree.New(1), tree.New(1)))
    fmt.Println(Same(tree.New(1), tree.New(2)))
}

output:

1
2
3
4
5
6
7
8
9
10
true
false    

another way:

package main

import "fmt"
import "golang.org/x/tour/tree"

// Walk walks the tree t sending all values
// from the tree to the channel ch.
func Walk(t *tree.Tree, ch chan int) {
    _walk(t, ch)
    close(ch)
}

func _walk(t *tree.Tree, ch chan int) {
    if t != nil {
        _walk(t.Left, ch)
        ch <- t.Value
        _walk(t.Right, ch)
    }
}

// Same determines whether the trees
// t1 and t2 contain the same values.
func Same(t1, t2 *tree.Tree) bool {
    ch1, ch2 := make(chan int), make(chan int)
    go Walk(t1, ch1)
    go Walk(t2, ch2)
    for v := range ch1 {
        if v != <-ch2 {
            return false
        }
    }
    return true
}

func main() {
    ch := make(chan int)
    go Walk(tree.New(1), ch)
    for v := range ch {
        fmt.Println(v)
    }
    fmt.Println(Same(tree.New(1), tree.New(1)))
    fmt.Println(Same(tree.New(1), tree.New(2)))
}

output:

1
2
3
4
5
6
7
8
9
10
true
false    

and see:
Go Tour Exercise: Equivalent Binary Trees

Answer 2

The suggestion by Amd is a valid one in the previous answer. However, looking at the problem you're trying to solve, it still does not solve the it. (If you run the program, it will output true for both the cases)

Here's the problem:

for {
    select {
    case tree1val, tree1open = <-ch1:
        out1 = append(out1, tree1val)
    case tree2val, tree2open = <-ch2:
        out2 = append(out2, tree2val)
    default:
        //runtime.Gosched()
        if !tree1open && !tree2open {
            break ForLoop
        } else {
            fmt.Println("Channel open?", tree1open, tree2open)
        }
    }
}

In this case, since the default values of tree1open and tree2open are false (according to golang specification), it goes to the 'default' case, because select is non-blocking, and simply breaks from the ForLoop, without even filling the out1 and out2 slices (possibly, since these are goroutines). Hence the lengths of out1 and out2 remain zero, due to which it outputs true in most cases.

Here is the correction:

ForLoop:
for {
    select {
    case tree1val, tree1open = <-ch1:
        if tree1open {
            out1 = append(out1, tree1val)
        }
        if !tree1open && !tree2open {
            break ForLoop
        }
    case tree2val, tree2open = <-ch2:
        if tree2open {
            out2 = append(out2, tree2val)
        }
        if !tree1open && !tree2open {
            break ForLoop
        }
    default:

    }
}

The key thing to note is that we have to check whether the channels have been closed in both cases (equivalent to saying whether tree1open and tree2open are both false). Here, it will correctly fill up out1 and out2 slices and then further compare their respective values.

The check for tree1open (or tree2open) being true has been added before append simply to avoid appending zero values to out1 (or out2).

Thanks,