Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ConnectionPool state after async cancellation #986

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

grynko
Copy link

@grynko grynko commented Dec 18, 2024

Summary

The PR fixes ConnectionPool state after exceptions/async cancellations.

Root cause analysis

ConnectionPool removes only connections which are:

  1. closed
  2. idle
  3. expired

At the same time, some implementations of AsyncConnectionInterface, e.g. AsyncHTTPConnection don't make actual connection on construction, but are left in semi-valid state with _connection = None. Thus, is_closed()/is_idle/has_expired() return False before actual connection is established or failed during the call to AsyncHTTPConnection.handle_async_request.

Let's take a look at ConnectionPool.handle_async_request method:

try:
    ...
        with self._optional_thread_lock:
            # [POINT 1] the `AsyncHTTPConnection` may be created to handle the request:
            closing = self._assign_requests_to_connections()
        await self._close_connections(closing)

        connection = await pool_request.wait_for_connection(timeout=timeout)

        try:
            # [POINT 2]  the `AsyncHTTPConnection` may create an actual connection:
            response = await connection.handle_async_request(
                pool_request.request
            )
        ...

Please, pay attention, that if the request task is being cancelled between [POINT 1] and [POINT 2], the Schrödinger connection (that is neither opened, nor closed) stays in the pool forever. In the long run (that depends on the load and pool max size), all new requests are refused by the pool, raising PoolTimeout for any request.

Please, see the test_connection_pool_cancellation_during_waiting_for_connection() for a reproducible example.

Solution

Introduce new method of AsyncConnectionInterface - is_connected(). It's not just not is_closed(), but returns False for such semi-constructed connections, allowing the ConnectionPool to collect them.

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.

@grynko grynko force-pushed the fix/connection_pool_after_async_cancellation branch from 49a5855 to c28f55c Compare December 18, 2024 16:43
@grynko grynko force-pushed the fix/connection_pool_after_async_cancellation branch from c28f55c to 6a92426 Compare December 18, 2024 16:47
@drozdvadym
Copy link

Thanks for working on this! We’ve encountered similar issues with connections getting stuck in the pool after async cancellations, so it’s great to see this being addressed. This fix will be helpful—I'm looking forward to it getting merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants