Connection pool optimization: move socket polling from expiry checks to connection usage #928

MarkusSintonen · 2024-06-13T18:13:48Z

Summary

(Split from original PR here #924 but this is more refined)

Connection pool implementation is heavily doing socket polling via AsyncConnectionInterface.has_expired(). This check in turn does AsyncNetworkStream.get_extra_info("is_readable") which finally does select.poll(). This check quickly starts to add up when doing many requests through httpcore with great concurrency. Many connection sockets in the pool are constantly polled over and over again when doing requests and also when request closes.

This PR instead moved the socket polling into AsyncHTTP11Connection.handle_async_request. Here the readable-IDLE connection raises ConnectionNotAvailable which makes the request to choose a next connection from the pool. This causes lot less socket polling as now its not done every single time for all connections when pool connections are assigned. Broken connections are still properly maintained as they are still removed from the pool.

This approach is very similar on how urllib3 is validating the health of the connection coming from the pool. (This check finally uses wait_for_read which uses similar socket polling as httpcore)

Adds some previously missing tests.

Benchmark shows how the heavy socket logic causes over 4.6x overhead.

In master with async requests:

PR with async:

With synchronous code the overhead is not as large. There the connection pools socket polling causes 1.6x overhead (maybe the overhead is lower due to sync concurrency using threads where the socket polling IO is not in GIL). Rest of overhead in sync is coming from the connection pools maintenance loops that is fixed here.

In master with sync code:

PR with sync:

Trio-based backend is also affected by this. There the overhead is at similar levels of about 5x as was seen here.

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 · 2024-06-15T17:44:09Z

@T-256 I fixed this PR to drop the interval based socket polling approach as you suggested here.

Instead this is now using a very similar approach as in urllib3 where the socket health is validated at the connection usage time when its taken from the pool. In oppose of polling sockets heavily for all the connections in the pool every single time requests are done and closed.

Even with the polling on usage, performance is much better than previous one.

T-256

Less overheads than interval mechanism :)
btw here are few review comments:

T-256 · 2024-06-15T21:01:41Z

httpcore/_exceptions.py

@@ -19,6 +19,10 @@ class ConnectionNotAvailable(Exception):
    pass


+class ServerDisconnectedError(Exception):


It could be subclass of ConnectionNotAvailable since there connection won't be available after server closed it. this would prevent from this change.

Also, we need to add it in docs.

Done, made it a subclass of the existing one. Should we have a subclass also for the previous state related error?

Also, we need to add it in docs.

I can not see existing one mentioned in the docs. Probably due to the exception being handled by the pool. I added docstrings instead to the classes about this.

Im now wondering do we even need the new ServerDisconnectedError. Because this is also internal to the pooling mechanism like the previous one. Im fine either way.

T-256 · 2024-06-15T21:08:29Z

httpcore/_async/http11.py

+            if self._state == HTTPConnectionState.SERVER_DISCONNECTED:
+                raise ServerDisconnectedError()
+
+            # If the HTTP connection is idle but the socket is readable, then the
+            # only valid state is that the socket is about to return b"", indicating
+            # a server-initiated disconnect.
+            server_disconnected = (
+                self._state == HTTPConnectionState.IDLE
+                and self._network_stream.get_extra_info("is_readable")
+            )
+            if server_disconnected:
+                self._state = HTTPConnectionState.SERVER_DISCONNECTED
+                raise ServerDisconnectedError()
+


most of users make subclass of handle_async_request to customize their Client instances. I think it could be better for us to keep it cleaner and more readable.
Could you export this change into priavte method _raise_for_state which have no returns and only raises if possible.

Done, moved it into _update_state function as we also need to update the state as part of raising. (Also for http2 to have it consistent)

T-256 · 2024-06-15T21:27:24Z

tests/_sync/test_http11.py

 import pytest

 import httpcore
+from httpcore._exceptions import ServerDisconnectedError


Suggested change

from httpcore._exceptions import ServerDisconnectedError

Don't import from private modules

T-256 · 2024-06-15T21:27:30Z

tests/_sync/test_http11.py

+        with pytest.raises(ServerDisconnectedError):
+            conn.request("GET", "https://example.com/")
+        assert conn.has_expired() and not conn.is_idle()
+
+        with pytest.raises(ServerDisconnectedError):
+            conn.request("GET", "https://example.com/")
+        assert conn.has_expired() and not conn.is_idle()


Suggested change

with pytest.raises(ServerDisconnectedError):

conn.request("GET", "https://example.com/")

assert conn.has_expired() and not conn.is_idle()

with pytest.raises(ServerDisconnectedError):

conn.request("GET", "https://example.com/")

assert conn.has_expired() and not conn.is_idle()

with pytest.raises(httpcore.ServerDisconnectedError):

conn.request("GET", "https://example.com/")

assert conn.has_expired() and not conn.is_idle()

Fixed (the latter one was actually for coverage but I changed to logic so that we dont need the extra assert step here for coverage)

T-256 · 2024-06-15T21:29:48Z

@MarkusSintonen Do you mind/working to send PR for re-adding asyncio backend and drop anyio backend? I'm also have interest on those works.

MarkusSintonen · 2024-06-16T06:13:54Z

@MarkusSintonen Do you mind/working to send PR for re-adding asyncio backend and drop anyio backend? I'm also have interest on those works.

Sure, I was wishing on getting the synchronization PR first approved/merged before opening it. But I can open it already.

MarkusSintonen · 2024-06-16T07:38:46Z

httpcore/_async/connection_pool.py

