fix(intra): don't discard rwext read/write deadlines when TCP_USER_TIMEOUT is set - #249
fix(intra): don't discard rwext read/write deadlines when TCP_USER_TIMEOUT is set#249varunagarwal-pro wants to merge 1 commit into
Conversation
…MEOUT is set forward() unwrapped the remote conn from its rwext deadline wrapper whenever rwext.SetTimeout() reported didSet=true, treating a successfully-applied low-level sockopt as equivalent to having a software read/write deadline in place. SetTimeout() only sets TCP_USER_TIMEOUT via core.SetTimeoutSockOpt(). TCP_USER_TIMEOUT bounds how long unacknowledged outbound data may go unacked before the kernel force-closes the connection - it has no effect on a blocking Read() that is simply waiting to receive more data from a peer that has gone idle without sending RST/FIN. It does not implement a receive/idle timeout. Once remote was unwrapped, the only mechanism that could bound such a Read() - rwext's extendr()/extendw(), which apply Go's real per-call SetReadDeadline/SetWriteDeadline via settings.DialerOpts - was discarded entirely. As a result, a relayed TCP connection to a peer that silently stops sending (common with some CDN/load-balancer behavior on idle keep-alive connections, or after a NAT/middlebox timeout that never surfaces an RST) blocks forward()'s Read() forever. The socket stays visibly ESTABLISHED with zero rx/tx queue activity indefinitely, and the app-level effect is a permanent hang (e.g., a media player stuck in a buffering state) with no path to recovery short of killing the connection/process. This was reproduced consistently on-device: a live TCP socket to a video CDN would enter this idle-ESTABLISHED state with zero queue bytes and never recover, while process CPU stayed idle (ruling out a busy loop) and DNS/WAN connectivity remained healthy throughout - pointing squarely at a stuck blocking Read() in the relay path. Fix: only unwrap remote from rwext when timeoutsecs <= 0, i.e. when no read/write deadline is configured at all and rwext.Read/Write would be a true no-op. When a positive timeout is configured, keep remote wrapped so extendr()/extendw() continue to enforce a genuine per-call deadline in addition to (not instead of) the TCP_USER_TIMEOUT sockopt optimization.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🔇 Additional comments (1)
📝 WalkthroughWalkthroughThe forwarding path now retains the ChangesForwarding timeout handling
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change preserves deadline enforcement for timed forwarding and is ready to merge after normal checks. 🚥 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 |
|
🔍 OpenCodeReview found 1 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if r, ok := remote.(rwext); ok { | ||
| isrwext = true | ||
| if timeoutsecs, didSet = r.SetTimeout(); didSet || timeoutsecs <= 0 { | ||
| if timeoutsecs, didSet = r.SetTimeout(); timeoutsecs <= 0 { |
There was a problem hiding this comment.
[bug · medium]
Dropping the didSet clause means remote now stays wrapped in rwext for every positive-timeout case (previously TCP with a successfully-set sockopt was unwrapped to *net.TCPConn). This has a side effect beyond the intended deadline fix: rwext embeds net.Conn but does not implement core.DuplexCloser/core.TCPConn (no CloseRead/CloseWrite; see the TODO? var _ core.DuplexCloser = (*rwext)(nil) in rwconn.go). Consequently core.CloseOp(remote, core.CopW/CopR) in upload/download falls through to the io.Closer case and performs a full Close() instead of a TCP half-close, which can abort the opposite direction of the relay (e.g. a client that half-closes after its request body while the server is still sending the response). Suggest making rwext implement CloseRead/CloseWrite (delegating to the underlying conn when it supports it) so per-call deadline extension and graceful half-close can coexist.
1 participant
forward()unwraps the remote conn from itsrwextdeadline wrapper wheneverrwext.SetTimeout()returnsdidSet=true, treating a successfully-applied low-level sockopt as equivalent to a software deadline.SetTimeout()only setsTCP_USER_TIMEOUTviacore.SetTimeoutSockOpt().TCP_USER_TIMEOUTbounds how long unacknowledged outbound data may go unacked before the kernel force-closes the connection - it has no effect on a blockingRead()simply waiting for more data from a peer that has gone idle without sending RST/FIN. It is not a receive/idle timeout.Once unwrapped, the only mechanism that could bound such a
Read()-rwext'sextendr()/extendw(), which apply real per-callSetReadDeadline/SetWriteDeadlineviasettings.DialerOpts- is discarded entirely. A relayed TCP connection whose peer silently stops sending (idle CDN/load-balancer connection reuse, or a NAT/middlebox timeout that never surfaces an RST) then blocksforward()'sRead()forever. The socket staysESTABLISHEDwith zero rx/tx queue activity indefinitely, with no path to recovery short of killing the connection/process.Reproduced consistently on-device: a live TCP socket to a video CDN entered this idle-
ESTABLISHEDstate with zero queue bytes and never recovered, while process CPU stayed idle (ruling out a busy loop) and DNS/WAN connectivity stayed healthy - pointing squarely at a stuck blockingRead()in the relay path.Fix: only unwrap
remotefromrwextwhentimeoutsecs <= 0(i.e. no deadline configured at all, sorwext.Read/Writeis a true no-op). When a positive timeout is configured, keepremotewrapped soextendr()/extendw()continue enforcing a real per-call deadline in addition to (not instead of) theTCP_USER_TIMEOUTsockopt optimization.Verified:
go build/go vetclean forlinux/arm64(matches Android's kernel ABI for the syscalls touched).Summary by CodeRabbit