Skip to content

Fixing race condition between start() and close() in TopicRetryableStream - #707

Open
Myllyenko wants to merge 1 commit into
ydb-platform:masterfrom
Myllyenko:fix/topic-stream-start-close-race
Open

Fixing race condition between start() and close() in TopicRetryableStream#707
Myllyenko wants to merge 1 commit into
ydb-platform:masterfrom
Myllyenko:fix/topic-stream-start-close-race

Conversation

@Myllyenko

Copy link
Copy Markdown

start() checked isClosed before creating the stream and only published it afterwards. When close() ran in between it found an empty realStream slot, returned false - which WriterImpl reads as "the stream will never call onClose", so it finishes the shutdown - and start() then went on to open a stream that nothing would ever close. It stayed connected until the server gave up on it.

Re-check isClosed once the stream is published and started, and close it if the slot is still ours. The check has to come after start() because closing a stream that was never started would half-close a gRPC call that has not begun.

start() checked isClosed before creating the stream and only published it
afterwards. When close() ran in between it found an empty realStream slot,
returned false - which WriterImpl reads as "the stream will never call
onClose", so it finishes the shutdown - and start() then went on to open a
stream that nothing would ever close. It stayed connected until the server
gave up on it.

Re-check isClosed once the stream is published and started, and close it
if the slot is still ours. The check has to come after start() because
closing a stream that was never started would half-close a gRPC call that
has not begun.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@codecov

codecov Bot commented Aug 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 72.43%. Comparing base (d876581) to head (5add468).
⚠️ Report is 44 commits behind head on master.

Files with missing lines Patch % Lines
...java/tech/ydb/topic/impl/TopicRetryableStream.java 66.66% 0 Missing and 1 partial ⚠️

❌ Your patch check has failed because the patch coverage (66.66%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #707      +/-   ##
============================================
+ Coverage     71.91%   72.43%   +0.52%     
- Complexity     3474     3524      +50     
============================================
  Files           390      391       +1     
  Lines         16220    16344     +124     
  Branches       1698     1703       +5     
============================================
+ Hits          11664    11839     +175     
+ Misses         3904     3866      -38     
+ Partials        652      639      -13     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant