fix: WithTimeout mutates a caller-supplied *http.Client — confirmed data race

[karlwaldman]

Closes #38

The defect

WithTimeout wrote straight through to Client.httpClient.Timeout (client.go:118). When the caller supplied their own client via WithHTTPClient, that pointer was theirs — so the option reconfigured an object the SDK does not own.

Verified still reproducing on main (cd51e3a) before any change was written. Two distinct failures:

Symptom	Measured on main
Cross-client contamination	Two SDK clients over one shared *http.Client; WithTimeout(1ns) on the first; the second client's timeout read 1ns (want 30s)
Data race	-race reports a read/write race: net/http.(*Client).do at net/http/client.go:608 vs WithTimeout at client.go:118

Both are in the P2 category because they are silent. The contaminated sibling does not error at construction — it fails later, as a timeout on an unrelated request, in a place with no connection to the line that caused it.

The fix

NewClient resolves the HTTP client once, after the options have run, into a struct the SDK owns (ownedHTTPClient). A supplied client is shallow-copied: Transport, Jar and CheckRedirect carry over and stay shared — that is what the caller supplied it for, and connection pooling must not be thrown away — while Timeout, the only field the SDK sets, lives on a struct nobody else holds.

Two contract clarifications fall out, both covered by tests:

• An explicit WithTimeout wins over a supplied client's Timeout, in either option order. Previously whichever option ran last won, so NewClient(k, WithTimeout(5s), WithHTTPClient(hc)) and NewClient(k, WithHTTPClient(hc), WithTimeout(5s)) disagreed. This is a behaviour change; it is the direction that matches the documented intent of WithTimeout.
• WithTimeout(0) still means "no timeout". timeoutSet is tracked separately from timeout so the zero value is not misread as "unset" and silently restored to DefaultTimeout.

A supplied client with no explicit WithTimeout keeps its own Timeout — the SDK does not impose DefaultTimeout on a client the caller configured.

Red

Tests written first, run with -race against unmodified main:

--- FAIL: TestWithTimeoutDoesNotMutateCallerClient (0.00s)
    http_client_ownership_test.go:29: caller's own *http.Client was mutated: Timeout = 1ns, want 30s
--- FAIL: TestWithTimeoutDoesNotContaminateSiblingClient (0.00s)
    http_client_ownership_test.go:42: sibling client's timeout was contaminated: got 1ns, want 30s
==================
WARNING: DATA RACE
Read at 0x00c0002e4aa8 by goroutine 11:
  net/http.(*Client).do()
      /opt/homebrew/Cellar/go/1.26.5/libexec/src/net/http/client.go:608 +0xc8
  net/http.(*Client).Do()
      /opt/homebrew/Cellar/go/1.26.5/libexec/src/net/http/client.go:592 +0x4bc
  github.com/OilpriceAPI/oilpriceapi-go.(*Client).doRequestWithHeaders()
      client.go:849 +0x4a4
  github.com/OilpriceAPI/oilpriceapi-go.(*Client).doRequest()
      client.go:804 +0x150
  github.com/OilpriceAPI/oilpriceapi-go.(*Client).GetLatestPrices()
      client.go:211 +0x120
  github.com/OilpriceAPI/oilpriceapi-go.TestWithTimeoutRaceOnSharedHTTPClient.func2()
      http_client_ownership_test.go:65 +0xb8

Previous write at 0x00c0002e4aa8 by goroutine 12:
  github.com/OilpriceAPI/oilpriceapi-go.TestWithTimeoutRaceOnSharedHTTPClient.func3.WithTimeout.2()
      client.go:118 +0x50
  github.com/OilpriceAPI/oilpriceapi-go.NewClient()
      client.go:102 +0x1b0
==================
--- FAIL: TestWithTimeoutRaceOnSharedHTTPClient (0.05s)
    testing.go:1712: race detected during execution of test
--- FAIL: TestSuppliedHTTPClientRetainsTransportJarAndRedirectPolicy (0.00s)
    http_client_ownership_test.go:96: SDK kept the caller's *http.Client pointer instead of its own copy
