Fix/pr107 bad stream - #111
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan includes up to 4 reviews per rolling hour; 3 remain after this review. 📝 WalkthroughWalkthroughTCP connections now distinguish codec-backed streams from raw connections. Codec reads and writes apply stall handling and latch fatal stream failures. Flate and snappy writers support batched payloads with one flush. Tests cover codec, raw, stalled, idle, timeout, close, and configuration behavior. ChangesTCP codec stream handling
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The change makes compressed single-buffer and batched sends use consistent framing. The only remaining item is a trivial test-listener cleanup that does not create a merge-blocking product or production risk. Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Sender
participant TCPConnection
participant CodecWriter
participant PeerSocket
Sender->>TCPConnection: Send batch
TCPConnection->>CodecWriter: WriteBuffers or write each buffer
CodecWriter->>PeerSocket: Encode and flush payload
PeerSocket-->>TCPConnection: Timeout or stream error
TCPConnection->>PeerSocket: Close socket on fatal codec timeout
TCPConnection-->>Sender: Return ErrCodecStreamBroken
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
transport/connection.go (1)
397-402: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a wire-format regression test.
The current
transport/client_test.gocoverage checks return values and counters. It does not verify that a codec peer can decode bytes sent through bothSend([]byte)andSend([][]byte)afterSetCompressType(CompressNone).Add a test with codec-enabled peers. Send a single payload and a batch. Read and compare the exact combined payload on the peer. This test must fail if the batch path writes directly to
t.conn.🤖 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 `@transport/connection.go` around lines 397 - 402, Add a codec-enabled regression test in transport/client_test.go that configures both peers with SetCompressType(CompressNone), sends one payload via Send([]byte) and another via Send([][]byte), then reads from the peer and compares the exact combined wire payload. Ensure the test exercises the codec peer path so it fails when batched data bypasses the writer and writes directly to t.conn.
🤖 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.
Nitpick comments:
In `@transport/connection.go`:
- Around line 397-402: Add a codec-enabled regression test in
transport/client_test.go that configures both peers with
SetCompressType(CompressNone), sends one payload via Send([]byte) and another
via Send([][]byte), then reads from the peer and compares the exact combined
wire payload. Ensure the test exercises the codec peer path so it fails when
batched data bypasses the writer and writes directly to t.conn.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 43bc8db9-437e-4ac2-bac1-54fc2051defd
📒 Files selected for processing (1)
transport/connection.go
There was a problem hiding this comment.
Pull request overview
This PR fixes TCP stream corruption when SetCompressType(CompressNone) is used by distinguishing “raw connection” vs “codec-installed connection” and ensuring the Send([][]byte) fast path does not bypass the compression writer. It also introduces batched write+single-flush support for flate/snappy writers to improve throughput when sending [][]byte under compression.
Changes:
- Re-introduces an
isCompressedflag to track whether a codec (flate/snappy) has been installed, sinceCompressNonecan still install a flate codec. - Updates TCP read/write deadline behavior to only apply on truly raw connections (avoids deadlines on codec streams).
- Adds
WriteBuffers([][]byte)support to compression writers and uses it fromSend([][]byte)when available.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- rename isCompressed to codecEnabled; SetCompressType(CompressNone) still installs a flate codec stream, so the state must not be named by compression level - keep batch [][]byte sends on the codec writer (single flush) so a codec connection never mixes coded and raw frames (AlexStocks#102/AlexStocks#107) - add wire-format regression tests: mixed []byte and [][]byte sends over a CompressNone codec pair, plus raw writev coverage when SetCompressType was never called - make TestTCPClient's dummy peer a TCP discard server so codec frames do not make an HTTP peer close mid-test - run make fmt; imports are formatter-clean
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@transport/client_test.go`:
- Around line 253-268: Register a t.Cleanup callback immediately after
listenLocalServer succeeds in TestTCPClient to close listener. Ensure the
existing Accept loop exits when the listener is closed, releasing the socket
after the test completes.
🪄 Autofix
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: 6311c0f3-f23e-463d-97f3-7ef6c23e37c1
📒 Files selected for processing (3)
transport/client_test.gotransport/connection.gotransport/connection_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- transport/connection.go
|
已按反馈更新：
本地已用 Go 1.25 跑通 go test ./... 和 make lint（0 issues），提交 8900f19 已推 |
- arm a read deadline on codec streams via CodecStallTimeout (wider than the raw rTimeout poll interval) so a peer that sends half a codec block cannot block recv forever; the write deadline now always reaches the socket, so SetWriteTimeout and WritePkg(pkg, timeout) work on codec connections - on a codec timeout, latch the stream as broken and close the socket: recv/Send fail fast with ErrCodecStreamBroken, the session treats it as fatal and closes/reconnects instead of retrying a dead decoder - keep normal shutdown working: session.stop()'s unblock deadline is passed through on the read path, and CloseConn skips flushing a broken snappy writer - add stalled peer read/write regression tests (flate and snappy) that verify the call returns within the deadline, the codec connection is actually terminated, and idle/clean-close behavior is preserved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
transport/connection_test.go (1)
569-594: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the comment with the close semantics this test produces.
server.CloseConn(0)callsSetLinger(0), so the peer aborts the connection with RST. The client read then usually fails withECONNRESET, notio.EOF. The assertions remain correct, because the test only checks that the error is neitherErrCodecStreamBrokennor latched. The comment "must keep its own EOF-family error" describes a different case.Either update the comment, or use a graceful close so the test really covers the EOF path.
♻️ Optional: assert the graceful-close path explicitly
- server.CloseConn(0) + // close without SetLinger(0) so the peer performs a graceful FIN and + // the reader observes an EOF-family error rather than a reset. + _ = server.conn.Close()🤖 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 `@transport/connection_test.go` around lines 569 - 594, Align TestCodecRecvPeerCloseKeepsErrorIdentity with the actual close behavior: either replace server.CloseConn(0) with the existing graceful-close operation so the test exercises EOF semantics, or revise the test comment to describe the ECONNRESET/RST path produced by CloseConn(0). If retaining the current close, remove the EOF-specific wording while preserving the assertions that the error is not ErrCodecStreamBroken and codecBroken remains unset.
🤖 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 `@transport/connection.go`:
- Around line 216-223: Synchronize SetCompressType with session startup by
protecting codecEnabled, reader, and writer installation against concurrent
recv, Send, and the session.run read goroutine; alternatively, reject
SetCompressType calls after startup and document that restriction. Ensure codec
state cannot be replaced while active I/O accesses it.
- Around line 584-591: Synchronize access to t.conn between codecIOError and
session.gc: protect every read and write with the connection’s existing
synchronization, or capture an immutable connection reference and track shutdown
separately with close-once state. Update the relevant connection-close paths,
including CloseConn, so concurrent shutdown cannot race with codecIOError.
---
Nitpick comments:
In `@transport/connection_test.go`:
- Around line 569-594: Align TestCodecRecvPeerCloseKeepsErrorIdentity with the
actual close behavior: either replace server.CloseConn(0) with the existing
graceful-close operation so the test exercises EOF semantics, or revise the test
comment to describe the ECONNRESET/RST path produced by CloseConn(0). If
retaining the current close, remove the EOF-specific wording while preserving
the assertions that the error is not ErrCodecStreamBroken and codecBroken
remains unset.
🪄 Autofix
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: ca872ddf-b666-47a1-886f-dbdb75ff74da
📒 Files selected for processing (2)
transport/connection.gotransport/connection_test.go
|
please handle the rabbit's comment. |
SetCompressType raced with concurrent Send/recv on reader/writer, and even a serialized mid-stream switch would desynchronize the peer's decoder since there is no codec renegotiation. Guard the codec fields with a mutex and freeze them once the first recv/Send marks the stream as started; a late SetCompressType now panics (logged and documented), matching how the method already reports an illegal compress type. recv/Send snapshot the codec state under the lock and never hold it across blocking IO. CloseConn used to nil t.conn while codecIOError/recv/Send read it from other goroutines. Keep the conn reference immutable and make closing idempotent via sync.Once instead, for the UDP conn as well. Add race regression tests: a late SetCompressType must panic on a started stream, and SetCompressType racing with Send must be rejected without a data race while the raw stream stays intact.
|
使用"连接状态机 + 拒绝启动后切换"方案——codec 配置在第一次 recv/Send 后冻结,晚到的调用 panic;t.conn 改为不可变引用 + sync.Once 关闭;并发回归测试已加,go test -race 验证无竞态。 |
023aef3 to
3f8fe74
Compare
|
用 Claude Fable 5.0 扫了一批新 issue 出来，我审核了以后没问题提上来了，如果这个 PR 没问题合并后，我着手一个一个解决 |
…ebug log TestTCPClient never closed its listener, leaving the accept goroutine blocked in Accept() for the rest of the test binary (review P2 by @AlexStocks). The [][]byte send path logged the never-assigned length variable instead of the actual written byte count and formatted a nil error with %s (flagged by copilot review).
本 PR 修复 setCompress 出现坏流的现象
SetCompressType(CompressNone)
会走进SetCompressType的第一个 case， 安装 flate reader/writer —— 因为CompressNone == flate.NoCompression == 0`，级别 0 仍然产生 deflate 分帧。
但
Send里[][]byte的分支用t.compress == CompressNone判断是否走net.Buffers裸写快路径，对这类连接判断为真，于是：Send([]byte)（单包）走t.writer，输出是 deflate 帧Send([][]byte)（批量，即WriteBytesArray）裸写 socket，输出无帧同一条连接上两种格式，对端 flate reader 报
flate: corrupt input before offset N。现象是单发正常、批量发送才坏。
Fixes #110
加回 #102 删去的 isCompressed 字段
将 send 函数的 for _,b := range buffers 下沉为接口，并提供批量 flush，效率显著提升，以下是 banchmark 的结果
Summary by CodeRabbit
Performance
Reliability