"Server disconnected" on HTTP/2 connection pool with big keepalive_expiry. #2983

Activity

1. lovelydinosaur commented on Dec 6, 2023

   

   lovelydinosaur

   on Dec 6, 2023

   Member

   More actions

   Thanks @jonathanslenders.

   First places I'd start narrowing this down are...

   Different HTTP versions.

   • Does this reproduce with HTTP/1.1 at all? (From my understanding you've suggested not, but am clarifying.)

   Different clients.

   • Does this reproduce with httpx using a sync client?
   • Does this reproduce with httpx async running under trio?
   • Does this reproduce with other HTTP clients? (In particular requests, aiohttp, curl.)

   Different servers.

   • Does this reproduce with both hypercorn and uvicorn?
2. moshego189 commented on Dec 6, 2023

   

   moshego189

   on Dec 6, 2023

   More actions

   Hi @jonathanslenders
   it seems to me that the problem is hypercorn specific
   it just happened to me on a similar setup,
   i think hypercorn is closing the connection in some unexpected way when using http2
3. jonathanslenders commented on Dec 6, 2023

   

   jonathanslenders

   on Dec 6, 2023

   Author

   More actions

   Thank you @tomchristie for looking into it!

   I've been diving deeper. Maybe I have a fix (see below).

   I think the problem is due to the complexity of terminating TLS connections. It's hard to half-close a TLS connection, and by doing so, the underlying TCP stream is not necessarily closed, which makes it very hard for the other end to observe a half closed TLS connection.

   From httpx's point of view, when performing the second request, the HTTPConnectionState is IDLE, and _network_stream.get_extra_info('is_readable') is True. The TCP connection is still full open in both directions, and the data that is readable at this point could very well be TLS unwrap data.

   Our is_socket_readable check works fine for non-TLS connections, but for a TLS connection, we have to decrypt any pending traffic to see whether the TLS stream was not terminated. Basically, that means passing any buffered data through the ssl.MemoryBio and see whether an EOF/ssl.SSLZeroReturnError comes out at the other end, indicating that the other party unwrapped the connection.

   From Hypercorn's point of view, I can see that Hypercorn does call asyncio.StreamWriter.close() after the idle timeout. However, the transport is of type asyncio.sslproto._SSLProtocolTransport, which means there's no guarantee that the actual socket is closed. Looking at netstat, the TCP socket remains ESTABLISHED even after Hypercorn tries to close the writer. (@pgjones, you might want to look into this.)

   Diving into httpcore, and looking at httpcore/_async/http2, in handle_async_request, I don't see is_socket_readable being called at any point in time. Where do we check when the connection is still alive for http/2+TLS connections? I don't think we do that.

   The following httpcore patch does detect whether the connection is alive. I don't know which exception to raise, but this could be a starting point:

   diff --git a/httpcore/_async/http2.py b/httpcore/_async/http2.py
   index 8dc776f..e08e018 100644
   --- a/httpcore/_async/http2.py
   +++ b/httpcore/_async/http2.py
   @@ -136,6 +136,10 @@ class AsyncHTTP2Connection(AsyncConnectionInterface):
                self._request_count -= 1
                raise ConnectionNotAvailable()
   
   +        alive = await self._network_stream.is_alive()
   +        if not alive:
   +            raise Exception('Not alive!')
   +
            try:
                kwargs = {"request": request, "stream_id": stream_id}
                async with Trace("send_request_headers", logger, request, kwargs):
   diff --git a/httpcore/_backends/anyio.py b/httpcore/_backends/anyio.py
   index 1ed5228..d18217f 100644
   --- a/httpcore/_backends/anyio.py
   +++ b/httpcore/_backends/anyio.py
   @@ -1,5 +1,6 @@
    import ssl
    import typing
   +from collections import deque
   
    import anyio
   
   @@ -19,10 +20,13 @@ from .base import SOCKET_OPTION, AsyncNetworkBackend, AsyncNetworkStream
    class AnyIOStream(AsyncNetworkStream):
        def __init__(self, stream: anyio.abc.ByteStream) -> None:
            self._stream = stream
   +        self._buffer = deque()
   
        async def read(
            self, max_bytes: int, timeout: typing.Optional[float] = None
        ) -> bytes:
   +        if len(self._buffer) > 0:
   +            return self._buffer.popleft()
            exc_map = {
                TimeoutError: ReadTimeout,
                anyio.BrokenResourceError: ReadError,
   @@ -92,6 +96,25 @@ class AnyIOStream(AsyncNetworkStream):
                return is_socket_readable(sock)
            return None
   
   +    async def is_alive(self) -> bool:
   +        """
   +        Test whether the connection is still alive. If this is a TLS stream,
   +        then that is different from testing whether the socket is still alive.
   +        It's possible that the other end did a half-close of the TLS stream,
   +        but not a half-close of the underlying TCP stream.
   +
   +        We test whether it's alive by trying to read data. When we get b"", we
   +        know EOF was reached.
   +        """
   +        try:
   +            data = await self.read(max_bytes=1, timeout=0.01)
   +        except ReadTimeout:
   +            return True
   +        if data == b'':
   +            return False
   +        self._buffer.append(data)
   +        return True
   +
   
    class AnyIOBackend(AsyncNetworkBackend):
        async def connect_tcp(

   👍React with 👍1Reacted by 叶子
4. zanieb commented on Dec 6, 2023

   

   zanieb

   on Dec 6, 2023

   Contributor

   More actions

   Looks similar to the previously resolved encode/httpcore#679

   Thanks for all the detailed commentary!
5. lovelydinosaur

   added

   http/2Issues and PRs related to HTTP/2Issues and PRs related to HTTP/2

   on Sep 27, 2024
6. GreyElaina commented on Dec 4, 2024

   

   GreyElaina

   on Dec 4, 2024

   Last edited by GreyElaina

   Contributor

   More actions

       def is_closed(self) -> bool:
           return self._state == HTTPConnectionState.CLOSED or not self._network_stream.get_extra_info("is_readable")

   this patch just works (reproduce in macos), but i think network stream could have a is_readable property/method, which has been implemented in current all *stream expect MockStream. for MockStream, just simply leave return True.

   @tomchristie any interface suggest?

   ---

   UPDATE: a sample patch ready in https://github.com/GreyElaina/httpcore/tree/stream-is-readable-property