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.

Sign up for GitHub

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

Set reaper rate to minimum value #158

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

Set reaper rate to minimum value #158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ecbf806
Set reaper rate to minimum value
levkk May 3, 2023
70f4317
the else branchn is wrong
levkk May 3, 2023
d6e498c
add idle_timeout test
levkk May 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion bb8/src/api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ use crate::inner::PoolInner;
use crate::internals::Conn;
pub use crate::internals::State;

// Default connection reaper rate
static DEFAULT_REAPER_RATE: Duration = Duration::from_secs(30);

/// A generic connection pool.
pub struct Pool<M>
where
Expand Down Expand Up @@ -111,7 +114,7 @@ impl<M: ManageConnection> Default for Builder<M> {
connection_timeout: Duration::from_secs(30),
retry_connection: true,
error_sink: Box::new(NopErrorSink),
reaper_rate: Duration::from_secs(30),
reaper_rate: DEFAULT_REAPER_RATE,
connection_customizer: None,
_p: PhantomData,
}
Expand Down Expand Up @@ -183,6 +186,11 @@ impl<M: ManageConnection> Builder<M> {
Some(Duration::from_secs(0)),
"max_lifetime must be greater than zero!"
);

if let Some(max_lifetime) = max_lifetime {
Copy link
Owner

Choose a reason for hiding this comment

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

This isn't very precise... if max_lifetime is 30s and the reaper rate is set to 30s as well, if the reaper comes along after 15s it won't reap and then it will be 45s before the max_lifetime is enforced. I'm inclined to think this is more like a documentation problem, and/or maybe we could emit some warnings if there is a large difference between these values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. Alternatively, we can just run the reaper more frequently, e.g. PgBouncer runs its "janitor" 3 times per second.

Copy link
Owner

Choose a reason for hiding this comment

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

That's what the configuration is for. I'm comfortable with the current default configuration as a default, so the goal here is to try and make sure people don't get surprised by the interactions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well... if we look at how the interface is, right now someone can set max_lifetime to 1s and nothing will happen for 30 seconds. So ideally, someone sets that setting to any value they want and the pool just works. reaper_rate is a leaked abstraction imo, it requires the user to understand how the pool works internally.

Copy link
Owner

Choose a reason for hiding this comment

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

I don't disagree that it's a leaky abstraction, my point is more that I don't think it's feasible to avoid the leakage in a fairly general-purpose abstraction like this. So instead of trying do so in a limited way which could be surprising in different ways, I'd prefer clearly documenting how these values work together.

self.reaper_rate = std::cmp::min(DEFAULT_REAPER_RATE, max_lifetime);
}

self.max_lifetime = max_lifetime;
self
}
Expand All @@ -204,6 +212,11 @@ impl<M: ManageConnection> Builder<M> {
Some(Duration::from_secs(0)),
"idle_timeout must be greater than zero!"
);

if let Some(idle_timeout) = idle_timeout {
self.reaper_rate = std::cmp::min(DEFAULT_REAPER_RATE, idle_timeout);
}

self.idle_timeout = idle_timeout;
self
}
Expand Down
56 changes: 55 additions & 1 deletion bb8/tests/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,61 @@ async fn test_max_lifetime() {
let pool = Pool::builder()
.max_lifetime(Some(Duration::from_secs(1)))
.connection_timeout(Duration::from_secs(1))
.reaper_rate(Duration::from_secs(1))
.max_size(5)
.min_idle(Some(5))
.build(manager)
.await
.unwrap();

let (tx1, rx1) = oneshot::channel();
let (tx2, rx2) = oneshot::channel();
let clone = pool.clone();
tokio::spawn(async move {
let conn = clone.get().await.unwrap();
tx1.send(()).unwrap();
// NB: If we sleep here we'll block this thread's event loop, and the
// reaper can't run.
let _ = rx2
.map(|r| match r {
Ok(v) => Ok((v, conn)),
Err(_) => Err((Error, conn)),
})
.await;
});

rx1.await.unwrap();

// And wait.
assert!(timeout(Duration::from_secs(2), pending::<()>())
.await
.is_err());
assert_eq!(DROPPED.load(Ordering::SeqCst), 4);
tx2.send(()).unwrap();

// And wait some more.
assert!(timeout(Duration::from_secs(2), pending::<()>())
.await
.is_err());
assert_eq!(DROPPED.load(Ordering::SeqCst), 5);
}

#[tokio::test]
async fn test_idle_timeout() {
static DROPPED: AtomicUsize = AtomicUsize::new(0);

#[derive(Default)]
struct Connection;

impl Drop for Connection {
fn drop(&mut self) {
DROPPED.fetch_add(1, Ordering::SeqCst);
}
}

let manager = NthConnectionFailManager::<Connection>::new(5);
let pool = Pool::builder()
.idle_timeout(Some(Duration::from_secs(1)))
.connection_timeout(Duration::from_secs(1))
.max_size(5)
.min_idle(Some(5))
.build(manager)
Expand Down