replaced check IsContextError to check stream context error#2102
replaced check IsContextError to check stream context error#2102asmyasnikov merged 7 commits intomasterfrom
Conversation
|
@copilot добавь тесты для изменения в PR |
summaryInferred base version: v3.135.2 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2102 +/- ##
==========================================
- Coverage 74.82% 72.35% -2.47%
==========================================
Files 427 427
Lines 36902 36902
==========================================
- Hits 27613 26702 -911
- Misses 8090 8998 +908
- Partials 1199 1202 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR updates gRPC client stream error handling to detect stream cancellation/deadline via the stream’s context rather than relying on xerrors.IsContextError(err).
Changes:
- Switched context-error detection in
CloseSend,SendMsg, andRecvMsgfromxerrors.IsContextError(err)tos.streamCtx.Err() != nil. - Added contextual wrapping for errors when the stream context is done.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Agent-Logs-Url: https://github.com/ydb-platform/ydb-go-sdk/sessions/00e0ba42-0e37-44b2-a92c-ac0b35abc8f3 Co-authored-by: asmyasnikov <14202262+asmyasnikov@users.noreply.github.com>
Done in commit f9016a3. Added
Also added a CHANGELOG.md entry at the top of the file. |
Agent-Logs-Url: https://github.com/ydb-platform/ydb-go-sdk/sessions/00e0ba42-0e37-44b2-a92c-ac0b35abc8f3 Co-authored-by: asmyasnikov <14202262+asmyasnikov@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![robot-vibe-db[bot]](https://avatars.githubusercontent.com/in/3319454?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review Summary
Verdict: ✅ No critical issues found
The fix is correct — checking s.streamCtx.Err() directly is more reliable than inspecting the gRPC error type via IsContextError, since gRPC may return non-standard errors (e.g. "connection closed") when the stream context is cancelled. The fmt.Errorf + xerrors.Join wrapping preserves both the original gRPC error and the context error in the unwrap chain, so callers using errors.Is, IsContextError, etc. continue to work.
Critical issues
No critical issues found.
Other findings
- Minor | Medium: The existing
ContextErrortests (e.g.CloseSend/ContextErrorat line 151) usecontext.Background()asstreamCtxwhile having gRPC returncontext.Canceled. With the new code,s.streamCtx.Err()returnsnil, so these tests now fall through to the transport wrapping branch instead of the context-error branch. They still pass becauseIsContextErrortraverses the unwrap chain throughtransportError, but they no longer exercise the intended context detection code path. Consider updating them to use a cancelled stream context. —internal/conn/grpc_client_stream_test.go:164 - Nit | Low: The new
StreamContextDoneReturnsNonTransportErrortests assert!IsTransportErrorandErrorIs(streamErr), but don't assertIsContextError(err) == true. Since the error wrapscontext.Canceledviaxerrors.Join, this property holds — but asserting it would document the contract and catch future regressions. —internal/conn/grpc_client_stream_test.go:209
This review was generated automatically. Critical issues require attention; other findings are advisory.
If this comment was useful, please give it a 👍 — it helps us improve the review bot.
| // When the stream context is done, the error must NOT be wrapped as a | ||
| // transport error regardless of what gRPC returned. | ||
| require.False(t, xerrors.IsTransportError(err)) | ||
| require.ErrorIs(t, err, streamErr) |
There was a problem hiding this comment.
Severity: Nit
Confidence: Low
Consider adding require.True(t, xerrors.IsContextError(err)) here (and in the analogous SendMsg / RecvMsg variants). The returned error wraps context.Canceled via xerrors.Join, so IsContextError should return true — asserting this documents the contract that callers can still detect the error as a context error, not just as "not a transport error".
| @@ -173,6 +174,42 @@ func TestGrpcClientStream_CloseSend(t *testing.T) { | |||
| require.True(t, xerrors.IsContextError(err)) | |||
There was a problem hiding this comment.
Severity: Minor
Confidence: Medium
This existing test now exercises a different code path than before. With the old IsContextError(err) check, context.Canceled from gRPC was caught immediately. With the new s.streamCtx.Err() check, context.Background() returns nil, so the code falls through to transport wrapping — the error is now both IsTransportError and IsContextError.
The assertion still passes (the unwrap chain reaches context.Canceled through transportError), but the test no longer validates the context-detection branch. Consider changing streamCtx to a cancelled context so this test exercises the intended path:
cancelledCtx, cancel := context.WithCancel(context.Background())
cancel()
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Other information