feat(go): expose connect/read/write timeouts - #17210
Conversation
Add an optional, additive `timeouts` block (connect/read/write in seconds, fractional allowed) to the Go SDK generator config. When set — and the caller has not supplied their own HTTP client — the generated SDK builds a default *http.Client backed by a custom transport: - connect -> net.Dialer.Timeout (+ http.Transport.TLSHandshakeTimeout). Clean. - read -> http.Transport.ResponseHeaderTimeout, plus an idle read deadline on the response body to approximate a full body-read timeout. - write -> net/http exposes no first-class client write deadline, so the SDK bounds the idle time between reads of the request body (which the transport performs while writing the body to the wire) to approximate it as closely as feasible. This residual approximation is documented in the changelog. When `timeouts` is unset, generated output is byte-identical to before (no default client is injected; the existing user-supplied *http.Client behavior is preserved). The same phases are overridable per request via `option.WithConnectTimeout`, `option.WithReadTimeout`, and `option.WithWriteTimeout`, which win over the configured defaults for the phases they cover. The config/runtime lives in the Go v1 generator (core/request_option.go and option/request_option.go); the v2 Zod schema accepts the field so its strict config validation does not reject it. The custom-transport runtime and per-call fields are only emitted when a `timeouts` block is present. Co-Authored-By: Claude <noreply@anthropic.com>
![nitpickybot[bot]](https://avatars.githubusercontent.com/in/2901896?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review Summary
The PR adds opt-in connect/read/write timeouts to the Go SDK generator. The config plumbing and gating are sound, but the write-timeout runtime has a significant concurrency bug (goroutine leak / data race on the buffer) in the idle-read approximation, and there are a couple of correctness edge cases in the transport wiring.
- 🔴 1 critical issue(s)
- 🟡 2 warning(s)
- 🔵 3 suggestion(s)
| func (r *idleTimeoutReadCloser) Read(p []byte) (int, error) { | ||
| done := make(chan readResult, 1) | ||
| go func() { | ||
| n, err := r.rc.Read(p) | ||
| done <- readResult{n: n, err: err} | ||
| }() | ||
| timer := time.NewTimer(r.timeout) | ||
| defer timer.Stop() | ||
| select { | ||
| case res := <-done: | ||
| return res.n, res.err | ||
| case <-timer.C: | ||
| return 0, &timeoutError{} | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 critical
idleTimeoutReadCloser.Read leaks a goroutine and creates a data race on p. On timeout it returns without waiting for the spawned goroutine, which is still calling r.rc.Read(p). The caller (net/http) will reuse or read p while that goroutine writes into it → data race and buffer corruption. It also leaks a goroutine per timed-out read, and the goroutine blocks forever if the underlying Read never returns. This approximation is unsafe for the request body in particular, where net/http may retry/reuse buffers. Consider using http.NewRequestWithContext + a context deadline per phase, or at minimum abandon the buffer after timeout (never reuse p) and document the leak.
| if t.write > 0 && req.Body != nil { | ||
| req.Body = &idleTimeoutReadCloser{ | ||
| rc: req.Body, | ||
| timeout: t.write, | ||
| } | ||
| } | ||
| resp, err := t.base.RoundTrip(req) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if t.read > 0 && resp.Body != nil { | ||
| resp.Body = &idleTimeoutReadCloser{ | ||
| rc: resp.Body, | ||
| timeout: t.read, | ||
| } | ||
| } | ||
| return resp, nil |
There was a problem hiding this comment.
🟡 warning
RoundTrip mutates the incoming *http.Request by reassigning req.Body. Per the http.RoundTripper contract, RoundTrip must not modify the request. Under retries (the SDK has its own retrier) the same request may be reused, and wrapping an already-wrapped body compounds. Clone the request (req = req.Clone(req.Context())) before swapping the body.
|
|
||
| func (e *timeoutError) Error() string { return "fern: i/o timeout" } | ||
| func (e *timeoutError) Timeout() bool { return true } | ||
| func (e *timeoutError) Temporary() bool { return true }` |
There was a problem hiding this comment.
🔵 suggestion
Temporary() is deprecated on net.Error (Go 1.18+) and golangci-lint may flag it. Since the emitted seed runs go vet/lint in CI, consider dropping it or confirm the configured linter version doesn't error on the deprecation.
| } | ||
| // Convert seconds to nanoseconds to preserve fractional precision, and emit | ||
| // a time.Duration literal (nanoseconds is time.Duration's base unit). | ||
| nanos := int64(*seconds * float64(time.Second)) |
There was a problem hiding this comment.
🔵 suggestion
int64(*seconds * float64(time.Second)) can overflow or lose precision for large values, and truncates rather than rounds. Minor, but int64(math.Round(*seconds * float64(time.Second))) would preserve fractional inputs more faithfully (e.g. 2.5s is exact here, but 0.1s isn't).
| MaxIdleConns: 100, | ||
| IdleConnTimeout: 90 * time.Second, | ||
| TLSHandshakeTimeout: connect, | ||
| ResponseHeaderTimeout: read, |
There was a problem hiding this comment.
🟡 warning
ResponseHeaderTimeout: read conflates two different semantics: read is documented as covering both response-header wait and per-read idle on the body. Setting the whole read budget as the header timeout means a slow-header + slow-body response effectively gets read for headers and another read per idle body read — the total can far exceed the user's intended read. Worth clarifying in docs that read is a per-phase idle bound, not a total.
| if err := f.writeRequestOptionStructs(auth, headers, len(idempotencyHeaders) > 0, isMultiURL, inferredParams, serverURLVariablesFromConfig(f.serverURLVariables, environmentsConfig)); err != nil { | ||
| return err | ||
| } | ||
| f.writeTimeoutOptionStructsAndRuntime(timeouts) |
There was a problem hiding this comment.
🔵 suggestion
f.writeTimeoutOptionStructsAndRuntime returns nothing but the other write helpers return errors that are checked. Fine since it can't fail, but if writeOptionStruct ever surfaces an error you're swallowing it here (_ = on lines 1126-1128). Consider propagating rather than discarding.
SDK Generation Benchmark ResultsComparing PR branch against median of 5 nightly run(s) on Full benchmark table (click to expand)
main (generator): generator-only time via --skip-scripts (includes Docker image build, container startup, IR parsing, and code generation — this is the same Docker-based flow customers use via |
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
1 participant
Description
Adds independent connect / read / write timeout options to the generated Go SDK. Ships as its own PR (Java #17207 and Python #17209 are the companions for the same feature). Mirrors their config shape and per-call approach in Go idiom.
Go has no timeout config today — timeout is entirely user-supplied via
option.WithHTTPClient(&http.Client{Timeout: ...})+context.WithTimeout. This PR adds an optional, additivetimeoutsblock that, when set, builds a default*http.Clientbacked by a custom transport.Changes Made
generators.ymlconfig:): optional, additive nestedtimeoutsblock (connect/read/writein seconds, fractional allowed). Non-negative validation. Parsed in Go v1 (cmd.go->config.go) and accepted by the v2 Zod schema so its strict validation does not reject it.core/request_option.go): whentimeoutsis set and the caller has not supplied their own HTTP client,NewRequestOptionsinjects a default*http.Clientbuilt from a custom transport:connect->net.Dialer.Timeout(+http.Transport.TLSHandshakeTimeout). Clean.read->http.Transport.ResponseHeaderTimeout, plus an idle read deadline wrapping the response body to approximate a full body-read timeout.write-> approximation:net/httphas no first-class client write deadline, so a customRoundTripperbounds the idle time between reads of the request body (which the transport performs while writing the body to the wire). Documented in the changelog.option/request_option.go):option.WithConnectTimeout/WithReadTimeout/WithWriteTimeout, which win over the configured defaults for the phases they cover.With*helpers, extra imports) is emitted only when atimeoutsblock is present. When unset, generated output is byte-identical to before.featentry undergenerators/go/sdk/changes/unreleased/noting the write-timeout (and body-read) approximation.Testing
timeouts_test.go:IsConfigured,timeoutLiteral) and v2 Zod schema (timeouts.test.ts, 6 cases).phase-timeoutsvariant toseed/go-sdk/examples(timeouts: {connect: 5, read: 30, write: 30}); regenerated allexamplesvariants — 11/11 seed cases pass.timeoutsare byte-identical (only.fern/metadata.jsonlocal artifacts changed; reverted).go build ./...andgo vet ./...on the generatedphase-timeoutsseed both pass clean (the real gate — Go fails on unused imports / undefined symbols).corepackage confirmed the connect timeout fires (~100ms on a non-routable address), the read timeout fires (~500ms server delay), andresolveTimeoutprefers the per-call override.Notes / residual approximation
writeand the body-read portion ofreadare approximations becausenet/httpexposes no first-class client-side read/write socket deadlines. They are implemented via a customRoundTripper+ idle-deadlineio.ReadCloserwrappers on the request/response bodies.connect(dialer + TLS handshake) and the response-header portion ofreadare exact.Generated with Claude Code