fix(triple): make stream response close non-blocking#3466
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request updates the Triple stream client close lifecycle so that closing the read/response side is non-blocking: duplexHTTPCall.CloseRead() now closes the HTTP response body directly rather than draining it, preventing hangs when the peer keeps the response stream open or stops producing bytes. It also adds regression tests covering the affected public close paths across server-streaming, bidi-streaming, client-streaming, and the underlying duplex call.
Changes:
- Change
duplexHTTPCall.CloseRead()to closeresponse.Bodywithout draining it first. - Add new external tests validating that stream close paths return promptly even when the server keeps streams open or returns early.
- Add a focused unit regression test to ensure
CloseRead()does not attempt to read/drain the response body.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| protocol/triple/triple_protocol/duplex_http_call.go | Makes CloseRead() non-blocking by closing the response body directly. |
| protocol/triple/triple_protocol/duplex_http_call_test.go | Adds a unit regression test ensuring CloseRead() doesn’t drain and doesn’t block. |
| protocol/triple/triple_protocol/stream_close_ext_test.go | Adds end-to-end regression tests for non-blocking close behavior across stream types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| select { | ||
| case err := <-done: | ||
| return err | ||
| case <-time.After(200 * time.Millisecond): |
There was a problem hiding this comment.
这里也把本次pr 其他 200 * time.Millisecond 的改成了 time.Second，避免测试超时时间过短
| body.unblock() | ||
| assert.Nil(t, <-done) | ||
| t.Fatal("CloseRead attempted to drain the response body") | ||
| case <-time.After(200 * time.Millisecond): |
|
|
||
| stream, err := client.CountUp(context.Background(), triple.NewRequest(&pingv1.CountUpRequest{})) | ||
| assert.Nil(t, err) | ||
| assert.True(t, stream.Receive(&pingv1.CountUpResponse{})) |
There was a problem hiding this comment.
[P1] 这里把它当作验证 close 行为的回归测试，但我在当前 PR 分支本地执行作者自己在 PR 描述里给出的命令后，这条用例会先在 assert.True(t, stream.Receive(...)) 失败，根本还没进入 stream.Close() 路径。这样它不能证明本次 CloseRead 改动的高层行为。建议先修正这个测试的建模/初始化条件，确保它至少能稳定收第一条消息，再用它验证 close 不阻塞。
There was a problem hiding this comment.
应该是这个测试的建模比较依赖调度顺序，我会先去把 setup 和 close assertion 拆开
|
|
||
| stream, err := client.CumSum(context.Background()) | ||
| assert.Nil(t, err) | ||
| assert.Nil(t, stream.Send(&pingv1.CumSumRequest{Number: 2})) |
There was a problem hiding this comment.
[P1] 这条回归测试当前在本地并不能走到 CloseResponse()：stream.Send(...) 这里直接返回 unknown: write envelope: EOF。既然失败点发生在 close 前，这个用例就不能作为“close 不再 drain 响应体”的验证证据。建议先把 bidi 场景下首个 request/response 往返稳定下来，再断言 CloseResponse() 的非阻塞行为。
|
|
||
| stream, err := client.CumSum(context.Background()) | ||
| assert.Nil(t, err) | ||
| assert.Nil(t, stream.Send(&pingv1.CumSumRequest{Number: 1})) |
There was a problem hiding this comment.
[P1] 同样的问题，这条用例在当前 PR 分支本地会先失败在 stream.Send(...)，错误是 unknown: write envelope: EOF，还没进入后面的 stream.Receive(...) / CloseResponse() 路径。现在这条测试验证不到 PR 想修的生命周期问题。建议把“server stopped reading”场景和 close 行为拆开：先保证 client 能稳定拿到服务端错误，再验证随后 CloseResponse() 不会阻塞。
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3466 +/- ##
===========================================
+ Coverage 46.76% 53.88% +7.11%
===========================================
Files 295 461 +166
Lines 17172 35476 +18304
===========================================
+ Hits 8031 19117 +11086
- Misses 8287 14853 +6566
- Partials 854 1506 +652 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| // CloseRead is a cleanup path. Do not drain the response body here: if the | ||
| // peer stops writing or keeps the stream open, draining can block close | ||
| // indefinitely. Callers that need final trailers should receive until EOF. |
There was a problem hiding this comment.
这种一看就是ai生成的comment要注意一下人类能不能看懂
因为人是不知道之前这里删了代码的
|
What
duplexHTTPCall.CloseRead()to close the response body directly instead of draining the remaining response body first.duplexHTTPCall.Why
CloseRead()previously drained the remaining response body before closing it. If the server kept the response stream open, returned an error early, stopped reading the request side, or the transport stopped producing response bytes, client close paths such asServerStreamForClient.Close,BidiStreamForClient.CloseResponse, andClientStreamForClient.CloseAndReceivecould block unnecessarily.This selectively ports only the stream close lifecycle behavior from the connect-go v1.18.0 fix without bringing in unrelated connect-go features.
Fixes #3457
Tests