fix(proxy): surface chosen ProxyURL on Request and Response - #874
fix(proxy): surface chosen ProxyURL on Request and Response #874Shinku-Chen wants to merge 11 commits into
Conversation
StantonMatt
left a comment
StantonMatt
left a comment
There was a problem hiding this comment.
I checked this locally.
Validation passed:
go test ./proxy -run 'TestRoundRobinProxySwitcher_PropagatesProxyURL|TestRoundRobinProxySwitcher_ProxyURLOnError' -count=1go test ./... -count=1git diff --check upstream/master...HEAD
I also rechecked the visible GitHub checks: Build/Test for Go 1.21-1.24, Codecov, and License Compliance are all green.
The default RoundRobinProxySwitcher path looks good to me, including the error path before response headers.
One compatibility point I would tighten before merge: the PR body says custom ProxyFunc implementations need no migration, but the implementation now only reads ProxyURLKey as *string. A custom proxy function following the previous exported-key pattern still returns the proxy URL correctly, but no longer populates Request.ProxyURL / Response.ProxyURL:
c.SetProxyFunc(func(pr *http.Request) (*url.URL, error) {
ctx := context.WithValue(pr.Context(), colly.ProxyURLKey, proxyURL.String())
*pr = *pr.WithContext(ctx)
return proxyURL, nil
})I verified that shape with a local-only scratch test on this branch; Visit() succeeded, but Request.ProxyURL was empty in OnResponse.
So I think either syncProxyURL should accept both *string and legacy string values from ProxyURLKey, or the PR should explicitly document that custom proxy functions that set ProxyURLKey need to switch to the new pointer-holder pattern. A small test for the custom ProxyFunc case would make that expectation clear.
|
Rechecked current head The new Local checks: go test ./proxy -run 'TestRoundRobinProxySwitcher_PropagatesProxyURL|TestRoundRobinProxySwitcher_ProxyURLOnError|TestSetProxyFunc_LegacyContextStringPropagates|TestSetProxyFunc_LegacyContextStringOnError' -count=1 -v
go test ./... -count=1
git diff --check origin/master...HEADThose passed. The visible hosted checks are also green now: Build/Test for Go 1.21-1.24, Codecov, and License Compliance. |
|
Thanks for taking the time to re-verify on 1295e39, @StantonMatt — really appreciate the careful follow-up. Glad the wrapper + new tests address the concern. Now just waiting on a maintainer pass for merge. |
Shinku-Chen
changed the title
2 participants
Summary
r.Request.ProxyURLwas always empty afterVisit()through anyProxyFunc, including the built-inRoundRobinProxySwitcher.Response.ProxyURLmirrored fromRequest.ProxyURLsoOnResponse/OnErrorcan both see the proxy that was tried.ProxyFuncimplementations.Root cause
httpBackend.Initsetshttp.Client.Timeout, which makesnet/http.send()callforkReq()and shallow-copy the request beforeTransport.RoundTrip. Writing to the fork'sctxvia*pr = *pr.WithContext(...)was lost when the fork was discarded — colly readProxyURLKeyoff the original request and got nothing.Approach
Use a
*stringholder placed in the request context — pointer writes survive bothforkReqand the error path (where no*Responseflows back).SetProxyFuncwraps the user'sProxyFunc: capturesorigCtxfirst, then writes the returned*url.URLthrough the holder. CapturingorigCtxkeeps the holder reachable even if a legacyfshadowsProxyURLKeyonpr.syncProxyURLreads the holder twice: insidecheckResponseHeadersFunc(soOnResponseHeaderssees it) and afterCachereturns (error-path fallback).Response.ProxyURLis mirrored at the three Response construction sites.proxy/proxy.go:roundRobinSwitcher.GetProxyno longer touches the context — wrapper handles the write.Test plan
TestRoundRobinProxySwitcher_PropagatesProxyURL/…_ProxyURLOnError— built-in switcher, success + dial-refused.TestSetProxyFunc_LegacyContextStringPropagates/…OnError— custom legacy-shapeProxyFuncusingWithContext+string; asserts the returned*url.URL(not the user's discarded ctx write) is what surfaces.go test ./... -count=1All green locally and on hosted CI (Go 1.21–1.24, Codecov, License Compliance) at
1295e39.