loong / go-concurrency-exercises

Hands on exercises with real-life examples to study and practice Go concurrency patterns. Test-cases are provided to verify your answers.
Other
1.17k stars 401 forks source link

Exercise 0: Test fail for correct solution when crawling is started immediately #3

Closed zyhazwraith closed 7 years ago

zyhazwraith commented 7 years ago

Hi, I used channel and time.Sleep to work out this exercise, If i write like this

<-ch
fetcher.Fetch(url)
time.Sleep(1s)
ch <- 1

test would fail, and code below would work

<-ch
Sleep()
Fetch
ch <- 1

I guess that check_test shouldn't check time gap when first crawl, or may initial start in check_test to a very small value to avoid this case :) (it is my first time to open an issue, i'm bit of nervous :S, sorry if i'm not wrting in the proper way)

loong commented 7 years ago

Hi @zyhazwraith,

Thanks for the feedback and what an honor to receive your very first issue!

I am quite curious about your approach because I haven't thought of doing it in this way, to be honest. However, I am having trouble to reproduce what you did, can you post your full main.go solution?

P.S. don't need to worry too much about the format of an issue. Just make sure everything is described clear and detailed!

zyhazwraith commented 7 years ago

Hi @mindworker , sorry to reply late below are two copies of my main.go, the first one can pass the test the only difference is time.Sleep run before or after fetch

var ch = make(chan int, 1)

// Crawl uses `fetcher` from the `mockfetcher.go` file to imitate a
// real crawler. It crawls until the maximum depth has reached.
func Crawl(url string, depth int, wg *sync.WaitGroup) {
    defer wg.Done()

    if depth <= 0 {
        return
    }

    ch<-1
    time.Sleep(time.Second)
    body, urls, err := fetcher.Fetch(url)
    <-ch
    if err != nil {
        fmt.Println(err)
        return
    }

    fmt.Printf("found: %s %q\n", url, body)

    wg.Add(len(urls))
    for _, u := range urls {
        // Do not remove the `go` keyword, as Crawl() must be
        // called concurrently
        go Crawl(u, depth-1, wg)
    }
    return
}

func main() {
    var wg sync.WaitGroup

    wg.Add(1)
    Crawl("http://golang.org/", 4, &wg)
    wg.Wait()
}

And this one couldn't

var ch = make(chan int, 1)

// Crawl uses `fetcher` from the `mockfetcher.go` file to imitate a
// real crawler. It crawls until the maximum depth has reached.
func Crawl(url string, depth int, wg *sync.WaitGroup) {
    defer wg.Done()

    if depth <= 0 {
        return
    }

    ch<-1
    body, urls, err := fetcher.Fetch(url)
    time.Sleep(time.Second)
    <-ch
    if err != nil {
        fmt.Println(err)
        return
    }

    fmt.Printf("found: %s %q\n", url, body)

    wg.Add(len(urls))
    for _, u := range urls {
        // Do not remove the `go` keyword, as Crawl() must be
        // called concurrently
        go Crawl(u, depth-1, wg)
    }
    return
}

func main() {
    var wg sync.WaitGroup

    wg.Add(1)
    Crawl("http://golang.org/", 4, &wg)
    wg.Wait()
}
loong commented 7 years ago

@zyhazwraith thanks a lot for this. I found out that I should better start the timer after the first crawl have been made! Pull the new check_test.go and your solution should pass the test now.

And I actually haven't thought of the way you have solved it. Feel free to publish the solution and I link to it in my solutions collection (work in progress).

Do have a look at my solution as well. I use the limiter pattern to solve this: https://github.com/mindworker/go-concurrency-solutions/blob/master/0-limit-crawler/main.go#L17

zyhazwraith commented 7 years ago

@mindworker Got it, Thanks for your great efforts in preparing these exercises, it helps me a lot in starting programming with golang.Also thank for your solutions.