--- FAIL: TestExplicitTimeoutWinsInEitherOptionOrder (0.00s)
    --- FAIL: TestExplicitTimeoutWinsInEitherOptionOrder/client_then_timeout (0.00s)
        http_client_ownership_test.go:143: caller's client was mutated: 5s
    --- FAIL: TestExplicitTimeoutWinsInEitherOptionOrder/timeout_then_client (0.00s)
        http_client_ownership_test.go:140: timeout = 30s, want the explicit 5s
FAIL
FAIL	github.com/OilpriceAPI/oilpriceapi-go	0.075s

The race test drives the real path — a real httptest server, GetLatestPrices, httpClient.Do — rather than asserting on a field, so it is the race detector that decides, not the test author.

Green

=== RUN   TestWithTimeoutDoesNotMutateCallerClient
--- PASS: TestWithTimeoutDoesNotMutateCallerClient (0.00s)
=== RUN   TestWithTimeoutDoesNotContaminateSiblingClient
--- PASS: TestWithTimeoutDoesNotContaminateSiblingClient (0.00s)
=== RUN   TestWithTimeoutRaceOnSharedHTTPClient
--- PASS: TestWithTimeoutRaceOnSharedHTTPClient (0.05s)
=== RUN   TestSuppliedHTTPClientRetainsTransportJarAndRedirectPolicy
--- PASS: TestSuppliedHTTPClientRetainsTransportJarAndRedirectPolicy (0.00s)
=== RUN   TestSuppliedHTTPClientKeepsItsOwnTimeout
--- PASS: TestSuppliedHTTPClientKeepsItsOwnTimeout (0.00s)
=== RUN   TestExplicitTimeoutWinsInEitherOptionOrder
--- PASS: TestExplicitTimeoutWinsInEitherOptionOrder (0.00s)
    --- PASS: TestExplicitTimeoutWinsInEitherOptionOrder/client_then_timeout (0.00s)
    --- PASS: TestExplicitTimeoutWinsInEitherOptionOrder/timeout_then_client (0.00s)
=== RUN   TestWithTimeoutZeroMeansNoTimeout
--- PASS: TestWithTimeoutZeroMeansNoTimeout (0.00s)
PASS
ok  	github.com/OilpriceAPI/oilpriceapi-go	1.073s

Full suite

Check	Result
go test ./...	ok github.com/OilpriceAPI/oilpriceapi-go 16.550s — 294 pass / 0 fail (285 baseline + 9 new)
go test -race ./...	ok github.com/OilpriceAPI/oilpriceapi-go 18.371s
go vet ./...	clean
gofmt -l .	clean

Interaction with #41

The #41 PR (fix/nil-http-client) guards WithHTTPClient(nil). The two branches touch different functions and do not conflict in either merge order. ownedHTTPClient here also handles a nil supplied client, so a nil is safe once either lands; #41 remains the explicit statement of that contract.

Scope

No version bump, no tag, no release.

🤖 Generated with Claude Code

https://claude.ai/code/session_015ao5paex73xXvuM424Libo

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: c2db3a96-31f0-4efd-a5a5-595d346baf34

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[karlwaldman]

Closing as superseded by #44, which fixed #38 first and merged at 19:05:05 UTC. No replacement PR.

This PR's implementation duplicates #44. It resolves the HTTP client after the options run, applies an explicit timeout to a shallow copy, and keeps WithTimeout(0) separate from unset. This branch now conflicts with main, so it should not be re-landed.

Before closing, I checked whether any of this PR's tests pin behaviour that main's client_timeout_test.go does not. The two candidates were the cross-client contamination case and the -race case.

Check	Result
main's client_timeout_test.go against pre-#44 client.go (cd51e3a), -race	Red. The sibling case fails at client_timeout_test.go:55 ("client without a timeout override was changed"). TestConcurrentTimeoutOptionsWithSharedHTTPClient reports WARNING: DATA RACE / race detected during execution of test.
this PR's TestWithTimeoutDoesNotContaminateSiblingClient + TestWithTimeoutRaceOnSharedHTTPClient against fixed main, -race -count=3	Green 3/3. No residual defect.

Main already covers both assertions, and its tests catch both failure modes on the old code. This PR's version of the race test also issues requests while the timeout is being written, so it exercises the read path inside net/http.(*Client).do. The race detector already fails main's test on the same field, so that extra coverage is small and not worth a separate PR.

🤖 Generated with Claude Code

https://claude.ai/code/session_015ao5paex73xXvuM424Libo