fix: make read deadline interrupt an in-flight blocked stream read - #14
Conversation
streamConn.Read checked the deadline once on entry and then blocked in rwc.Read (an HTTP/2 resp.Body read fed by a background frame-reader goroutine). A deadline set before the block only flipped a boolean that nothing rechecked, so SetReadDeadline could not interrupt a stalled read: a server that completed the handshake and then went silent (e.g. under DPI throttling) held the connection — and its slot in the client's healthy-outbound pool — indefinitely. Replace the boolean deadline with a channel-based one and give streamConn a single reader-pump goroutine. Read now selects the pump's output against the deadline and the done channel, so an expired deadline returns a timeout promptly and the timeout stays recoverable (a later SetReadDeadline + Read succeeds). The pump is the sole reader of the stream body, which also lets Close keep draining to EOF rather than aborting with an RST_STREAM — preserving the existing anti-fingerprint close behavior, now centralized in the pump instead of a separate drain goroutine in h2StreamRWC.Close. This is the samizdat side of the client-info OK-read stall; the injector deadline in lantern-box (getlantern/lantern-box#291) is a no-op for samizdat without it because the stream conn's deadline did not propagate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesStream lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ReadCaller
participant StreamConn
participant readLoop
participant UnderlyingRWC
participant pipeDeadline
ReadCaller->>StreamConn: Read(buffer)
StreamConn->>pipeDeadline: wait()
readLoop->>UnderlyingRWC: Read(pooled buffer)
UnderlyingRWC-->>readLoop: data and terminal error
readLoop-->>StreamConn: readResult
StreamConn-->>ReadCaller: buffered data or error
pipeDeadline-->>StreamConn: deadline expiry
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes a long-standing issue where streamConn.SetReadDeadline could not interrupt an already-blocked Read on HTTP/2 stream bodies, by introducing a channel-observable deadline and a single read-pump goroutine that allows Read to select on deadline expiry.
Changes:
- Replace the prior boolean-based deadline tracking with a
pipeDeadlinethat exposes expiry via a channel. - Introduce a dedicated read-pump goroutine in
streamConnand updateRead/Closeto support interruptible reads and drain-to-EOF close behavior. - Add tests validating deadline interruption/recovery and correct delivery of data split across multiple caller buffers.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| streamconn.go | Implements channel-based deadlines plus a read-pump-driven Read that can be interrupted by SetReadDeadline, and updates close/drain behavior. |
| samizdat_test.go | Adds unit tests covering blocked-read interruption and split-buffer delivery correctness. |
| h2transport.go | Simplifies h2StreamRWC.Close now that draining is centralized in streamConn’s read pump. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
readLoop sent data and a terminal error as two separate channel sends. When a read returned both n>0 and an error (e.g. final read with io.EOF), a caller that consumed the data but never read again or closed would leave the pump blocked forever on the second send. Deliver data and error in one readResult and exit after a single send. Read now holds the terminal error in a sticky readErr, so reads past EOF return the error (matching io.Reader) instead of blocking on the exited pump, and buffered data is fully drained before the error surfaces. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 14 minutes. |
net.Conn permits concurrent method calls, so guard readBuf/readErr with a readMu instead of assuming callers serialize Read. Uncontended on the single-reader relay path; removes the data-race window on concurrent Read. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
- Pool the pump's read buffers (sync.Pool) instead of allocating a
readChunkSize buffer per read; Read copies out and returns the buffer.
The rare short-read tail is copied into readBuf so the pooled buffer can
be reused without racing the pump's next read.
- Funnel rwc.Close through rwcCloseOnce so the drain force-timer and the
post-drain goroutine can't close a generic ReadWriteCloser twice.
- Return <-chan struct{} from pipeDeadline.wait (receive-only).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
streamconn.go:168
- After Close(), streamConn no longer closes the underlying rwc synchronously (it drains/force-closes asynchronously). Write() doesn’t check sc.closed/sc.done, so a write-after-close can still succeed (notably for rwc implementations without CloseWrite, e.g. net.Pipe), violating net.Conn semantics and potentially sending data after the caller believes the connection is closed. Add an early closed check in Write and return net.ErrClosed (or a consistent closed error) when closed is set.
func (sc *streamConn) Write(b []byte) (int, error) {
select {
case <-sc.writeDeadline.wait():
return 0, &timeoutError{}
default:
}
if sc.shaper != nil {
return sc.shaper.Write(sc.rwc, b)
}
return sc.rwc.Write(b)
Close now drains the response body to EOF (bounded by drainForceTimeout) before closing it, and documents why: closing resp.Body with data pending makes net/http2 send RST_STREAM, an abrupt reset a censor could fingerprint, versus a clean END_STREAM. This restores the self-safety the pre-pump code had, so a direct caller of Close no longer risks an RST. To avoid a second reader racing the pump, the force path is split: the streamConn force-timer now calls abort() (raw reader close) to unblock a stalled pump, and only the finalize goroutine — after the pump has exited — runs the graceful drain-and-close via closeRWC. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
If rwc has no CloseWrite, streamConn.Close can't elicit a clean remote EOF, so draining is pointless and the pump would stay blocked until the force timer fires (delaying resource release and lingering a goroutine). Close rwc right away in that case. The production H2 stream always supports CloseWrite, so the graceful drain path is unchanged; this only affects plain conns (e.g. tests). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
streamconn.go (1)
187-198: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSingle-shot force path: if
abort()doesn't unblock the pump,rwcis never closed.
forceTimerfires once and, for areadAborter, only closes the read side. If that read stays blocked,pumpDonenever closes, so the goroutine parks forever andcloseRWCnever runs (fd + goroutine leak). Also noteClosereturnsnilunconditionally, discardingrwc.Close's error.♻️ Suggested fallback after abort
- forceTimer := time.AfterFunc(drainForceTimeout, sc.abortRWC) + forceTimer := time.AfterFunc(drainForceTimeout, func() { + sc.abortRWC() + // Last resort: if aborting the reader didn't unblock the pump, + // close the whole rwc so the pump returns and the fd is released. + time.AfterFunc(drainForceTimeout, sc.closeRWC) + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@streamconn.go` around lines 187 - 198, Update the abort/close flow around forceTimer and the pumpDone goroutine so a stalled pump cannot prevent rwc from eventually being closed: add a fallback after sc.abortRWC that closes rwc when the pump remains blocked, while preserving the existing graceful close when pumpDone completes. Also propagate the error from rwc.Close through Close instead of returning nil unconditionally.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@streamconn.go`:
- Around line 159-163: Update streamConn.Write to check the connection’s closed
state before writing, and return net.ErrClosed without delivering data when
Close has been called, including for rwc implementations without CloseWrite.
Coordinate this check with the existing close synchronization used by streamConn
so writes cannot race with Close.
- Around line 92-96: Change the buffer pool used by the stream connection read
path to store *([]byte) values instead of []byte, updating its initialization,
Get/Put calls, and all related helpers such as the read result handling.
Dereference pooled buffers for Read and copy operations, then return the pointer
to the pool at every existing release site, including the zero-byte read path in
the shown logic.
---
Nitpick comments:
In `@streamconn.go`:
- Around line 187-198: Update the abort/close flow around forceTimer and the
pumpDone goroutine so a stalled pump cannot prevent rwc from eventually being
closed: add a fallback after sc.abortRWC that closes rwc when the pump remains
blocked, while preserving the existing graceful close when pumpDone completes.
Also propagate the error from rwc.Close through Close instead of returning nil
unconditionally.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d4963535-7de7-4e7c-b3d3-b356a514a9dd
📒 Files selected for processing (3)
h2transport.gosamizdat_test.gostreamconn.go
- Read returns (0, nil) immediately for a zero-length buffer instead of blocking on the pump/deadline/done (io.Reader/net.Conn convention). - Pool *[]byte rather than []byte so Put doesn't box a slice into an interface and allocate every time (staticcheck SA6002). - Write returns net.ErrClosed once done is closed, so a write after Close fails regardless of whether rwc's own write side was closed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
Read checked done only in its final select, so after Close it could still return buffered tail bytes or the sticky readErr from the early-return paths. Check done first and return net.ErrClosed (matching Write and net.Conn semantics), and return net.ErrClosed from the select's done case too for consistency. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
Pulls in getlantern/samizdat#14, which makes streamConn's read deadline actually interrupt a blocked stream read. Without it, the client-info OK-read deadline added in this PR is a silent no-op for the samizdat transport (its stream conn's deadline was only an entry-time guard). The two changes are the two halves of the same client-info-stall fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ng (#291) * fix(tracker): bound client-info OK exchange and fix short-read handling The post-handshake CLIENTINFO/OK exchange runs on the connection's critical path (sing-box calls ConnHandshakeSuccess before starting the copy goroutines), where dial-layer timeouts no longer apply. Three fixes: - Set a 10s deadline around the exchange in both sendInfo variants, cleared afterward so the long-lived piped conn is unaffected. A server that stalls after the handshake (e.g. under DPI throttling) previously held the flow and its healthy-pool slot for 10+ minutes. - TCP: read the OK response with io.ReadFull; a legal short read previously failed the exchange as "invalid response", fatally tearing down a healthy connection. - UDP: read the response into a datagram-sized buffer and compare the prefix; wrapped conns return io.ErrShortBuffer for a 2-byte buffer whenever the reply carries transport overhead, killing successful QUIC/UDP handshakes (getlantern/engineering#3718). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * address review: use %q on a slice for the invalid-response error Formatting the [2]byte array with %s produced a malformed message (%!s([2]uint8=...)). Slice it and use %q so unexpected bytes are readable. Applied to both the TCP and UDP sendInfo paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * deps: bump samizdat to v0.0.3-0.20260724223841-a5ee9ab56830 Pulls in getlantern/samizdat#14, which makes streamConn's read deadline actually interrupt a blocked stream read. Without it, the client-info OK-read deadline added in this PR is a silent no-op for the samizdat transport (its stream conn's deadline was only an entry-time guard). The two changes are the two halves of the same client-info-stall fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Pulls in getlantern/lantern-box#291 (client-info OK-read deadline + short-read fixes) and the transitive getlantern/samizdat#14 (read deadline actually interrupts a blocked stream read). Together these stop a proxy that stalls after the handshake — e.g. Rostelecom/DPI throttling in Russia — from holding a flow and its healthy-outbound-pool slot for minutes instead of failing fast. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Problem
streamConn.Readchecked the read deadline once on entry and then blocked insc.rwc.Read— an HTTP/2resp.Bodyread fed by a background frame-reader goroutine. The deadline timer only flipped a boolean that nothing rechecked mid-read, soSetReadDeadlinecould not interrupt an already-blocked read.Impact: a server that completes the TLS+H2 handshake and then goes silent (the Rostelecom / DPI-throttling failure mode in Freshdesk #180563) held the connection — and its slot in the client's healthy-outbound pool — for as long as the caller waited, with no way to time out.
This is the samizdat half of the client-info
OK-read stall. The injector-side deadline added in getlantern/lantern-box#291 is a silent no-op for samizdat without this change, because the deadline never reached anything that could interrupt the read. (Audited across all Lantern transports; samizdat was the only one whereSetDeadlinedid not interrupt a blocked read.)Fix
deadlineTimerwith a channel-based deadline (the standard library'snet.Pipedeadline shape) whose expiry is observable on a channel.streamConna single reader-pump goroutine that owns all reads from the underlying stream and delivers chunks over an unbuffered channel (one-chunk backpressure).Readnowselects the pump output against the deadline and adonechannel.Closekeeps draining to EOF rather than aborting the stream with anRST_STREAM— preserving the existing deliberate anti-fingerprint close behavior, now centralized in the pump instead of a separate drain goroutine inh2StreamRWC.Close.SetWriteDeadlineremains an entry-time guard (unchanged); the stall this fixes is on the read side.Testing
TestStreamConnDeadlineInterruptsBlockedRead(deadline interrupts a never-answered read, then recovers),TestStreamConnReadDeliversDataSplitAcrossBuffers(partial-chunk remainder survives to the next read).TestStreamConn*/TestH2StreamRWC*/ integration tests unchanged and green.go test -race ./...clean;go build,go vet,gofmtclean.Follow-up
Once merged, bump samizdat in
lantern-box(then radiance → lantern,go mod tidyat each step) so the fix reaches a 9.1.x client build. Pairs with lantern-box#291.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
io.EOF, without unnecessary blocking.Tests