Adding max active connection limit for http2 connections

[alohamora]

Hey guys,
First of all, would like to thank you for the amazing work you guys have done here.

I was experimenting with async http2 connection pool for burst request handling, and I noticed that httpcore does not initialize more than 1 http2 connection in the pool ever as the is_available check for an http2 connection shall always return True until all the stream_ids (2^31 - 1) have been used.
This behaviour prevents us from using the full capabilities of the client as a single http2 connection has its own set of bottlenecks due to network bandwidth and we are not able to utilize multiple connections in the pool.

So I propose the following solution to maintain a state variable in http2 connection class called active_connections which will store the count of the current in-flight active requests, and check the current active connections in is_available function

This change has helped me improve the performance of my client by 2x and now we are able to support sending a bulk load of 5000 requests in 7 seconds as compared to 15 seconds using the older implementation by utilizing 10 connections instead of 1.

So I would request you to consider this solution and I would also like to work on the PR for the same.
Also, would love to discuss any other possible solutions for this problem.

Thank you

class AsyncHTTP2Connection(AsyncConnectionInterface):
    def __init__():
        self._active_connections = 0
        self.max_active_connections = 500

    async def handle_async_request(self, request: Request) -> Response:
        if not self.can_handle_request(request.url.origin):

        async with self._state_lock:
            if self._state in (HTTPConnectionState.ACTIVE, HTTPConnectionState.IDLE):
                self._request_count += 1
                self._expire_at = None
                self._state = HTTPConnectionState.ACTIVE
                self._active_connections += 1

            else:
                raise ConnectionNotAvailable()

    def is_available(self) -> bool:
        return (
            self._state != HTTPConnectionState.CLOSED
            and not self._connection_error
            and not self._used_all_stream_ids
            and (self._active_connections <= self.max_active_connections)

        )

    async def _receive_response_body(
        self, request: Request, stream_id: int
    ) -> typing.AsyncIterator[bytes]:
        while True:
            event = await self._receive_stream_event(request, stream_id)
            if isinstance(event, h2.events.DataReceived):
                amount = event.flow_controlled_length
                self._h2_state.acknowledge_received_data(amount, stream_id)
                await self._write_outgoing_data(request)
                yield event.data
            elif isinstance(event, (h2.events.StreamEnded, h2.events.StreamReset)):
                async with self._state_lock:            
                    self._active_connections -= 1

                break

    async def _response_closed(self, stream_id: int) -> None:
        await self._max_streams_semaphore.release()
        del self._events[stream_id]
        async with self._state_lock:
            if self._state == HTTPConnectionState.ACTIVE and not self._events:
                self._state = HTTPConnectionState.IDLE
                self._active_connections -= 1

[jhominal]

Thank you for opening this discussion.

Looking at your code sample, it seems to me that a new TCP connection is opened for each request, up to max_active_connections. Is that correct?

I have the feeling that it should probably not be the default behavior of a http/2 client to open up to 500 connections to a given host, so I think that there should be a way of tuning that behavior when initializing the client (and I would be in favor of keeping the default behavior as it is now).

[alohamora]

Sorry, I think the variable naming has led to this confusion.

Actually, I am not opening multiple TCP connections for each request, I am just limiting the no of active requests that are handled by a single http2 connection.
The current behaviour of a http2 connection is that it will always accept incoming requests until all the stream_ids are used i.e (2^31 - 1). - ref

I am suggesting that in addition to the above condition, we should add a check for active in-flight requests that are being handled by the connection, this in turn allows us to use multiple connections in a connection pool as now a single connection won't accept all the requests.
have changed the variable naming in code above to clarify this confusion.

[tomchristie]

The HTTP/2 spec mandates opening a single connection to an origin, and using the stream multiplexing for multiple concurrent requests, rather than using multiple connections. We're careful to follow this in httpx.

Have you taken a look at how the performance differs between HTTP/1.1 and HTTP/2 for your example use case?

[alohamora]

Yes, I agree that http2 mandates opening a single connection to an origin, but the maximum concurrent requests that can be served on a single connection can be limited at client/server level using the SETTINGS_MAX_CONCURRENT_STREAMS setting, which is documented here
So, even though the client is sending a lot of requests over a single connection, they would not be processed concurrently on the server if this setting is configured to a low value.
In addition to this, there are also network bottlenecks while serving a lot of data on a single connection, which shall hamper the performance of sending large requests on a single connection.

Coming to my usecase, I am running a http client which needs to make bulk requests to a server which has an average response time of 300ms.
I don't have control over the server, so cannot decode the SETTINGS_MAX_CONCURRENT_STREAMS setting for the same

I need to achieve an RPS of around 600 and for that I tried 3 combinations.

1. plain http2 connection pool - couldn't achieve the target RPS at all, as only single connection is being used.
2. http2 connection pool with the above modifications - achieved target RPS with 10 connections in the pool and each processing a maximum of 500 concurrent requests.
3. http1 connection pool - achieved target RPS with 170 connections in the connection pool.

So I believe that the second approach provides the option to extract better performance out of the server by allowing the client to use multiple connections from the pool and we are not limited by the server's ability to handle concurrent requests on a single http2 connection.

[tomchristie]

I was thinking that we could drive this feature using the httpx.Limits with the keyword argument, http2_max_concurrent_streams and keep the default value as None, which shall disable this limit and keep the behaviour as it is right now.

This is confusing.

The opening discussion is framed as "configurable connection limit for http2 connections", but your comment here is requesting "configurable stream limit for http2 connections".

any suggestions on how to take this discussion forward?

Yep. Pick one of these two, and then let's discuss further...

[alohamora]

Sure, sorry for the confusion due to the incorrect framing of my question. I got confused between these two terms, and it's my first time engaging with an open source community, so please let me know how can I drive these conversations in a more better way 😄

My final proposal is to introduce a "configurable active streams limit for a single http2 connection" so that we can use multiple http2 connections from the connection pool and the performance of the client is not limited by the server's SETTINGS_MAX_CONCURRENT_STREAMS setting.

[alohamora]

Hey @tomchristie, any updates on this discussion?

[alohamora]

Hey guys, any plans to take this forward? would be great if we could incorporate this

[tomchristie]

So... I didn't reply earlier on this discussion because the proposal still wasn't clear to me, and I wasn't sure how to communicate that.

I think my overall assessment would be that we probably should continue to respect the "one connection per HTTP/2 origin" and the "observe the server's max stream setting". Or at least that we probably should do so for the current moment.

A minimum requirement that we'd need to start wit if we were to consider a change here would be a fully reproducible example, so that we're able to observe the same behaviour you're seeing.