Skip to content
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

[Python AIO] Return EOF from UnaryStreamCall.read() as documented #36660

Closed
wants to merge 2 commits into from

Conversation

XuanWang-Amos
Copy link
Contributor

@XuanWang-Amos XuanWang-Amos commented May 17, 2024

Fix: #36581

Based on our documentation, we should return grpc.aio.EOF to indicate the end of the stream:

async def read(self) -> Union[EOFType, ResponseType]:
"""Reads one message from the stream.
Read operations must be serialized when called from multiple
coroutines.
Note that the iterator and read/write APIs may not be mixed on
a single RPC.
Returns:
A response message, or an `grpc.aio.EOF` to indicate the end of the
stream.
"""

But if the call was intercepted, we're raising StopAsyncIteration, This Pr changes the return to match the documentation (Which is also the behavior for non-intercepted call).

@XuanWang-Amos XuanWang-Amos added the release notes: yes Indicates if PR needs to be in release notes label May 17, 2024
@XuanWang-Amos XuanWang-Amos marked this pull request as ready for review May 17, 2024 18:06
@gnossen gnossen changed the title [Python AIO] Change Intercepted UnaryStreamCall.read() return [Python AIO] Return EOF from UnaryStreamCall.read() as documented May 17, 2024
Copy link
Contributor

@gnossen gnossen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks for the fix

@@ -223,6 +223,50 @@ async def test_response_iterator_using_read(self):

await channel.close()

async def test_too_many_reads(self):
for interceptor_class in (
_UnaryStreamInterceptorEmpty,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the use of the subtest with the loop. It would be nice if we could also have a subtest that didn't include an interceptor at all. Running the exact same test against both an interceptor and a channel without an interceptor would help reinforce our principle that it should not be possible to tell whether a channel has an interceptor enabled or not externally.

I think you can change this by changing the iterable you're iterating over from Tuple[Callable[[], Interceptor]] to Tuple[List[Callable[[], Interceptor]]], and using [] to represent the variant with no interceptors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I'll add a subtest for channel without interceptor as well.

@gnossen
Copy link
Contributor

gnossen commented May 22, 2024

CC @sourabhsinghs for visibility

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug lang/Python release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

StopAsyncIteration exception when adding python aio.UnaryStreamClientInterceptor no-op interceptor
3 participants