GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — postgres: use wait_for in disconnect by blueyed · Pull Request #97 · encode/databases

This repository was archived by the owner on Aug 19, 2025. It is now read-only.

Conversation

@blueyed

@blueyed blueyed commented May 7, 2019

Copy link
Copy Markdown
Contributor

This is recommended by asyncpg [1].

1: https://github.com/MagicStack/asyncpg/blob/92c2d81256a1efd8cab12c0118d74ccd1c18131b/asyncpg/pool.py#L655-L656

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@blueyed

blueyed commented May 7, 2019

Copy link
Copy Markdown
Contributor Author

Note that there is also a timeout for acquire, which gets used by release then also: https://github.com/MagicStack/asyncpg/blob/92c2d81256a1efd8cab12c0118d74ccd1c18131b/asyncpg/pool.py#L560-L564
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@blueyed
blueyed force-pushed the wait_for branch from b2ce151 to 8514a46 Compare May 25, 2019 01:34
@lovelydinosaur

lovelydinosaur commented Jun 20, 2019

Copy link
Copy Markdown
Member

This is a good call yup.
Presumably we also want to add the timeout to acquire right?
Any thoughts about adding timeout to the API someplace, and using that value?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@blueyed
postgres: use wait_for in disconnect
7685960 
This is recommended by asyncpg [1].

1: https://github.com/MagicStack/asyncpg/blob/92c2d81256a1efd8cab12c0118d74ccd1c18131b/asyncpg/pool.py#L655-L656
@blueyed
blueyed force-pushed the wait_for branch from 8514a46 to 7685960 Compare July 14, 2019 13:29
@lovelydinosaur
Merge branch 'master' into wait_for
406184e
vmarkovtsev
vmarkovtsev suggested changes Apr 29, 2020
View reviewed changes

@vmarkovtsev vmarkovtsev left a comment

Copy link
Copy Markdown
Contributor

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread databases/backends/postgres.py
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)

@vmarkovtsev vmarkovtsev Apr 29, 2020

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@blueyed

blueyed commented Jun 4, 2021

Copy link
Copy Markdown
Contributor Author

@vmarkovtsev

I can take care of finishing this PR if you don't have time right now. WDYT?

Sure, please go ahead.
(Sorry for the late reply, of course)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@blueyed
Merge branch 'master' into wait_for
8cb5c01
@taybin

taybin commented Sep 8, 2021

Copy link
Copy Markdown
Contributor

Any progress on this?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This branch has not been deployed

No deployments
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@vmarkovtsev vmarkovtsev vmarkovtsev requested changes
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@blueyed @lovelydinosaur @taybin @vmarkovtsev
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.