Draft PR (no merge!!!): check CI after remove line with canceling stream context#2120
Draft PR (no merge!!!): check CI after remove line with canceling stream context#2120asmyasnikov wants to merge 1 commit into
Conversation
summaryInferred base version: v3.135.10 |
![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: ❌ 1 critical issue(s) found
Critical issues
- Critical | High: Removing
s.streamCancel()causes context and memory leak inCancelsGuard—internal/conn/grpc_client_stream.go:128
Other findings
- Minor | High:
streamCancelfield becomes dead code (assigned but never called) —internal/conn/grpc_client_stream.go:24
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.
| stack.FunctionID("github.com/ydb-platform/ydb-go-sdk/v3/internal/conn.(*grpcClientStream).finish"), err, | ||
| ) | ||
| s.streamCancel() | ||
| } |
There was a problem hiding this comment.
Severity: Critical
Confidence: High
Removing s.streamCancel() introduces a context and memory leak. streamCancel is the wrapped cancel function returned by CancelsGuard.WithCancel (conn.go:485). It does two things:
- Cancels the stream's derived context.
- Removes the cancel function entry from the
CancelsGuard.cancelsmap (seecancels_quard.go:27-32).
Without this call, each finished stream leaves a stale entry in childStreams.cancels. These entries accumulate over the connection's lifetime and are only cleaned up when the connection is closed (conn.go:548). For long-lived connections handling many streams, this is a memory leak proportional to the total number of streams ever opened.
Additionally, the stream's context remains uncanceled after the stream finishes, which can keep context-dependent resources (e.g., goroutines selecting on ctx.Done()) alive longer than intended.
Suggested fix: keep the s.streamCancel() call in finish().
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #2120 +/- ##
===========================================
- Coverage 75.16% 57.20% -17.97%
===========================================
Files 427 408 -19
Lines 36974 35101 -1873
===========================================
- Hits 27792 20078 -7714
- Misses 7993 13632 +5639
- Partials 1189 1391 +202
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:
|
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