Removing AsyncConnectionPool._pool_lock

[Tinche]

Hello,

first time contributor here. I was going to submit a PR but noticed in the repo guidelines you folks would prefer a discussion first.

A little context first. I'm in charge of a mid-size asyncio codebase, serving mostly mobile clients but also web, around 10 services, each between 15k and 30k lines of code, processing several thousand requests per second in total. I'm also in charge of operating said system, and making sure it's running as smoothly as possible.

We've been big users of aiohttp, both on the server and client sides, for a long time. For our DynamoDB services, recently we've migrated off of aiobotocore (based on aiohttp) to aiodynamo (based on httpx, but also supports aiohttp). One of the reasons for the migration was that aiohttp sometimes breaks down under load, and I was hoping httpx would be doing better. Also aiohttp seems to be somewhat undermaintained at the moment, and httpx seems very active.

Unfortunately we've had aiodynamo+httpx break down under load for us as well today, but this discussion isn't about that. I see httpcore has recently received a rework of the AsyncConnectionPool class, and until we've tried that out in production I'm not going to spend too much time debugging the existing issue or opening bugs here.

What I did think I'd do is take a look at the new code and see if I can spot any possible areas of improvement. I've noticed the AsyncConnectionPool uses an async lock to keep some internal state consistent. Now I don't have any experience with anyio, but when using asyncio, locks like these won't necessarily keep your state safe. Even while holding the lock, if the block of code contains any suspension points (i.e. you await), the state may be left inconsistent if the task running it is cancelled. (The aiohttp server will cancel handler tasks when the peer disconnects, which I think is ultimately the correct approach.)

So I put together a pull request to convert _close_expired_connections and _attempt_to_acquire_connection to ordinary functions. The only async thing these methods to is closing connections; which is a waste to wait for anyway. So I close them async using an anyio task group that's scoped to the lifetime of the pool itself. With these changes the lock is unnecessary. Less complexity, less time spent locking/unlocking, less suspension points to mess up the state.

Unfortunately I have a couple of tests failing that I don't really understand, and will need some help there.

Let me know if you feel this is worth pursuing.

[Tinche]

I have also noticed the connection pool doesn't handle cancellations very well, so I've improved that too. I've also implemented some more tests. My work is available here: https://github.com/Tinche/httpcore/tree/tin/remove-async-lock

Tangentially related, I think our production use has uncovered an issue with anyio locks: agronholm/anyio#387. It's being worked on by upstream.

[tomchristie]

Needs some consideration. It's entirely possible that we want the lock portions to just be shielded from cancellation, for the portion of time during which they're running. They shouldn't be unbounded.

(Also worth noting that you shouldn't really need cancellation directly, since we've got sensible timeout controls throughout anyways, but I guess we can't necessarily pre-judge what kind of context or expectations httpx will be running in, so maybe?)

[Tinche]

My implementation removes the lock portions completely, updating the state happens between suspension points which makes it atomic. There's no shielding needed.

The pool needs to keep its internal state consistent in the face of cancellation on any suspension point, as a matter of principle. The cancellation can come from many other sources rather than timeouts.

[Tinche]

After a short discussion on Gitter looks like this wouldn't be interesting to contribute, so we can close this.