Race condition due to incorrect sync.WaitGroup usage about colly HOT 6 CLOSED

Why do you a errChan channel?
Is it correct to do this?

     go func() {
         defer c.wg.Done()
 
         errChan := c.scrape(...)

         // Do some stuff ....
 
     }()
 

from colly.

Why do you a errChan channel?

I use it here to let caller wait for an error on that channel.

errChan := scrapeAsync(...)

... do stuff ...

err := <-errChan

Is it correct to do this?

It depends on what are you going to do with that error (c.scrape(...) returns error).

from colly.

@THE108 you are right, the current behavior is not suitable for your needs. As a workaround you can wrap c.Visit with a function and use a WaitGroup there.

The func (c *Collector) scrapeAsync(...) chan<- error approach is interesting, how do you imagine the public API for this? Duplicating all the Visit, Post, PostRaw, etc.. functions of the Collector and the Request with an Async prefix wouldn't be the best..
Perhaps Collector could be extended with an Async bool member and if Collector.Async is true then scrapeAsync would be called instead of scrape. With this solution it is not possible to return an error channel, this is the only downside, but errors can be handled in OnError callbacks. What do you think?

from colly.

I think it's better to add new functionality than changing existing one (breaking back compatibility).

Anyway, I think both options are not perfect. So, maybe we can think about:

1. Let caller manage concurrency themselves. Just provide a sync.WaitGroup as a param (possibly nil).

2. Manage concurrency internally. Collector could handle its own goroutine pool (which will provide better goroutine reuse and request rate limiting).

from colly.

I think it's better to add new functionality than changing existing one (breaking back compatibility).

The solution I suggested doesn't break the existing api.

from colly.

Sorry, I misunderstood your suggestion.

Perhaps Collector could be extended with an Async bool member and if Collector.Async is true then scrapeAsync would be called instead of scrape.

I think that should work.

from colly.