Repository navigation
Fix clone client transport race - #48
Merged
Merged
Conversation
Why this change was needed: Clone is documented as the safe reuse path for concurrent requests, but clones shared one http.Client. Concurrent clone requests wrote Client.Transport on the same client while net/http read it, producing a race under go test -race. What changed: - Bind New clients to their transport during construction. - Make Clone copy the http.Client while sharing the transport by default. - Copy-on-write clone transports when clone transport settings change. - Isolate DisableRedirect client changes from the base agent. - Add regression coverage for clone client ownership and transport setting isolation. - Update usage docs for the new clone ownership model. Problem solved: Concurrent clone requests no longer race on shared client state, while the shared transport connection pool behavior is preserved.
|
Bugbot is not enabled for this team, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Owner
Author
Clone client/transport flowThis is the ownership model after this PR. Default clone reuseflowchart TD
Base["base SuperAgent"]
BaseClient["base http.Client"]
T0["shared http.Transport / connection pool"]
A["clone A SuperAgent"]
AClient["clone A http.Client"]
B["clone B SuperAgent"]
BClient["clone B http.Client"]
Base --> BaseClient
Base --> T0
BaseClient --> T0
A --> AClient
A --> T0
AClient --> T0
B --> BClient
B --> T0
BClient --> T0
Each clone owns its own Transport copy-on-writeflowchart TD
T0["base/shared Transport"]
T1["clone B private Transport copy"]
Base["base SuperAgent"] --> BaseClient["base Client"] --> T0
A["clone A"] --> AClient["clone A Client"] --> T0
B["clone B"] --> BClient["clone B Client"] --> T1
B -. "Proxy/TLS/Timeouts/DisableCompression" .-> T1
When a clone changes transport settings, it gets a private transport and its own client is rebound to that private transport. The base agent and other clones keep using the original shared transport. Request execution pathsequenceDiagram
participant G as Goroutine
participant S as Clone SuperAgent
participant C as Owned http.Client
participant T as Bound Transport
G->>S: End()
S->>S: MakeRequest()
S->>C: sync Client.Transport = SuperAgent.Transport
Note over S,C: This writes only this clone's owned client
S->>C: Do(req)
C->>T: RoundTrip(req)
T-->>C: response
C-->>S: response
S-->>G: resp, body, errs
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This fixes the clone concurrency race by changing clone ownership of HTTP client state while preserving shared transport reuse:
New()now binds the newly createdhttp.Clientto the agenthttp.Transportimmediately.Clone()now copies thehttp.Clientvalue instead of sharing the same*http.Clientpointer.*http.Transportby default, preserving connection pool reuse.clone.Client.Transportaligned withclone.Transport:DisableCompression()TLSClientConfig()via existingsafeModifyTransport()pathProxy()via existingsafeModifyTransport()pathTimeouts()DisableRedirect()now isolates clone client changes throughsafeModifyHttpClient().DisableTransportSwapand directSuperAgent.Transportassignment compatibility, but writes only to each clone's own client afterClone().docs/usage.mdnow documents that clones copy HTTP client settings and share transport by default, with private transports created when transport settings change on a clone.Why
Clone()is the documented reuse path for concurrent requests. Before this change, clones shared the same*http.Client; concurrent clone requests could writeClient.Transportwhilenet/httpread it inClient.Do, causinggo test -race -run '^TestConcurrently$'to report a data race.Tests
env GOCACHE=/tmp/gorequest-go-build go test -run 'TestConfigurationSettersAndCloneIsolation|TestCloneClientSettingsDoNotLeak|TestCloneTransportSettingsDoNotLeak|TestTimeoutsUpdatesTransportAndIgnoresNonTransport' -count=1 -v ./...env GOCACHE=/tmp/gorequest-go-build go test -race -run '^TestConcurrently$' -count=1 -v ./...env GOCACHE=/tmp/gorequest-go-build GOLANGCI_LINT_CACHE=/tmp/gorequest-golangci-lint make lintenv GOCACHE=/tmp/gorequest-go-build make testenv GOCACHE=/tmp/gorequest-go-build go test -v ./...