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

On the client side, handle a server shutdown. #25

Merged
merged 1 commit into from
May 16, 2018

Conversation

miracle2k
Copy link
Contributor

I have a client that is reading a stream from the server. When the server shuts down, the client hangs, basically because it is waiting here:

  File ".../grpclib/grpclib/protocol.py", line 93, in read
    await self._ready_event.wait()

Making sure that when we get connection_lost from the asyncio protocol that all the streams are closed fixes it for me (this will set the ready_event in the buffer).

In addition, I had to make sure that the async with api.Method.open() as stream: context manager doesn't hang in __aexit__ after that trying to read the trailing metadata.

What else is needed?

@vmagamedov
Copy link
Owner

There are two things client/server can wait: HEADERS and DATA. I think we should put some indicator into the grpclib.protocol.Stream.__headers__ queue in the grpclib.protocol.Stream.__ended__ method to wake up recv_headers coroutine. There is a corresponding issue - #8.

I think it is necessary to distinguish normal Stream.__ended__() and other causes (errors). So when connection is lost, we should raise exceptions in recv_headers and recv_data coroutines.

@vmagamedov
Copy link
Owner

I forgot another two things client/server can wait: WINDOW_UPDATED and H2Protocol.resume_writing. It would be too complex to fix all of these in one PR, so I will update #8 issue.

@vmagamedov vmagamedov merged commit c43f210 into vmagamedov:master May 16, 2018
@vmagamedov
Copy link
Owner

Thank you for your contribution. Going to complete this fix as soon as possible to make new release.

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.

2 participants