Make Pool.close() wait until all checked out connections are released #299
Conversation
| def _abort(self): | ||
| # Put the connection into the aborted state. | ||
| self._aborted = True | ||
| self._protocol.abort() |
There was a problem hiding this comment.
I'd also add self._protocol = None to catch any use of connection after _abort().
asyncpg/connection.py
Outdated
| self._clean_tasks() | ||
| if self._proxy is not None: | ||
| # Connection is a member of a pool, but is getting | ||
| # aborted directly. Notify the pool about the fact. |
There was a problem hiding this comment.
"is getting aborted directly." -- outdated comment?
asyncpg/pool.py
Outdated
| @@ -163,31 +164,29 @@ def __init__(self, pool, *, connect_args, connect_kwargs, | |||
| # we close it. A new connection will be created | |||
| # when `acquire` is called again. | |||
| try: | |||
| proxy._detach() | |||
| # Use `close` to close the connection gracefully. | |||
| # An exception in `setup` isn't necessarily caused | |||
| # by an IO or a protocol error. | |||
| await self._con.close() | |||
There was a problem hiding this comment.
Add a comment that con.close() will cleanup its holder too.
asyncpg/pool.py
Outdated
| assert self._in_use | ||
| self._in_use = False | ||
| self._timeout = None | ||
| assert self._in_use is not None |
There was a problem hiding this comment.
Let's raise a proper exception here.
asyncpg/pool.py
Outdated
| self._timeout = None | ||
|
|
||
| if self._con._protocol.queries_count >= self._max_queries: | ||
| await self._con.close(timeout=timeout) |
There was a problem hiding this comment.
Should we add an early return here?
try:
await self._con.close(..)
finally:
self._release()
return
asyncpg/pool.py
Outdated
| self._con.terminate() | ||
| finally: | ||
| self._con = None | ||
|
|
||
| def _maybe_cancel_inactive_callback(self): | ||
| if self._inactive_callback is not None: | ||
| self._inactive_callback.cancel() | ||
| self._inactive_callback = None | ||
|
|
||
| def _deactivate_connection(self): |
There was a problem hiding this comment.
I'd rename this to _deactivate_inactive_connection()
asyncpg/pool.py
Outdated
| self._con.terminate() | ||
| assert self._in_use is None | ||
| if self._con is not None: | ||
| self._con.terminate() |
There was a problem hiding this comment.
Add a comment that we terminate the connection instead of just closing it because it's not in use.
asyncpg/pool.py
Outdated
| self._in_use = None | ||
|
|
||
| # Let go of the connection proxy. | ||
| if self._proxy is not None: |
There was a problem hiding this comment.
Let's make sure we set and unset self._proxy and self._con consistently.
There was a problem hiding this comment.
Connection and proxy lifetimes are not the same. A proxy only lives while the connection is acquired. I'll clarify with a comment.
asyncpg/pool.py
Outdated
| con = connection._detach() | ||
| self._check_init() | ||
|
|
||
| con = connection._con | ||
| con._on_release() |
There was a problem hiding this comment.
connection._con._on_release() to make it easier for the reader (o/w I need to track if con is used in the code below)
asyncpg/pool.py
Outdated
| con._on_release() | ||
| ch = connection._holder |
asyncpg/pool.py
Outdated
| # operations on it will fail. | ||
| if self._proxy is not None: | ||
| if self._proxy._con is not None: | ||
| self._proxy._detach() |
There was a problem hiding this comment.
Just make _proxy._detach() idempotent.
Currently, `pool.close()`, despite the "graceful" designation, closes all connections immediately regardless of whether they are acquired. With this change, pool will wait for connections to actually be released before closing. WARNING: This is a potentially incompatible behavior change, as sloppily written code which does not release acquired connections will now cause `pool.close()` to hang forever. Also, when `conn.close()` or `conn.terminate()` are called directly on an acquired connection, the associated pool item is released immediately. Closes: #290
Currently,
pool.close(), despite the "graceful" designation, closesall connections immediately regardless of whether they are acquired.
With this change, pool will wait for connections to actually be released
before closing.
WARNING: This is a potentially incompatible behavior change, as sloppily
written code which does not release acquired connections will now cause
pool.close()to hang forever.Also, when
conn.close()orconn.terminate()are called directlyon an acquired connection, the associated pool item is released
immediately.
Closes: #290