Why is my code causing a stall or race condition?

Question

For some reason, once I started adding strings through a channel in my goroutine, the code stalls when I run it. I thought that it was a scope/closure issue so I moved all code directly into the function to no avail. I have looked through Golang's documentation and all examples look similar to mine so I am kind of clueless as to what is going wrong.

func getPage(url string, c chan<- string, swg sizedwaitgroup.SizedWaitGroup) {
    defer swg.Done()
    doc, err := goquery.NewDocument(url)

    if err != nil{
        fmt.Println(err)
    }

    nodes := doc.Find(".v-card .info")
    for i := range nodes.Nodes {
        el := nodes.Eq(i)
        var name string
        if el.Find("h3.n span").Size() != 0{
            name = el.Find("h3.n span").Text()
        }else if el.Find("h3.n").Size() != 0{
            name = el.Find("h3.n").Text()
        }

        address := el.Find(".adr").Text()
        phoneNumber := el.Find(".phone.primary").Text()
        website, _ := el.Find(".track-visit-website").Attr("href")
        //c <- map[string] string{"name":name,"address":address,"Phone Number": phoneNumber,"website": website,};
        c <- fmt.Sprint("%s%s%s%s",name,address,phoneNumber,website)
        fmt.Println([]string{name,address,phoneNumber,website,})

    }
}

func getNumPages(url string) int{
    doc, err := goquery.NewDocument(url)
    if err != nil{
        fmt.Println(err);
    }
    pagination := strings.Split(doc.Find(".pagination p").Contents().Eq(1).Text()," ")
    numItems, _ := strconv.Atoi(pagination[len(pagination)-1])
    return int(math.Ceil(float64(numItems)/30))
}


func main() {
    arrChan := make(chan string)
    swg := sizedwaitgroup.New(8)
    zips := []string{"78705","78710","78715"}

    for _, item := range zips{
        swg.Add()
        go getPage(fmt.Sprintf(base_url,item,1),arrChan,swg)
    }
    swg.Wait()

}

Edit: so I fixed it by passing sizedwaitgroup as a reference but when I remove the buffer it doesn't work does that mean that I need to know how many elements will be sent to the channel in advance?

Answer 1

You don't need to know size to make it work. But you might in order to exit cleanly. Which can be a bit tricky to observe at time because your program will exit once your main function exits and all goroutines still running are killed immediately finished or not.

As a warm up example, change readChannel in photoionized's response to this:

func readChannel(c <-chan string) {
  for {
      url := <-c
      fmt.Println (url)
  }
}

It only adds printing to the original code. But now you'll see better what is actually happening. Notice how it usually only prints two strings when code actually writes 3. This is because code exits once all writing goroutines finish, but reading goroutine is aborted mid way as result. You can "fix" it by removing "go" before readChannel (which would be same as reading the channel in main function). And then you'll see 3 strings printed, but program crashes with a dead lock as readChannel is still reading from the channel, but nobody writes into it anymore. You can fix that too by reading exactly 3 strings in readChannel(), but that requires knowing how many strings you expect to receive.

Here is my minimal working example (I'll use it to illustrate the rest):

package main

import (
    "fmt"
    "sync"
) 

func getPage(url string, c chan<- string, wg *sync.WaitGroup) {
    defer wg.Done()
    c <- fmt.Sprintf("Got page for %s\n",url)
}


func readChannel(c chan string, wg *sync.WaitGroup) {
    defer wg.Done()
    var url string
    ok := true
    for ok {
        url, ok = <- c
        if ok {
            fmt.Printf("Received: %s\n", url)
        } else {
            fmt.Println("Exiting readChannel")
        }
    }
}

func main() {
    arrChan := make(chan string)
    var swg sync.WaitGroup
    base_url := "http://test/%s/%d"
    zips := []string{"78705","78710","78715"}

    for _, item := range zips{
        swg.Add(1)
        go getPage(fmt.Sprintf(base_url,item,1),arrChan,&swg)
    }

    var wg2 sync.WaitGroup
    wg2.Add(1)
    go readChannel(arrChan, &wg2)

    swg.Wait()

    // All written, signal end to readChannel by closing the channel 
    close(arrChan)
    wg2.Wait()
}

Here I close the channel to signal to readChannel that there is nothing left to read, so it can exit cleanly at proper time. But sometimes you might want instead to tell readChannel to read exactly 3 strings and finish. Or may be you would want to start one reader for each writer and each reader will read exactly one string... Well, there are many ways to skin a cat and choice is all yours.

Note, if you remove wg2.Wait() line your code becomes equivalent to photoionized's response and will only print two strings whilst writing 3. This is because code exits once all writers finish (ensured by swg.Wait()), but it does not wait for readChannel to finish.

If you remove close(arrChan) line instead, your code will crash with a deadlock after printing 3 lines as code waits for readChannel to finish, but readChannel waits to read from a channel which nobody is writing to anymore.

If you just remove "go" before the readChannel call, it becomes equivalent of reading from channel inside main function. It will again crash with a dead lock after printing 3 strings because readChannel is still reading when all writers have already finished (and readChannel has already read all they written). A tricky point here is that swg.Wait() line will never be reached by this code as readChannel never exits.

If you move readChannel call after the swg.Wait() then code will crash before even printing a single string. But this is a different dead lock. This time code reaches swg.Wait() and stops there waiting for writers. First writer succeeds, but channel is not buffered, so next writer blocks until someone reads from the channel the data already written. Trouble is - nobody reads from the channel yet as readChannel has not been called yet. So, it stalls and crashes with a dead lock. This particular issue can be "fixed", but making channel buffered as in make(chan string, 3) as that will allow writers to keep writing even though nobody is reading from that channel yet. And sometimes this is what you want. But here again you have to know the maximum of messages to ever be in the channel buffer. And most of the time it's only deferring a problem - just add one more writer and you are where you started - code stalls and crashes as channel buffer is full and that one extra writer is waiting for someone to read from the buffer.

Well, this should covers all bases. So, check your code and see which case is yours.

Answer 2

Issue

Building off of Colin Stewart's answer, from the code you have posted, as far as I can tell, your issue is actually with reading your arrChan. You write into it, but there's no place where you read from it in your code.

From the documentation :

If the channel is unbuffered, the sender blocks until the receiver has received the value. If the channel has a buffer, the sender blocks only until the value has been copied to the buffer; if the buffer is full, this means waiting until some receiver has retrieved a value.

By making the channel buffered, what's happening is your code is no longer blocking on the channel write operations, the line that looks like:

c <- fmt.Sprint("%s%s%s%s",name,address,phoneNumber,website)

My guess is that if you're still hanging at when the channel has a size of 5000, it's because you have more than 5000 values returned across all of your loops over node.Nodes. Once your buffered channel is full, the operations block until the channel has space, just like if you were writing to an unbuffered channel.

Fix

Here's a minimal example showing you how you would fix something like this (basically just add a reader)

package main

import "sync"

func getPage(item string, c chan<- string) {
    c <- item
}

func readChannel(c <-chan string) {
    for {
        <-c
    }
}

func main() {
    arrChan := make(chan string)
    wg := sync.WaitGroup{}
    zips := []string{"78705", "78710", "78715"}

    for _, item := range zips {
        wg.Add(1)
        go func() {
            defer wg.Done()
            getPage(item, arrChan)
        }()
    }
    go readChannel(arrChan) // comment this out and you'll deadlock
    wg.Wait()
}

Answer 3

Your channel has no buffer, so writes will block until the value can be read, and at least in the code you have posted, there are no readers.