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

WIP pool changes #3582

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

WIP pool changes #3582

wants to merge 2 commits into from

Conversation

abonander
Copy link
Collaborator

@abonander abonander commented Oct 29, 2024

  • 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

/// `set_connect_options` and `get_connect_options` were removed in 0.9.0 because they complicated
/// the pool internals. They can be reimplemented by capturing a mutex, or similar, in the callback.
///
/// This example uses Postgres and [`tokio::sync::Mutex`] but may be adapted to any driver
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed I actually use RwLock in the example.

}

#[cfg(not(feature = "_rt-async-std"))]
missing_rt((duration, f))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When playing around with this PR locally (to see if it fixes an acquire timeout issue, which it unfortunately doesn't), I found that this caused a compile error. I think it should be

Suggested change
missing_rt((duration, f))
missing_rt((deadline, f))

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jplatte if you have a solid repro for acquire timeouts, I'd love to add it as a test here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment