diff --git a/colly.go b/colly.go index b4b96e61e..fce752fdc 100644 --- a/colly.go +++ b/colly.go @@ -1417,10 +1417,13 @@ func (c *Collector) Cookies(URL string) []*http.Cookie { } // Clone creates an exact copy of a Collector without callbacks. -// HTTP backend, robots.txt cache and cookie jar are shared -// between collectors. +// The visited-URL store, robots.txt cache, cookie jar and HTTP transport +// are shared between collectors so cookies and the connection pool flow +// across them. The *http.Client and the callback-protecting lock are +// independent so per-collector settings like AllowURLRevisit take effect +// on the clone's redirects without leaking back into the parent. func (c *Collector) Clone() *Collector { - return &Collector{ + clone := &Collector{ AllowedDomains: c.AllowedDomains, AllowURLRevisit: c.AllowURLRevisit, CacheDir: c.CacheDir, @@ -1441,7 +1444,6 @@ func (c *Collector) Clone() *Collector { TraceHTTP: c.TraceHTTP, Context: c.Context, store: c.store, - backend: c.backend, debugger: c.debugger, Async: c.Async, redirectHandler: c.redirectHandler, @@ -1449,12 +1451,33 @@ func (c *Collector) Clone() *Collector { htmlCallbacks: make([]*htmlCallbackContainer, 0, 8), xmlCallbacks: make([]*xmlCallbackContainer, 0, 8), scrapedCallbacks: make([]ScrapedCallback, 0, 8), - lock: c.lock, + lock: &sync.RWMutex{}, requestCallbacks: make([]RequestCallback, 0, 8), responseCallbacks: make([]ResponseCallback, 0, 8), robotsMap: c.robotsMap, wg: &sync.WaitGroup{}, } + + // Independent httpBackend + http.Client so CheckRedirect closes over + // the clone, not the parent. Without this, the redirect handler would + // evaluate the parent's AllowURLRevisit and filters, so toggling those + // on the clone has no effect on redirects. + // + // Transport, cookie jar and Timeout are kept shared so connection + // pooling and session cookies continue to flow between collectors. + // LimitRules are copied by value (the slice header) — each *LimitRule + // is still shared because LimitRule.Init is idempotent. + clone.backend = &httpBackend{ + LimitRules: append([]*LimitRule(nil), c.backend.LimitRules...), + lock: &sync.RWMutex{}, + Client: &http.Client{ + Transport: c.backend.Client.Transport, + Jar: c.backend.Client.Jar, + Timeout: c.backend.Client.Timeout, + CheckRedirect: clone.checkRedirectFunc(), + }, + } + return clone } func (c *Collector) checkRedirectFunc() func(req *http.Request, via []*http.Request) error { diff --git a/colly_test.go b/colly_test.go index 4a8dda9c2..432f1c854 100644 --- a/colly_test.go +++ b/colly_test.go @@ -2236,3 +2236,52 @@ func TestLimitRuleClone(t *testing.T) { t.Error("clone.Init() must not mutate the source's unexported state") } } + +// TestCloneAllowURLRevisitIndependent verifies that AllowURLRevisit on a +// cloned Collector takes effect during redirects, independently of the +// parent's setting. +// +// Previously Clone() reused the parent's *http.Client, including its +// CheckRedirect closure — and that closure captures the *parent* +// Collector. As a result, the clone's CheckRedirect evaluated the +// parent's AllowURLRevisit field, so toggling AllowURLRevisit on the +// clone had no effect on redirects: visiting a redirect endpoint twice +// failed with "already visited" even though the clone allowed revisits. +func TestCloneAllowURLRevisitIndependent(t *testing.T) { + ts := newTestServer() + defer ts.Close() + + parent := NewCollector() + clone := parent.Clone() + clone.AllowURLRevisit = true + + redirectURL := ts.URL + "/redirect" + finalURL := ts.URL + "/redirected/" + + requestCount := make(map[string]int) + clone.OnRequest(func(r *Request) { + requestCount[r.URL.String()]++ + }) + responseCount := make(map[string]int) + clone.OnResponse(func(r *Response) { + responseCount[r.Request.URL.String()]++ + }) + + for i := 0; i < 2; i++ { + if err := clone.Visit(redirectURL); err != nil { + t.Fatalf("visit %d: unexpected error: %v", i+1, err) + } + } + if requestCount[redirectURL] != 2 { + t.Errorf("redirect URL visited %d times, want 2", requestCount[redirectURL]) + } + if responseCount[finalURL] != 2 { + t.Errorf("final URL visited %d times, want 2", responseCount[finalURL]) + } + + // Parent should keep its original (default false) AllowURLRevisit and + // not gain revisit semantics just because the clone toggled it. + if parent.AllowURLRevisit { + t.Error("parent.AllowURLRevisit was mutated by setting it on the clone") + } +}