0

I use multiple goroutines to run the task, and when one of them is done, return and close the channel, that will cause an panic:send on closed channel.

See code:

func fetch(urls []string) *http.Response {
    ch := make(chan *http.Response)
    defer close(ch)
    for _, url := range urls {
        go func() {
            resp, err := http.Get(url)
            if err == nil {
                ch <- resp
            }
        }()
    }
    return <-ch
}

If don't close the channel, there is no problem, but I don't think so good, so is there any elegant solution?

Thanks for all the answers,here is my final code:

func fetch(urls []string) *http.Response {
    var wg sync.WaitGroup
    ch := make(chan *http.Response)
    wg.Add(len(urls))
    for _, url := range urls {
        go func(url string) {
            defer wg.Done()
            resp, err := http.Get(url)
            if err == nil {
                ch <- resp
            }
        }(url)
    }
    go func() {
        wg.Wait()
        close(ch)
    }()
    return <-ch
}
liwei2633
  • 61
  • 7
  • 1
    The problem is that `fetch()` returns before your goroutines complete, and when it returns, the `defer close(ch)` executes. You probably need a waitgroup. – Jonathan Hall Oct 17 '19 at 08:10
  • `return <-ch` also doesn't make sense... it will return only a single value (the first one returned by any goroutine), but you're fetching multiple values. I'm not sure what your goal is, but your code doesn't make sense. – Jonathan Hall Oct 17 '19 at 08:11
  • It could make sense, say if you only want the first response. I also found it weird though, most likely the channel should be returned. – Clément Oct 17 '19 at 08:12
  • 1
    @Clément: That aspect might make sense, but then you need to (gracefully) clean up the other goroutines, which isn't being done. – Jonathan Hall Oct 17 '19 at 08:14

6 Answers6

4

In this version, the channel ch has sufficient rooms so that routines can push to it without blocking if the corresponding channel reader is absent.

package main

import (
    "fmt"
    "net/http"
    "sync"
)

func main() {
    urls := []string{"", "", ""}
    res := fetch(urls)
    fmt.Println(res)
}
func fetch(urls []string) *http.Response {
    var wg sync.WaitGroup
    ch := make(chan *http.Response, len(urls))

    for _, url := range urls {
        wg.Add(1)
        url := url
        go func() {
            defer wg.Done()
            req, err := http.NewRequest(http.MethodGet, url, nil)
            if err != nil {
                return
            }
            resp, err := http.DefaultClient.Do(req)
            if err != nil {
                return
            }
            if resp != nil {
                ch <- resp // no need to test the context, ch has rooms for this push to happen anyways.
            }
        }()
    }

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

    return <-ch
}

https://play.golang.org/p/5KUeaUS2FLg

This version illustrates the implementation with a context attached to the request for cancellation.

package main

import (
    "context"
    "fmt"
    "net/http"
    "sync"
)

func main() {
    ctx, cancel := context.WithCancel(context.Background())
    defer cancel()
    cancel()
    urls := []string{"", "", ""}
    res := fetch(ctx, urls)
    fmt.Println(res)
}

func fetch(ctx context.Context, urls []string) *http.Response {
    var wg sync.WaitGroup
    ch := make(chan *http.Response, len(urls))
    for _, url := range urls {
        if ctx.Err() != nil {
            break // break asap.
        }
        wg.Add(1)
        url := url
        go func() {
            defer wg.Done()
            req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
            if err != nil {
                return
            }
            resp, err := http.DefaultClient.Do(req)
            if err != nil {
                return
            }
            if resp != nil {
                ch <- resp // no need to test the context, ch has rooms for this push to happen anyways.
            }
        }()
    }
    go func() {
        wg.Wait()
        close(ch)
    }()
    return <-ch
}

https://play.golang.org/p/QUOReYrWqDp

