vmagamedov / grpclib

Pure-Python gRPC implementation for asyncio
http://grpclib.readthedocs.io
BSD 3-Clause "New" or "Revised" License
936 stars 92 forks source link

Is `graceful_exit` actually graceful? #148

Open jalaziz opened 2 years ago

jalaziz commented 2 years ago

graceful_exit ultimate calls server.close() which is defined as:

    def close(self) -> None:
        """Stops accepting new connections, cancels all currently running
        requests. Request handlers are able to handle `CancelledError` and
        exit properly.
        """
        if self._server is None:
            raise RuntimeError('Server is not started')
        self._server.close()
        for handler in self._handlers:
            handler.close()

This suggests graceful_exit is not as graceful as it could be because it ultimately cancels inflight requests. Wouldn't a more graceful approach be to simply call self._server.close() and then rely on server.wait_closed() to wait until all request handles end properly?

Or am I mistaken with the behavior of handler.close(). Digging into the code, it also seems that handler.wait_closed() only waits for handler completion when handler.close() was called, otherwise it returns immediately.

Assuming I'm interpreting the code correctly, is there any way to achieve a graceful exit that allows in flight requests to run to completion? I do realize this is a slippery slope as gRPC streaming can complicate the matter, but for that case, it would be great to give connections a fixed deadline to end on their own after which a forced cancellation is triggered.

I'd be happy to contribute a fix here, but I was hoping to get a better understanding of why the current implementation cancels in flight requests and anything else I'm missing.

vmagamedov commented 2 years ago

The main reason grpclib don't have a very graceful exit is because h2 library don't implement graceful connection close. Ideal solution would be to send GOAWAY frame from server to clients when we call server.close(). This should allow you to finish current requests without accepting new requests. But currently this renders HTTP/2 connection useless (closed), and your current requests can't finish correctly:

https://github.com/python-hyper/h2/blob/53feb0e0d8ddd28fa2dbd534d519a8eefd441f14/src/h2/connection.py#L207-L208

So we currently don't have abilities to inform client not to send new requests (over already established connections).

Currently to implement graceful exit you may need help from your infrastructure, you can have a HTTP/2-capable load balancer which manages incoming traffic for your gRPC servers. When you want to remove a server from serving requests you mark your server as "unhealthy" or "terminating" and load balancer stops sending new requests to your server. Then after some timeout you can stop your server.