diff --git a/main.go b/main.go index 7c6091f..6337d7e 100644 --- a/main.go +++ b/main.go @@ -140,7 +140,7 @@ func runTUI(seed *cliArgs) int { client := newClient(logger, voice) if seed != nil && seed.timeout > 0 { - client.http.Timeout = seed.timeout + client.SetTimeout(seed.timeout) } m := newModel(client, logger, dbg) @@ -248,7 +248,7 @@ func runCLI(a cliArgs) int { client := newClient(logger, errorChanFor(persona)) if a.timeout > 0 { - client.http.Timeout = a.timeout + client.SetTimeout(a.timeout) } // Stream the body when it isn't being rendered: to -o FILE when asked, or @@ -825,7 +825,10 @@ OPTIONS string a literal body (if -d is omitted and stdin is piped, the pipe is the body) -X, --request METHOD set the method explicitly - --timeout DUR request timeout, e.g. 10s, 500ms (default 30s) + --timeout DUR request timeout, e.g. 10s, 500ms (default 30s). For + streamed bodies (-o / piped) the default bounds the wait + for headers, not the transfer; an explicit --timeout + caps the whole transfer -v, --stats print a timing breakdown (dns/tcp/tls/send/wait/recv) and the negotiated TLS to stderr, even when piping --pretty force the pretty/colored body view (the default at a TTY: diff --git a/request.go b/request.go index 2a5563b..af6e616 100644 --- a/request.go +++ b/request.go @@ -156,19 +156,41 @@ func (r Result) bodySize() int64 { // Client is the single component that executes requests and handles their // results. Construct it once per process with the chosen logger and ErrorChan. type Client struct { - http *http.Client - log *log.Logger - voice ErrorChan + http *http.Client + stream *http.Client // BodySink requests: bounds time-to-headers, not the body copy + log *log.Logger + voice ErrorChan + + // explicitTimeout records that the user passed --timeout. An explicit + // value is a total cap, so Do keeps it on streamed transfers too; only + // the DEFAULT timeout steps aside for a BodySink. + explicitTimeout bool } func newClient(logger *log.Logger, voice ErrorChan) *Client { + // http.Client.Timeout covers reading the entire body, which would kill a + // streamed download (-o / piped) at 30s mid-transfer — and streaming + // exists precisely for arbitrarily large bodies. The stream client moves + // the default timeout to the response headers instead (the cloned default + // transport already carries a 30s dial timeout) and leaves the body copy + // unbounded, like curl -o. + tr := http.DefaultTransport.(*http.Transport).Clone() + tr.ResponseHeaderTimeout = defaultTimeout return &Client{ - http: &http.Client{Timeout: defaultTimeout, CheckRedirect: redirectPolicy}, - log: logger, - voice: voice, + http: &http.Client{Timeout: defaultTimeout, CheckRedirect: redirectPolicy}, + stream: &http.Client{Transport: tr, CheckRedirect: redirectPolicy}, + log: logger, + voice: voice, } } +// SetTimeout applies a user-supplied --timeout. Explicit means "cap the whole +// request", so unlike the default it also bounds streamed (BodySink) transfers. +func (c *Client) SetTimeout(d time.Duration) { + c.http.Timeout = d + c.explicitTimeout = true +} + // envHeaderKeysCtx carries spec.envHeaderKeys on the request context so // redirectPolicy can see which headers were env-injected — CheckRedirect // requests inherit the original request's context. @@ -262,9 +284,16 @@ func (c *Client) Do(spec RequestSpec) Result { tr := &reqTrace{} req = req.WithContext(httptrace.WithClientTrace(req.Context(), tr.clientTrace())) + // Streaming with the default timeout: bound the wait for headers, never + // the body copy. An explicit --timeout keeps the total cap even here. + httpc := c.http + if spec.BodySink != nil && !c.explicitTimeout { + httpc = c.stream + } + start := time.Now() tr.start = start - resp, err := c.http.Do(req) + resp, err := httpc.Do(req) if err != nil { dur := time.Since(start) res.Timing.Total = dur // no trace events fired; record the wall time at least diff --git a/request_test.go b/request_test.go index 2c803b7..285de63 100644 --- a/request_test.go +++ b/request_test.go @@ -10,6 +10,7 @@ import ( "path/filepath" "strings" "testing" + "time" log "charm.land/log/v2" ) @@ -310,6 +311,83 @@ func TestClientDoStreamsToSinkUncapped(t *testing.T) { } } +// dripServer sends headers immediately, then dribbles the body one byte per +// sleep, so the full transfer takes chunks*pace — long enough to outlast a +// tiny client timeout without slowing the test suite down. +func dripServer(t *testing.T, chunks int, pace time.Duration) *httptest.Server { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + fl := w.(http.Flusher) + w.WriteHeader(http.StatusOK) + fl.Flush() + for range chunks { + time.Sleep(pace) + if _, err := w.Write([]byte("x")); err != nil { + return // client gave up (the explicit-timeout case) + } + fl.Flush() + } + })) + t.Cleanup(srv.Close) + return srv +} + +// The DEFAULT timeout must not cap a streamed body copy: headers arrive +// instantly, the transfer outlasts the client timeout, and the stream still +// completes — that's the whole point of the BodySink path (curl -o). +func TestClientDoStreamDefaultTimeoutDoesNotCapBody(t *testing.T) { + srv := dripServer(t, 8, 30*time.Millisecond) // ~240ms transfer + + c := testClient() + c.http.Timeout = 50 * time.Millisecond // a tiny "default" timeout + + var sink bytes.Buffer + res := c.Do(RequestSpec{Method: "GET", URL: srv.URL, BodySink: &sink}) + + if !res.OK() { + t.Fatalf("streamed transfer was killed by the default timeout: %v", res.Err) + } + if sink.Len() != 8 { + t.Errorf("sink got %d bytes, want 8", sink.Len()) + } +} + +// An explicit --timeout is a total cap, so it still kills a too-slow streamed +// transfer. +func TestClientDoStreamExplicitTimeoutCapsBody(t *testing.T) { + srv := dripServer(t, 8, 30*time.Millisecond) + + c := testClient() + c.SetTimeout(50 * time.Millisecond) + + var sink bytes.Buffer + res := c.Do(RequestSpec{Method: "GET", URL: srv.URL, BodySink: &sink}) + + if res.Err == nil { + t.Error("explicit timeout should kill a slow streamed transfer") + } + if res.DisplayErr == "" { + t.Error("DisplayErr should be set when the explicit timeout fires") + } +} + +// Buffered (non-sink) requests keep the full-transfer timeout regardless. +func TestClientDoBufferedTimeoutStillCapsBody(t *testing.T) { + srv := dripServer(t, 8, 30*time.Millisecond) + + c := testClient() + c.http.Timeout = 50 * time.Millisecond + + res := c.Do(RequestSpec{Method: "GET", URL: srv.URL}) + + if res.Err == nil { + t.Error("default timeout should still cap a buffered transfer") + } + if res.DisplayErr == "" { + t.Error("DisplayErr should be set when a buffered transfer times out") + } +} + func TestParseSize(t *testing.T) { good := map[string]int64{ "1048576": 1 << 20,