Fix 2 bugs and improve performance from O(N²+NxM) to O(N+M) in _assign_requests_to_connections

[VictorPrins]

Summary

Fixes 2 logic bugs in _assign_requests_to_connections and improves complexity from O(N²+NxM) to O(N+M) where N=connections, M=requests.

Although I attempted a smaller PR with an atomic change, I ultimately decided for a redesign of the method because the algorithm design of the method was fundamentally flawed.

Motivation

_assign_requests_to_connections is in the hot path of this library and the bugs and complexity issue were severly slowing down my application.

Bug 1: max_keepalive_connections is not respected

This causes idle connections to be evicted unnecessarily. This bug is also addressed in this PR (currently approved but not merged).

The bug is on lines 294-295. The expression on line 294 simply returns len(self._connections), ignoring the number of idle connections, and compares this number to self._max_keepalive_connections.

httpcore/httpcore/_async/connection_pool.py

Lines 294 to 295 in 5974b03

	and len([connection.is_idle() for connection in self._connections])
	> self._max_keepalive_connections

This test demonstrates the bug. This test fails with the current code on master, but passes with the new implementation of this PR.

Bug 2: multiple requests are assigned to the same HTTP/1.1 connection, leading to ConnectionNotAvailable exceptions

The bug is on lines 322-323. An available connection is assigned to a request, but the status of that connection is not actually changed, resulting in that connection also being assigned to the next request in the queue.

httpcore/httpcore/_async/connection_pool.py

Lines 322 to 323 in 5974b03

	connection = available_connections[0]
	pool_request.assign_to_connection(connection)

This test demonstrates the bug. This test fails with the current code on master, but passes with the new implementation of this PR.

Perf: Reduce complexity from quadratic to linear

This PR reduces time complexity from O(N²+NxM) to O(N+M) where N=connections, M=requests.
In a typical use case, with for instance 100 connections in the pool and 500 requests in the queue this reduces complexity dramatically, especially since _assign_requests_to_connections is in the hot path of this library because it will be called twice for every request (on start and on finish).

In my use case, this PR reduces the CPU utilization of my application by half. Profiling shows that _assign_requests_to_connections previously accounted for 59% of all samples; after applying this PR, its share dropped to 4%.

The problem: unnecesary nested loops

This snippet contains a nested loop over all connections (O(N²)). This is unnecessary and can be done in a single pass (O(N)).

This snippet contains a nested loops over all requests and connections (O(NxM)). This is unnecessary and can be done in a single pass (O(M)) with early exit if all connections are occupied.

Checklist

• I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
• I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
• I've updated the documentation accordingly.

[MarkusSintonen]

Already tried here the same but httpcore seems to be abandoned #929

[VictorPrins]

@MarkusSintonen Thanks for replying and for linking that PR. httpcore gets almost 10M downloads a day through the httpx dependency, and usage is still increasing over time. So I hope the project isn't being abandoned. Hopefully we can draw some attention to this.

[VictorPrins]

Citing some issues related to this, including in downstream libraries relying on httpcore:

• Improve async performance. httpx#3215
• [Bug]: Cannot get past 50 RPS BerriAI/litellm#6592
• httpx client has very poor performance for concurrent requests compared to aiohttp openai/openai-python#1596
• Httpx not production ready under load httpx#3100

[florisheijmans]

+1

[svilupp]

+1

[blazewicz]

Good luck, #1000 has been hanging for 6 months, despite being so tiny. The review you saw is not binding, I can't merge it until some maintainer approves it. This project does seem abandoned, I'm afraid.

[lovelydinosaur]

@blazewicz Nope, just the attention isn't on this PR for a reason... v1 comprehensively addresses this issue and is being quietly worked on. It's worth taking a look at the relative complexity of the connection pool handling there vs. the connection pool handling in the the existing httpcore.