[Arpit-Khandelwal]

Follow-up to #1588, where I mentioned the TimeoutReader overhead.

TimeoutReader starts a goroutine, a channel and a context.WithTimeout for every Read. With the 64 KiB buffers in place, that's the main per-read cost left on fast downloads.

This uses one time.AfterFunc timer per reader instead, armed when Read starts and stopped before it returns, so the timeout still applies to each read and not to time spent between reads (the WriteAt under fileMu).

Timeout action: cancel the request. Every request now gets its own cancellable context, created before it is sent, and its cancel func is passed to the TimeoutReader for that response. When the timer fires it only cancels the request, and the transport interrupts the pending read. The reader never closes the body; that stays with the caller's defer resp.Body.Close(), so there is no second Close racing it. The Resolve request keeps its cancel func next to the kept-open response, because that body outlives Resolve (prefetch or the first connection reads it).

Errors: the timed-out read and every later one return context.DeadlineExceeded, same as before. The reader uses Timer.Stop() to tell whether the timer fired during the read. Any other error, including context.Canceled from the caller's context (pause or stop), is passed through unchanged.

Race fix: a timed-out Read used to return while its goroutine could still write into the caller's buffer, which the connection reuses for the retry. Read now only returns after the underlying read has, because cancelling doesn't prove the read has finished. TestTimeoutReader_TimedOutReadDoesNotOutliveCaller covers this; its first version failed on main.

CPU per GB downloaded. This measures client-process CPU only, with the server in a separate process: 8 connections, runs interleaved across the three builds, median with min–max in brackets. Loopback had 7 runs per build and throttled had 5.

	#1588 (64 KiB buffers)	timer, close body on timeout	timer, cancel request (this PR)
loopback, 2 GiB	0.667 (0.653–0.709)	0.605 (0.579–0.660)	0.629 (0.596–0.664)
throttled 30 MiB/s, 1 GiB	7.61 (6.17–9.01)	5.80 (5.02–6.44)	6.31 (5.17–6.71)

TimeoutReader.Read microbenchmark: ~885 ns, 9 allocs, 584 B per call on #1588, against ~56 ns and 0 allocs for both timer versions. The only added cost is one context.WithCancel per request, not per read.

An earlier measurement of the close version showed a larger gain, but it didn't reproduce on rerun.

Tests:

• a stalled body times out and a slow but steady body doesn't, on the native and req/utls transports over HTTP/1.1 and HTTP/2
• gzip-encoded bodies on the same four clients, one stalling inside the gzip header and one stalling after some compressed data. The close-on-timeout version reported a race in GzipReader.Close (req) and, on Go 1.25.4, in http2gzipReader.Close (native). This version is race-free over 30 separate -race runs on Go 1.25.4 and 1.27.
• cancelling the caller's context after the first bytes are read returns context.Canceled, not DeadlineExceeded, on all four clients
• idle time between reads isn't counted, and the timeout doesn't close the reader
• the outlive-the-caller case above
• go test ./internal/... ./pkg/... passes. go test -race ./internal/protocol/http/ shows the same two pre-existing failures as main (TestFetcher_DownloadResponseBodyReadTimeout and TestFetcher_DownloadWithProxy) with the same race sites, none in timeout_reader.go.

A nil cancel func can't interrupt the read; it still reports the timeout once its Read returns. Every request the fetcher sends passes one.

Note: #1451 removes the prefetch's TimeoutReader, probably because the old one could return before its read finished. With this change that no longer happens, so prefetch can keep responseReader and its stall timeout when #1451 is rebased.

[codecov]

Codecov Report

❌ Patch coverage is 95.83333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.46%. Comparing base (7d423e2) to head (0b1b4a4).

Files with missing lines	Patch %	Lines
internal/protocol/http/fetcher.go	93.93%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1597      +/-   ##
==========================================
+ Coverage   72.30%   72.46%   +0.15%     
==========================================
  Files         102      102              
  Lines       15683    15697      +14     
==========================================
+ Hits        11340    11375      +35     
+ Misses       3109     3094      -15     
+ Partials     1234     1228       -6     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[monkeyWie]

