Is it a mistake to call pool.open multiple times ?

[sjiekak]

I was trying to do lazy pool initialization

async def connection() -> AsyncContextManager[psycopg.AsyncConnection]:
    connect_timeout_sec = 10

    await pool.open(wait=True, timeout=connect_timeout_sec)
    return pool.connection()

This is executed every time we need a connection, and it sometimes lead to

  File "/srv/testapp/web/save.py", line 61, in get
    async with await sqldatabase.connection() as conn:
  File "/srv/testapp/utils/sqldatabase.py", line 18, in connection
    await pool.open(wait=True, timeout=connect_timeout_sec)
  File "/usr/local/lib/python3.10/site-packages/psycopg_pool/pool_async.py", line 227, in open
    await self.wait(timeout=timeout)
  File "/usr/local/lib/python3.10/site-packages/psycopg_pool/pool_async.py", line 92, in wait
    assert not self._pool_full_event

[dvarrazzo]

Opening it the second time should be idempotent. But you are probably calling open a second time before the open at the first time has finished.

This is not supported. open can be called more than once, but not concurrently.

[sjiekak]

Thanks. Good to know.

[tino]

I ran into the same. I guess I remove the wait=True, as it is in the wait() where this assert is triggered. Or should I employ a lock around calls to .open(wait=True)?

However I'm a bit confused by why this would happen or by why the code is set up as it is: wait() employs the lock twice (from here):

    async def wait(self, timeout: float = 30.0) -> None:
        # """ ... """
        self._check_open_getconn()

        async with self._lock:                                   # <- here
            assert not self._pool_full_event
            if len(self._pool) >= self._min_size:
                return
            self._pool_full_event = AEvent()

        logger.info("waiting for pool %r initialization", self.name)
        if not await self._pool_full_event.wait_timeout(timeout):
            await self.close()  # stop all the tasks
            raise PoolTimeout(f"pool initialization incomplete after {timeout} sec")

        async with self._lock:                               # <- here
            assert self._pool_full_event
            self._pool_full_event = None

        logger.info("pool %r is ready to use", self.name)

Shouldn't the whole body be under a single lock to prevent this? (And why is there a lock for the second statement? It does a single variable assignment, isn't that already atomic?)

(I hope I don't sound condescending, this is pure from curiosity and wanting to understand it better!)

[dvarrazzo]

I think that we need to wait outside the lock here, because the lock is acquired by the tasks that maintain the pool, such as when adding a new connection.

[dvarrazzo]

By the way, I would be open to a MR to make opening concurrent-friendly. I don't have a lot of time to work on it, and at the moment is low priority because I don't really see an use case.

Why do you need to open the pool in a thread/task? Don't your have control on when the workers of your app get called?