-
Notifications
You must be signed in to change notification settings - Fork 68
fix: handle asyncio.CancelledError as retriable in topic writer and reader #761
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Fixes reconnection behavior for TopicWriter/TopicReader when gRPC surfaces connection problems as asyncio.CancelledError, ensuring unintended cancellations trigger retry/backoff rather than permanently stopping the session.
Changes:
- Treat
asyncio.CancelledErroras retriable inWriterAsyncIOReconnector._connection_loopwhen not intentionally closing. - Treat
asyncio.CancelledErroras retriable inReaderReconnector._connection_loopand add_closedstate for clean shutdown behavior. - Add/extend asyncio tests to cover reconnection on
CancelledErrorfor both writer and reader.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| ydb/_topic_writer/topic_writer_asyncio.py | Updates writer reconnector loop to distinguish intentional vs. gRPC-originated cancellation and retry on the latter. |
| ydb/_topic_writer/topic_writer_asyncio_test.py | Adds test coverage ensuring writer reconnects and resends unacked messages after CancelledError. |
| ydb/_topic_reader/topic_reader_asyncio.py | Updates reader reconnector loop to retry on CancelledError and adds _closed to support clean exit. |
| ydb/_topic_reader/topic_reader_asyncio_test.py | Adds test coverage ensuring reader reconnects after CancelledError from stream error handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
7539c18 to
a2b9d5b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
a2b9d5b to
7d2f9ba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
7d2f9ba to
839bb35
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
asyncio.CancelledError from gRPC connection issues (e.g., YDB host downtime) was incorrectly treated as a fatal error, causing TopicWriter/TopicReader to stop instead of reconnecting.
The fix distinguishes between:
Fixes #735
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