Skip to content

Pool never closes if acquire with timeout is cancelled #547

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

Closed
aaliddell opened this issue Mar 27, 2020 · 0 comments · Fixed by #608
Closed

Pool never closes if acquire with timeout is cancelled #547

aaliddell opened this issue Mar 27, 2020 · 0 comments · Fixed by #608

Comments

@aaliddell
Copy link
Contributor

  • asyncpg version: 0.20.1
  • PostgreSQL version: 11
  • **Do you use a PostgreSQL SaaS? No
  • Python version: 3.6 and 3.7
  • Platform: Linux
  • Do you use pgbouncer?: No
  • Did you install asyncpg with pip?: Yes
  • If you built asyncpg locally, which version of Cython did you use?: N/A
  • Can the issue be reproduced under both asyncio and
    uvloop?
    : Yes

Here's a demo:

import asyncio
import asyncpg

async def main():
    # Create a pool
    pool = await asyncpg.create_pool(...)

    # An example function using the pool
    async def use_pool(pool):
        async with pool.acquire(timeout=200):
            pass

    # Schedule the task and cancel
    task = asyncio.ensure_future(use_pool(pool))
    await asyncio.sleep(0.0000000001)  # Yield to let task start
    task.cancel()

    print('Closing pool')
    await pool.close()
    print('Closed')

loop = asyncio.get_event_loop()
loop.run_until_complete(main())
loop.close()

This code creates a pool, then starts a task that tries to acquire a connection with a timeout. This task is allowed to start, but is then cancelled. Following this, the pool is closed. The expected behaviour is that the pool should close cleanly, as no connections are in use.

When running this code, I see 'Closing pool' printed, then the script hangs indefinitely. It appears the pool is waiting for the release of the connection, which has not correctly handled the cancellation. When observing this in actual code, printing the pending tasks shows one stuck at PoolConnectionHolder.wait_until_released() (line 229). This suggests that the _in_use future is not being resolved correctly during cancellation.

However, if I remove the timeout=200 on the acquire, the bug goes away. Additionally, the 0.0000000001 second sleep is somewhat important:

  • Setting this to a higher value eventually leads to the code working, as the task will complete before being cancelled
  • Removing this sleep also makes the code 'work', as the task is never scheduled.

Effectively, there is a very short 'critical' window (~10us seconds on this machine) during which a cancellation arriving will lead to the pool being unclosable. Hence this bug has been a total pain to hunt for.

@aaliddell aaliddell changed the title Pool never closes if aquire with timeout is cancelled Pool never closes if acquire with timeout is cancelled Mar 27, 2020
aaliddell added a commit to aaliddell/asyncpg that referenced this issue Mar 28, 2020
Closes MagicStack#547

When wait_for is cancelled, there is a chance that the waited
task has already been completed, leaving the connection
looking like it is in use. This fix ensures that the connection
is returned to the pool in this situation.

For context, see:
https://bugs.python.org/issue37658
MagicStack#467
elprans added a commit that referenced this issue Aug 16, 2020
`asyncio.wait_for()` currently has a bug where it raises a
`CancelledError` even when the wrapped awaitable has completed.
The upstream fix is in python/cpython#37658.  This adds a workaround
until the aforementioned PR is merged, backported and released.

Fixes: #467
Fixes: #547
Related: #468
Supersedes: #548
elprans added a commit that referenced this issue Aug 16, 2020
`asyncio.wait_for()` currently has a bug where it raises a
`CancelledError` even when the wrapped awaitable has completed.
The upstream fix is in python/cpython#21894.  This adds a workaround
until the aforementioned PR is merged, backported and released.

Fixes: #467
Fixes: #547
Related: #468
Supersedes: #548
elprans added a commit that referenced this issue Aug 16, 2020
`asyncio.wait_for()` currently has a bug where it raises a
`CancelledError` even when the wrapped awaitable has completed.
The upstream fix is in python/cpython#21894.  This adds a workaround
until the aforementioned PR is merged, backported and released.

Co-authored-by: Adam Liddell <[email protected]>
Fixes: #467
Fixes: #547
Related: #468
Supersedes: #548
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant