Skip to content
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

Acquisition timeout #19

Merged
merged 6 commits into from
Aug 29, 2022
Merged

Conversation

robx
Copy link
Contributor

@robx robx commented Aug 29, 2022

This introduces an acquisition timeout, making it possible to bail out of waiting for a connection cleanly. (Includes the changes from #16 to avoid dealing with conflicts, happy to rebase with only this change though if preferred.)

We want something like this for PostgREST (PostgREST/postgrest#2348) to be able to actively time out queries that are blocked waiting for an overloaded connection pool.

  • This times out only the "blocking for a slot" part of connection acquisition, not establishing the connection. (I think that's probably the cleaner approch, but it's a point to consider.)
  • Some alternative ideas of approaching this that wouldn't need changes to hasql-pool:
    • Just put a timeout on the whole action (i.e. timeout delay $ Pool.use $ query). Undesirable because it might mean interrupting while the query is running database server side, running into problems like returning a timeout error while postgres might still successfully apply the request.
    • A disarmable timeout (i.e. disarmableTimeout delay $ \disarm -> Pool.use $ disarm >> query). That should work, it's just tricky to get right and pretty complicated and I'd rather avoid going there. 😬

@robx robx force-pushed the acq-timeout-upstream branch from 2c24163 to 946358e Compare August 29, 2022 11:52
@robx
Copy link
Contributor Author

robx commented Aug 29, 2022

The actual change is the last commit only: 946358e

@nikita-volkov nikita-volkov merged commit 946358e into nikita-volkov:master Aug 29, 2022
@robx robx deleted the acq-timeout-upstream branch August 29, 2022 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants