sqlx icon indicating copy to clipboard operation
sqlx copied to clipboard

WIP pool changes

Open abonander opened this issue 1 year ago • 5 comments

  • Use a separate waiting queue for new connections.
  • Pool inheritance (used for testing) only steals connect permits, not acquire permits.
  • Spawn connection attempts as their own task so they may complete even if the acquire() call is cancelled.
  • Race opening a new connection with acquiring one from the idle queue.
  • acquire() should now be completely cancel-safe.
  • Separate timeout for connecting.
  • New PoolConnector trait superceding both before_connect (requested but not yet implemented) and after_connect callbacks.
    • Implemented for closures returning Future, albeit with a 'static requirement for the returned Future (instead of BoxFuture).
    • May be updated to use async closures in a future release (hopefully backwards compatible but will require an MSRV bump): https://blog.rust-lang.org/inside-rust/2024/08/09/async-closures-call-for-testing.html
    • Can be used to support high availability, or implement custom backoff or connection throttling schemes (e.g. token bucket).
  • Use usize for all connection counts to get rid of weird inconsistencies.

Breaking Changes

  • Pool::set_connect_options() and get_connect_options() have been removed. Instead, implement the new PoolConnector trait (or use a closure) using something like Arc<RwLock<impl ConnectOptions>>.
  • PoolOptions::after_connect() has been removed. Instead, implement PoolConnector (or use a closure), open a connection and then apply any operations necessary.
  • PoolOptions::min_connections(), PoolOptions::max_connections() and Pool::size() now use usize instead of u32.

Fixes #3513 Fixes #3315 Fixes #3132 Fixes #3117 Fixes #2848

abonander avatar Oct 29 '24 22:10 abonander

Just tested this with #2805 and this PR seems to fix that issue 🎉

FSMaxB avatar Oct 29 '25 05:10 FSMaxB

@FSMaxB there aren't any changes in this PR that I would expect to fix #2805. Can you make sure it's not just a flaky test?

abonander avatar Oct 29 '25 09:10 abonander

Yup, pretty sure it's not flaky. If I test with 0.9.0-alpha.1, I repeatably get between 50 and 80 warning logs about transaction already in progress or there not being any transactions.

If I run it with the ab/pool-changes branch, I get none. You can test it yourself with the reproducer in that issue.

FSMaxB avatar Oct 29 '25 09:10 FSMaxB

The only reason I even checked is this item in your list above:

  • acquire() should now be completely cancel-safe.

FSMaxB avatar Oct 29 '25 09:10 FSMaxB

It's perfectly possible that this PR doesn't fix all races that can lead to the issue in #2805, but it seems to have at least fixed the race that the reproducer is triggering.

FSMaxB avatar Oct 29 '25 09:10 FSMaxB