-
Notifications
You must be signed in to change notification settings - Fork 170
fix: receive eof while closing reads stream #1733
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
Summary of ChangesHello @Pulkit0110, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue in the asynchronous write stream where the stream might not correctly process all responses from the server during the closing sequence, particularly when the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request addresses an issue with handling the end-of-file (EOF) signal when closing an asynchronous write stream. The changes correctly manage scenarios where an intermediate response containing persisted_size is received before the final EOF. The accompanying tests effectively validate this new logic. My review includes a suggestion to refactor a part of the implementation for improved clarity and maintainability, which aligns with general best practices and does not conflict with any specific repository rules.
|
|
||
| if second_resp is not None: | ||
| _utils.update_write_handle_if_exists(self, second_resp) | ||
| _utils.update_write_handle_if_exists(self, second_resp) |
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.
why do we want to call _utils.update_write_handle_if_exists(self, second_resp) for second_resp. it should be EOF. We may add assert second_resp == grpc.aio.EOF
| @pytest.mark.asyncio | ||
| async def test_close_with_persisted_size_then_eof(self, mock_client): | ||
| """Test close when first recv has persisted_size, second is EOF.""" | ||
| import grpc |
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.
move this to top.
When
writer.close()is called without setting finalize_on_close flag, we need to get two responses:That's why added a check if the first response is not eof, then again receive the response from the stream.