Skip to content

[Aio] Call correctly the connect CB error when an error happens#21543

Merged
pfreixes merged 1 commit intogrpc:masterfrom
Skyscanner:allow_calling_none_existing_servers
Dec 24, 2019
Merged

[Aio] Call correctly the connect CB error when an error happens#21543
pfreixes merged 1 commit intogrpc:masterfrom
Skyscanner:allow_calling_none_existing_servers

Conversation

@pfreixes
Copy link
Collaborator

@pfreixes pfreixes commented Dec 21, 2019

Fixes a bug with the Aio socket that did not call correctly the
connect CB function when there was an issue connecting to a host

Issue related: #20818

Also fixes #20869 by removing the usage of timeout by calling invalid hosts.

@pfreixes pfreixes added lang/Python release notes: yes Indicates if PR needs to be in release notes kokoro:run labels Dec 21, 2019
@pfreixes pfreixes requested a review from lidizheng December 21, 2019 14:59
@pfreixes pfreixes requested a review from gnossen December 21, 2019 14:59
Copy link
Contributor

@lidizheng lidizheng left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

'/grpc.testing.TestService/UnaryCall',
request_serializer=messages_pb2.SimpleRequest.SerializeToString,
response_deserializer=messages_pb2.SimpleResponse.FromString,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

optional: switch to using our generated Stub.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We haven't been using any stub in any of the tests of the test suite, should we start using them?

Or let's use them only once we start giving support/coverage for using the stubs through the Aio module?

@@ -62,21 +61,15 @@ async def test_unary_unary(self):
self.assertIsInstance(response, messages_pb2.SimpleResponse)

async def test_unary_call_times_out(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: We could have a time out test case for succeed RPC just like before. You could refer to server_test.py. Not a blocker for this PR.

Copy link
Collaborator Author

@pfreixes pfreixes Dec 23, 2019

Choose a reason for hiding this comment

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

The only "problem" is that our test suite for the client is quite coupled to the basic proto stub test_pb2_grpc.py, so any change in functionality for any method could change the desired semantic, the same that happened with the EmptyCall method.

This is a recurrent question, should we decouple the test suite from that file, so no longer using it, and use our own one? or even better, use our one server fixture based on a programmatic RPC handlers builder, the same that is being used for testing the current Aio server.

Fixes a bug with the Aio socket that did not call correctly the
connect CB function when there was an issue connecting to a host
@pfreixes pfreixes merged commit a85d001 into grpc:master Dec 24, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

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.

[AIO] Avoid the usage of sleep when timeouts are being tested

4 participants