-                pool_request.assign_to_connection(connection)
+            else:
+                purged_connection = next(
+                    (c for c in self._connections if c.is_idle() or c.has_expired()),


This also considers expired connections here for removal. Because A) clock may have moved forward so that some are now expired here B) expired ones are the ones that maybe have been disconnected by the server (they are not idle)

MarkusSintonen · 2024-06-16T07:40:51Z

httpcore/_async/http11.py

+            server_disconnected = (
+                self._state == HTTPConnectionState.IDLE
+                and self._network_stream.get_extra_info("is_readable")
+            )


Should this check be also done in AsyncHTTP2Connection similarly as here?

I think yes, but cc @tomchristie who knows better about http2

MarkusSintonen · 2024-06-16T18:51:57Z

@MarkusSintonen Do you mind/working to send PR for re-adding asyncio backend and drop anyio backend? I'm also have interest on those works.

Its now here: #930

eth3lbert · 2024-07-26T22:46:05Z

httpcore/_async/connection_pool.py

@@ -267,15 +267,12 @@ def _assign_requests_to_connections(self) -> List[AsyncConnectionInterface]:
                for connection in self._connections
                if connection.can_handle_request(origin) and connection.is_available()
            ]


It seems like we could just find the first available one here?

available_connections = next( ( connection for connection in self._connections if connection.can_handle_request(origin) and connection.is_available() ), None, ) ... if available_connections: connection = available_connections ...

Oh, I just noticed that this has been split into separate PRs. 😅

eth3lbert · 2024-07-26T22:51:03Z

httpcore/_sync/connection_pool.py

@@ -267,15 +267,12 @@ def _assign_requests_to_connections(self) -> List[ConnectionInterface]:
                for connection in self._connections
                if connection.can_handle_request(origin) and connection.is_available()
            ]


MarkusSintonen changed the title ~~Connection pool optimization: reduce socket polling with expiry checks~~ Connection pool optimization: reduce socket polling in expiry checks Jun 13, 2024

MarkusSintonen mentioned this pull request Jun 13, 2024

Connection pool optimization: reduce connection maintenance loops complexity #929

Open

3 tasks

MarkusSintonen force-pushed the optimize-connection-pool-expiry branch from c4ab106 to 90bbfdc Compare June 15, 2024 16:48

MarkusSintonen changed the title ~~Connection pool optimization: reduce socket polling in expiry checks~~ Connection pool optimization: remove socket polling from expiry checks Jun 15, 2024

MarkusSintonen changed the title ~~Connection pool optimization: remove socket polling from expiry checks~~ Connection pool optimization: move socket polling from expiry checks to connection usage Jun 15, 2024

MarkusSintonen force-pushed the optimize-connection-pool-expiry branch 2 times, most recently from e3778a4 to 27e73bb Compare June 15, 2024 19:43

T-256 suggested changes Jun 15, 2024

View reviewed changes

MarkusSintonen commented Jun 16, 2024

View reviewed changes

T-256 approved these changes Jun 16, 2024

View reviewed changes

Move connection poll check away from pool expiry checks

6422799

MarkusSintonen force-pushed the optimize-connection-pool-expiry branch from b19e879 to 6422799 Compare June 16, 2024 18:39

MarkusSintonen mentioned this pull request Jul 10, 2024

Improve async performance. encode/httpx#3215

Open

eth3lbert reviewed Jul 26, 2024

View reviewed changes

Merge branch 'master' into optimize-connection-pool-expiry

96efe42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connection pool optimization: move socket polling from expiry checks to connection usage #928

Connection pool optimization: move socket polling from expiry checks to connection usage #928

MarkusSintonen commented Jun 13, 2024 •

edited

Loading

MarkusSintonen commented Jun 15, 2024 •

edited

Loading

T-256 left a comment

T-256 Jun 15, 2024

MarkusSintonen Jun 16, 2024

MarkusSintonen Jun 16, 2024 •

edited

Loading

T-256 Jun 15, 2024

MarkusSintonen Jun 16, 2024

T-256 Jun 15, 2024

MarkusSintonen Jun 16, 2024

T-256 Jun 15, 2024

MarkusSintonen Jun 16, 2024

T-256 commented Jun 15, 2024

MarkusSintonen commented Jun 16, 2024

MarkusSintonen Jun 16, 2024 •

edited

Loading

MarkusSintonen Jun 16, 2024

T-256 Jun 16, 2024

MarkusSintonen commented Jun 16, 2024

eth3lbert Jul 26, 2024

eth3lbert Jul 26, 2024

eth3lbert Jul 26, 2024

		@@ -19,6 +19,10 @@ class ConnectionNotAvailable(Exception):
		pass


		class ServerDisconnectedError(Exception):

Connection pool optimization: move socket polling from expiry checks to connection usage #928

Are you sure you want to change the base?

Connection pool optimization: move socket polling from expiry checks to connection usage #928

Conversation

MarkusSintonen commented Jun 13, 2024 • edited Loading

Summary

Checklist

MarkusSintonen commented Jun 15, 2024 • edited Loading

T-256 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkusSintonen Jun 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

T-256 commented Jun 15, 2024

MarkusSintonen commented Jun 16, 2024

MarkusSintonen Jun 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkusSintonen commented Jun 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkusSintonen commented Jun 13, 2024 •

edited

Loading

MarkusSintonen commented Jun 15, 2024 •

edited

Loading

MarkusSintonen Jun 16, 2024 •

edited

Loading

MarkusSintonen Jun 16, 2024 •

edited

Loading