As a friendly reminder, don't try to be too smart, use a sync.WaitGroup, write the process with the most simple logic and let it flow until you can safely close that channel.

  • I agree this is correct, and simpler in the sense that there is one less goroutine. Although: 1. Your `wg.Wait()` statement might be read before any `wg.Add(1)` is executed, which would cause a panic. You can move that goroutine after the `for` statement to fix. 2. Your last `select` will never user the context case, you can remove this `select`. – Clément Oct 18 '19 at 02:23
  • if Wait() is called before Add, it exits, it does not panic. but then, there could be a write on closed channel. I don t agree with you on 2. –  Oct 18 '19 at 07:09
  • 1
    Yes, and writing on the closed channel will panic... For 2, I don't see how you could select the "Done" case, since `cancel` will only be called after the `select` statement. – Clément Oct 18 '19 at 08:10
  • 1
    Since the buffered channel is in the scope of the function, the N-1 other responses (that won't be read after the function returns) are free for garbage collection later? – Tobias Pfandzelter Aug 17 '21 at 10:07
  • 1
    @TobiasPfandzelter yes, https://stackoverflow.com/a/36613932/4466350 –  Aug 17 '21 at 10:09
  • Dont be too clever by half but not nearly smart enough. This is subtly hard, see my edits on the post, so many traps i felt into.... –  Aug 17 '21 at 10:45
2

If your goal is to read only one result, then cancel the other requests, try something like this:

func fetch(urls []string) *http.Response {
    ch := make(chan *http.Response)
    defer close(ch)
    ctx, cancel := context.WithCancel(context.Background())
    defer cancel()
    for _, url := range urls {
        go func(ctx context.Context, url string) {
            req, _ := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
            resp, err := http.Do(req)
            if err == nil {
                select {
                case ch <- resp:
                case <- ctx.Done():
                }
            }
        }(ctx, url)
    }
    return <-ch
}

This uses a cancelable context, so once the first result is returned, the remaining http requests are signaled to abort.

NOTE: Your code has a bug that I have fixed above:

func _, url := range urls {
    go func() {
        http.Do(url) // `url` is changed here on each iteration through the for loop, meaning you will not be calling the url you expect
    }()
}

Fix this with by passing url to the goroutine func, rather than using a closure:

func _, url := range urls {
    go func(url string) {
        http.Do(url) // `url` is now safe
    }(url)
}

Related post: Why golang don't iterate correctly in my for loop with range?

Jonathan Hall
  • 75,165
  • 16
  • 143
  • 189
  • I was thinking about this as well, but I'm not convinced this is perfectly safe. What if two requests were completed by the time either could reach the `select` statement ? Or what if `fetch` consumes the value in the channel and closes the channel, right at the time a request completes and before it reaches the `select` statement ? – Clément Oct 17 '19 at 08:46
  • @Clément: The `cancel` should happen before the `close(ch)` to safeguard against that, you're right. I'll update the answer. – Jonathan Hall Oct 17 '19 at 08:47
  • note, an easy way to get it wrong with such implementation, is to introduce a legitimate but overlooked early return against non nil errors returned by `http` calls, `if err != nil {return}`. This is hypothetical, but lets consider some maintenance and an incomplete test case suite, and the function would deadlock if all requests fails, as `ch` will never be written to. –  Aug 17 '21 at 10:36
  • there is a subtle bug if all requests fail, ch is never written. Further more, it is possible for it to panic with a `write on a closed channel` because `defer close(ch)` might happen before those were all done `ch <- resp`. Can such panic happen within a select ? I need to double check.... –  Aug 17 '21 at 10:56
  • this is convoluted, i had to introduce a sleep (to return before the routines were done) and disable the errors (because i guess the play do not allow outgoing requests) for the panic on closed channel to happen. https://play.golang.org/p/7BXBRBVjaUV –  Aug 17 '21 at 11:06
  • this is the deadlock on errors https://play.golang.org/p/IaJTLHYocie –  Aug 17 '21 at 11:08
  • sorry for that many messages, there is also a typo in `http.Do`, it should be, i believe, `http.DefaultClient.Do` –  Aug 17 '21 at 11:08
0

Your code is returning after the first response is received. The channel is then closed, leaving the other go routines to send on a closed channel.

Rather than returning the first response, it may be more appropriate to return an array of responses, ordered in the same length as the urls.

Since the http request can error, it would be prudent to return an array of errors too.

package main

import (
    "fmt"
    "net/http"
)

func main() {
    fmt.Println(fetch([]string{
        "https://google.com",
        "https://stackoverflow.com",
        "https://passkit.com",
    }))
}

type response struct {
    key      int
    response *http.Response
    err      error
}

func fetch(urls []string) ([]*http.Response, []error) {
    ch := make(chan response)
    defer close(ch)
    for k, url := range urls {
        go func(k int, url string) {
            r, err := http.Get(url)
            resp := response {
                key:      k,
                response: r,
                err:      err,
            }
            ch <- resp
        }(k, url)
    }

    resp := make([]*http.Response, len(urls))
    respErrors := make([]error, len(urls))

    for range urls {
        r := <-ch
        resp[r.key] = r.response
        respErrors[r.key] = r.err
    }
    return resp[:], respErrors[:]
}

playground

PassKit
  • 12,231
  • 5
  • 57
  • 75
  • this strategy does not `return the valid first http response` but it is neat, dont get me wrong. –  Aug 17 '21 at 11:16
0

You close channel too soon so that's why you can see this error,
it's better to close channel only when you won't write anything more into channel, and for this purposes you may use sync.WaitGroup, like this:

package main

import (
    "fmt"
    "net/http"
    "sync"
)

func main() {
    ch := fetch([]string{"http://github.com/cn007b", "http://github.com/thepkg"})
    fmt.Println("\n", <-ch)
    fmt.Println("\n", <-ch)
}

func fetch(urls []string) chan *http.Response {
    ch := make(chan *http.Response, len(urls))
    wg := sync.WaitGroup{}
    wg.Add(len(urls))
    for _, url := range urls {
        go func() {
            defer wg.Done()
            resp, err := http.Get(url)
            if err == nil {
                ch <- resp
            }
        }()
    }
    go func() {
        wg.Wait()
        close(ch)
    }()
    return ch
}

Also, with purpose to provide slice with responses as result you may do something like this:

func fetch2(urls []string) (result []*http.Response) {
    ch := make(chan *http.Response, len(urls))
    wg := sync.WaitGroup{}
    wg.Add(len(urls))
    for _, url := range urls {
        go func() {
            defer wg.Done()
            resp, err := http.Get(url)
            if err == nil {
                ch <- resp
            }
        }()
    }
    wg.Wait()
    close(ch)
    for v := range ch {
        result = append(result, v)
    }
    return result
}
cn007b
  • 16,596
  • 7
  • 59
  • 74
0

You can add two goroutines:

  1. Receive all requests, sending the first one coming to be returned and dropping the following ones. When the WaitGroup finishes, it closes your first channel.
  2. One to wait for the WaitGroup and send the signal to close the first channel.
func fetch(urls []string) *http.Response {
    var wg sync.WaitGroup
    ch := make(chan *http.Response)
    for _, url := range urls {
        wg.Add(1)
        go func(url string) {
            resp, err := http.Get(url)          
            if err == nil {
                ch <- resp:
            }
            wg.Done()
        }(url)
    }
    done := make(chan interface{})
    go func(){
        wg.Wait()
        done <- interface{}{}
        close(done)
    }

    out := make(chan *http.Response)
    defer close(out)
    go func(){
        first = true
        for {
            select {
            case r <- ch:
                if first {
                    first = false
                    out <- r
                }
            case <-done:
                close(ch)
                return
            }
        }
    }()

    return <-out
}

This should be safe... maybe.

Clément
  • 804
  • 6
  • 16
  • 1
    uselessly complex. The async sequence `WaitGroup.Wait()+close(chan)` is safe. –  Oct 17 '19 at 18:33
  • I thought so as well, but I was just trying to find a solution to the safety issue. Your solution solves both the safety issue and the complexity issue. – Clément Oct 18 '19 at 02:29
0

The code you recommended at the end only works if at least one of your calls succeeds. If you get an error for each of the HTTP GETs you make, your function will block forever. You can add a second channel to inform you that your calls are done:

func fetch(urls []string) *http.Response {
    var wg sync.WaitGroup
    ch := make(chan *http.Response, len(urls))
    done := make(chan struct{})
    wg.Add(len(urls))
    for _, url := range urls {
        go func(url string) {
            defer wg.Done()
            resp, err := http.Get(url)
            // only put a response into the channel if we didn't get an error
            if err == nil {
                ch <- resp
            }
        }(url)
    }
    go func() {
        wg.Wait()
        // inform main routine that all calls have exited
        done <- struct{}{}
        close(ch)
    }()

    // return either the first response or nil
    select {
        case r := <-ch:
        return r
        case <-done:
        break
   }

   // you can do additional error handling here
   return nil
}
Tobias Pfandzelter
  • 317
  • 2
  • 6
  • yes. In your case consider to just `close(ch)` after `wg.Wait`. A read on closed channel returns the zero value (https://stackoverflow.com/a/29269960/4466350). A read on nil channel blocks forever (https://medium.com/justforfunc/why-are-there-nil-channels-in-go-9877cc0b2308). Though, one small remarks to make it a good answer. I did not understand easily `The code you recommended at the end only works if at least one of your calls succeeds.`, you can link to answer directly (there is a share button). Also the done channel is superfluous. –  Aug 17 '21 at 10:53
  • see https://play.golang.org/p/WKhnF8ghXVJ but this was a good catch –  Aug 17 '21 at 11:01