4

I am quite new to golang.So, please spare me the sword ( if possible ).

I was trying to get data from the web by studying the tutorial here

Now, the tutorial goes all well, but I wanted to check for edge cases and error-handling ( just to be thorough with my new learning of the language, don't want to be the one with half-baked knowledge ).

Here's my go-playground code.

Before asking I looked at a lot of references like :

  1. Go blog defer,panic and recover
  2. handling panics in goroutines
  3. how-should-i-write-goroutine

And a few more, however I couldn't figure it out much.

Here's the code in case you don't want to go to the playground ( for reasons yet unknown to man ) :

// MakeRequest : Makes requests concurrently
func MakeRequest(url string, ch chan<- string, wg *sync.WaitGroup) {
    start := time.Now()
    resp, err := http.Get(url)
    defer func() {
        resp.Body.Close()
        wg.Done()
            if r := recover(); r != nil {
                fmt.Println("Recovered in f", r)
            }
    }()
    if err != nil {
        fmt.Println(err)
        panic(err)
    }
    secs := time.Since(start).Seconds()
    body, _ := ioutil.ReadAll(resp.Body)

    ch <- fmt.Sprintf("%.2f elapsed with response length: %d %s", secs, len(body), url)
}

func main() {
    var wg sync.WaitGroup
    output := []string{
        "https://www.facebook.com",
        "",
    }
    start := time.Now()
    ch := make(chan string)
    for _, url := range output {
        wg.Add(1)
        go MakeRequest(url, ch, &wg)
    }

    for range output {
        fmt.Println(<-ch)
    }
    fmt.Printf("%.2fs elapsed\n", time.Since(start).Seconds())
}

Update

I changed the code to ( let's say ) handle the error in goroutine like this ( go-playground here ):

func MakeRequest(url string, ch chan<- string, wg *sync.WaitGroup) {
    start := time.Now()
    resp, err := http.Get(url)

    if err == nil {
        secs := time.Since(start).Seconds()
        body, _ := ioutil.ReadAll(resp.Body)

        ch <- fmt.Sprintf("%.2f elapsed with response length: %d %s", secs, len(body), url)
        // fmt.Println(err)
        // panic(err)
    }
    defer wg.Done()
}

Update 2 :

After an answer I changed the code to this and it successfully removes the chan deadlock, however now I need to handle this in main :

func MakeRequest(url string, ch chan<- string, wg *sync.WaitGroup) {
    defer wg.Done()
    start := time.Now()
    resp, err := http.Get(url)

    if err == nil {
        secs := time.Since(start).Seconds()
        body, _ := ioutil.ReadAll(resp.Body)

        ch <- fmt.Sprintf("%.2f elapsed with response length: %d %s", secs, len(body), url)
        // fmt.Println(err)
        // panic(err)
    }
    // defer resp.Body.Close()
    ch <- fmt.Sprintf("")
}

Isn't there a more elegant way to handle this ?

But now I get locked up in deadlock.

Thanks and regards.
Temporarya
( a golang noobie )

temporarya
  • 715
  • 1
  • 7
  • 16

1 Answers1

9

You are using recover correctly. You have two problems:

  1. You are using panic incorrectly. You should only panic when there was a programming error. Avoid using panics unless you believe taking down the program is a reasonable response to what happened. In this case, I would just return the error, not panic.

  2. You are panicing during your panic. What is happening is that you are first panicing at panic(err). Then in your defer function, you are panicing at resp.Body.Close(). When http.Get returns an error, it returns a nil response. That means that resp.Body.Close() is acting on a nil value.

The idiomatic way to handle this would be something like the following:

func MakeRequest(url string, ch chan<- string, wg *sync.WaitGroup) {
    defer wg.Done()
    start := time.Now()
    resp, err := http.Get(url)
    if err != nil {
        //handle error without panicing
    }
    // there was no error, so resp.Body is guaranteed to exist.
    defer resp.Body.Close()
    ...

Response to update: Ifhttp.Get() returns an error, you never send on the channel. At some point all goroutines except the main goroutine stop running and the main goroutine is waiting on <-ch. Since that channel receive will never complete and there is nothing else for the Go runtime to schedule, it panics (unrecoverably).


Response to comment: To ensure the channel doesn't hang, you need some sort of coordination to know when messages will stop coming. How this is implemented depends on your real program, and an example cannot necessarily extrapolate to reality. For this example, I would simply close the channel when the WaitGroup is done.

Playground

func main() {
    var wg sync.WaitGroup
    output := []string{
        "https://www.facebook.com",
        "",
    }
    start := time.Now()
    ch := make(chan string)
    for _, url := range output {
        wg.Add(1)
        go MakeRequest(url, ch, &wg)
    }

    go func() {
        wg.Wait()
        close(ch)
    }()

    for val := range ch {
        fmt.Println(val)
    }
    fmt.Printf("%.2fs elapsed\n", time.Since(start).Seconds())
}
Stephen Weinberg
  • 51,320
  • 14
  • 134
  • 113
  • So, how do I handle that chan to not hang, I assume there could be two ways, first I send something to it even in case of error or I should change the code in `main` to handle input from chan differently , am I correct ? If so, can you give some example for the same, it would be great help. Thanks. – temporarya Feb 06 '19 at 17:48
  • Yes, definitely [don't panic](https://github.com/golang/go/wiki/CodeReviewComments#dont-panic) @temporarya, this is one of the [go-proverbs](https://go-proverbs.github.io/) – Havelock Feb 06 '19 at 17:54
  • @Stephen Weinberg Why do you create a new goroutine `func()` to handle `wg.Wait()` , why not do it in the main thread itself ? – temporarya Feb 06 '19 at 18:27
  • 2
    Try it. If you are waiting for the wait group you are not listening on the channel. If you are not listening on the channel, you will deadlock. – Stephen Weinberg Feb 06 '19 at 19:06