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

Does detection of lost connections implicitly depend on asyncio? #346

Closed
florimondmanca opened this issue Sep 15, 2019 · 3 comments
Closed

Comments

@florimondmanca
Copy link
Member

florimondmanca commented Sep 15, 2019

I'm mostly opening this to track the issue — I'll try to tackle this based on the trio backend in #276, and see what could work, e.g. setting up a background task to check for socket readability.

Worth mentioning though that anyio already seems to handle this quite well.


The Stream interface exposes a is_connection_dropped() method which the ConnectionPool uses to know whether it can reuse a connection when acquiring a new one.

async def acquire_connection(self, origin: Origin) -> HTTPConnection:
logger.debug(f"acquire_connection origin={origin!r}")
connection = self.active_connections.pop_by_origin(origin, http2_only=True)
if connection is None:
connection = self.keepalive_connections.pop_by_origin(origin)
if connection is not None and connection.is_connection_dropped():
self.max_connections.release()
connection = None

Also noted by @njsmith in #276 (comment):

asyncio always keeps reading from the socket in the background, whether you call read or not, as long as the loop is running. Trio and sync code don't. Unfortunately, httpx currently relies on the asyncio behavior to detect when an idle connection has gone stale. IMO this is a bug in httpx's idle connection handling, but that's the current state [...].

Also in #276 (comment):

[A]rguably it's a bug that when we want to detect if a connection is reusable, then the asyncio backend checks for EOF, when ideally it should be checking for readability.

Indeed, that's what we do:

def is_connection_dropped(self) -> bool:
return self.stream_reader.at_eof()

There was an entire discussion about readable vs closed before, see for example #143 (comment). Counter-intuitively, it seems that when the other end has closed the conection then a socket is said to be readable (.recv() would return immediately with b"").

I guess the tl;dr here is that the way we implement .is_connection_closed() for asyncio is good enough (if the socket was closed, asyncio's background socket checker will mark at as having reached EOF), but we might need to change the way we check for dropped connections.

@florimondmanca florimondmanca changed the title Detection of lost connections implicitly depends on asyncio Does detection of lost connections implicitly depend on asyncio? Sep 15, 2019
@florimondmanca
Copy link
Member Author

It seems I tangled myself up on the previous discussions and the counter-intuitive notion of readability. As I said, in the asyncio case checking for EOF is good enough, because a socket that reached EOF in the sense of asyncio is also readable (right?). So the code might just need some comments, which I added in c542735.

Provided my understanding of these subtle notions is correct (i.e. if a socket is readable then the connection was dropped), the implementation in #276 should be correct, so I'll close this since there doesn't turn out to be any immediate issue with our asyncio implementation.

@njsmith
Copy link

njsmith commented Sep 15, 2019

Yeah, the current way of detecting stale connections on asyncio isn't optimal, in the sense that it will occasionally think a connection is ok when it's actually been closed by the server. But it's just a bug in that backend, that doesn't affect other backends, and the impact is just that this heuristic is a bit less reliable than it could be.

In particular, this thing that I said and you quoted is wrong, and I retracted it later in the original thread:

Unfortunately, httpx currently relies on the asyncio behavior

@florimondmanca
Copy link
Member Author

Yes, I think I understand that there’s actually not really a dependency there. Thanks for clarifying!

rowillia pushed a commit to rowillia/httpx that referenced this issue Sep 18, 2019
The current implementation doesn't require blocking, but given
encode#346 discussed some possible future improvements,
[trio.hazmat.wait_readable](https://trio.readthedocs.io/en/stable/reference-hazmat.html#trio.hazmat.wait_readable) is async,
and [urllib3 relies on blocking APIs to detect dropped connections](https://github.com/urllib3/urllib3/blob/f7a4bed04085975918b3469bf94e195df9faf29a/src/urllib3/util/wait.py#L105) it is worth marking this API as async pre-1.0 to avoid future breaking changes.
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

No branches or pull requests

2 participants