_RequestContextManager being awaitable is dangerous

[RunDevelopment]

I've recently debugged a case where a ClientSession always timeouted even if the server being connected to was still alive. I found that the cause of this issue was improper usage of a _RequestContextManager. The offending code looked something like this:

# Edit: `read()` implicitely releases (=no leak), so I picked a different code example
# resp = await session.get("/path")
# return await resp.json()
resp = await session.get("/path")
resp.raise_for_status()

This code is simple to write, looks correct, works, and silently leaks resources. We use ruff and pyright in our code base and both tools were unable to find this mistake.

I think that the fact that await session.get("/path") works is an oversight in the API design of _RequestContextManager. This makes it too simple to leak resources without any tools being able to help.

Instead of _RequestContextManager itself being awaitable, I would suggest a property that gives access to the underlying coroutine (or a wrapper around it).

[Dreamsorcerer]

The implementation of that has always looked rather awkward to me, but the design is deliberate. While we generally recommend the context manager, there are many examples in the docs that do it this way. The connection should be released after a read(): https://docs.aiohttp.org/en/stable/client_reference.html#aiohttp.ClientResponse.read

So, what specifically is leaking?

[RunDevelopment]

The connection should be released after a read()

Ah, so that's the problem. There was one case where await session.get("/path") was used to check whether a service was still alive. No read() was performed, so that's most likely where the leak occurred.

The behavior of read() certainly makes await session.get("/path") less dangerous, but it's still error-prone. The resource safety of ClientResponse relies upon the object being used in certain ways (either in an async with or via release or read). This is not safe, because those ways are not enforced.

Example:

resp = await session.get("/path")
i_might_raise_an_error()
return await resp.json()

Any code in between a ClientResponse object being created and used in the outlined certain ways invalidates its resource safety.

[Dreamsorcerer]

I don't really disagree, but such a change would have a very large impact on existing users, so I don't think we are about to change it. If someone wants to update the docs to replace the current await occurrences with async with versions, I'd be happy with that though. Then maybe less users will copy it in future.

When the (unclosed) response is garbage collected, you should get a warning. So, at that point you should recognise there is a problem and investigate.