-
Notifications
You must be signed in to change notification settings - Fork 263
postgres: use wait_for in disconnect #97
base: master
Are you sure you want to change the base?
Conversation
Note that there is also a timeout for |
This is a good call yup. |
This is recommended by asyncpg [1]. 1: https://github.com/MagicStack/asyncpg/blob/92c2d81256a1efd8cab12c0118d74ccd1c18131b/asyncpg/pool.py#L655-L656
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @blueyed I can take care of finishing this PR if you don't have time right now. WDYT?
@@ -66,7 +67,7 @@ async def connect(self) -> None: | |||
|
|||
async def disconnect(self) -> None: | |||
assert self._pool is not None, "DatabaseBackend is not running" | |||
await self._pool.close() | |||
await asyncio.wait_for(self._pool.close(), timeout=30) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @tomchristie wrote, it is a good idea to make the timeout configurable, otherwise, 30 seconds can be too much for everybody.
Sure, please go ahead. |
Any progress on this? |
This is recommended by asyncpg [1].
1: https://github.com/MagicStack/asyncpg/blob/92c2d81256a1efd8cab12c0118d74ccd1c18131b/asyncpg/pool.py#L655-L656