Thanks for the optimization and the additional tests! Reusing a timer and keeping the underlying Read synchronous are good improvements, especially for avoiding writes into the caller's buffer after a timeout.

Could you change the timeout action to cancel the HTTP request, rather than closing the response body from the timer callback?

There is a race in the current close-based approach: the callback can unblock the underlying Read before its own Body.Close() has finished. Read then returns, and the caller's existing defer resp.Body.Close() can run concurrently with that callback. For an AfterFunc timer, Stop() returning false does not wait for the callback to finish (Go docs).

I reproduced this on Go 1.25.4 with the existing req fixed/http2 client: send a gzip-compressed chunk, flush the gzip writer and HTTP response, then stall without finishing the gzip stream. Reading it through TimeoutReader and closing the response afterward produces this race under -race:

compress.(*GzipReader).Close()  // gzip_reader.go:39, writes zerr
  TimeoutReader.expire()
vs.
compress.(*GzipReader).Close()  // same write
  readBodyWithTimeout.deferwrap1()

With the added gzip case repeated 30 times, the old TimeoutReader passed and this implementation reported the race.

The approach I'd suggest is:

• Create a cancellable context once per HTTP request, before sending it, and bind it to that actual request.
• Keep the reusable timer armed only while a Read is in progress, as in this PR. On timeout, have it call the request's cancel function.
• Keep the underlying Read synchronous and wait for it to return before returning to the caller; cancellation itself is not proof that the read has finished.
• Preserve context.DeadlineExceeded for the read timeout, distinguish it from caller cancellation, and leave response-body closing to the caller.

This retains the allocation/goroutine improvements and the buffer-lifetime fix, while letting the HTTP transport interrupt network I/O without a second concurrent Body.Close() path. It would be useful to extend the native/req HTTP/1.1 and HTTP/2 tests with both an incomplete gzip header and a gzip body that stalls after some data, while keeping the idle-between-reads coverage.

[Arpit-Khandelwal]

@monkeyWie thanks for the detailed repro, you're right about the race. Updated as you suggested:

• Every request now gets its own cancellable context before it's sent (downloadChunkOnce, runConnectionFallback, and Resolve, which keeps its cancel func next to the kept-open response). The timer is armed only during a Read and on timeout only calls that cancel func. The reader never closes the body; that stays with the caller.
• Read stays synchronous and returns only after the underlying read has returned.
• A read timeout returns context.DeadlineExceeded (decided by Timer.Stop()); caller cancellation comes through as context.Canceled, with a test on all four clients.
• Added the gzip cases on native and req over HTTP/1.1 and HTTP/2: one stalling inside the gzip header, one stalling after some compressed data. On the close-based version they reproduce your race in 30/30 separate -race runs (req GzipReader.Close, and on Go 1.25.4 also native http2gzipReader.Close). With this change: 0/30 on Go 1.25.4 and 1.27. The idle-between-reads coverage is kept.

I also reran the CPU numbers with a cleaner harness (server in a separate process, interleaved runs) and updated the table in the description. The gain over #1588 is smaller than I first reported: about 6% on loopback and 17% throttled, with Read itself going from ~885 ns / 9 allocs to ~56 ns / 0 allocs.

[monkeyWie]

Thanks for making the cancellation changes and adding the gzip coverage! The updated timeout-reader tests passed locally with -race -count=3 on Go 1.25.4 and 1.26.2, and the HTTP fetcher/client package tests also passed on Go 1.25.4.

GitHub currently reports merge conflicts with main. Could you please update the branch against the latest main, resolve the conflicts, and push the result so CI can run on the updated code? We'll take another look once that's ready.

[Arpit-Khandelwal]

@monkeyWie thanks for testing it. Merged the latest main and pushed. The only conflict was in runConnectionFallback: main now calls downloadChunkOnce there instead of the inline request, and downloadChunkOnce already creates the per-request cancel on this branch, so I took main's side. Both remaining request sites (Resolve and downloadChunkOnce) pass their cancel func to the reader.

After the merge, go test ./internal/... ./pkg/... passes, the TestTimeoutReader* tests pass under -race, and the gzip and caller-cancellation tests are race-free over 30 separate -race runs (Go 1.27.0).