Allow sync drops for 'sync_db_pools' connections.

Prior to this commit, connections from 'sync_db_pools' assumed that they
were being dropped in an async context. This is overwhelmingly the
common case as connections are typically dropped immediately after
request processing. Nothing requires this, however, so holding a
connection beyond the scope of the async context was possible (i.e. by
storing a connection in managed state). Given the connection's `Drop`
impl calls `spawn_blocking`, this resulted in a panic on drop.

This commit resolves the issue by modifying `Drop` so that it calls
`spawn_blocking` only when it is executing inside an async context. If
not, the connection is dropped normally, without `spawn_blocking`.
This commit is contained in:
Sergio Benitez 2024-08-18 15:44:30 -07:00
parent 7fdcf2d1ed
commit 327b1ad064
3 changed files with 54 additions and 12 deletions

View File

@ -48,5 +48,11 @@ default-features = false
[build-dependencies]
version_check = "0.9.1"
[dev-dependencies.rocket]
version = "0.6.0-dev"
path = "../../../core/lib"
default-features = false
features = ["trace"]
[package.metadata.docs.rs]
all-features = true

View File

@ -182,31 +182,42 @@ impl<K, C: Poolable> Drop for Connection<K, C> {
let connection = self.connection.clone();
let permit = self.permit.take();
// See same motivation above for this arrangement of spawn_blocking/block_on
tokio::task::spawn_blocking(move || {
let mut connection = tokio::runtime::Handle::current().block_on(async {
connection.lock_owned().await
});
// Only use spawn_blocking if the Tokio runtime is still available
if let Ok(handle) = tokio::runtime::Handle::try_current() {
// See above for motivation of this arrangement of spawn_blocking/block_on
handle.spawn_blocking(move || {
let mut connection = tokio::runtime::Handle::current()
.block_on(async { connection.lock_owned().await });
if let Some(conn) = connection.take() {
drop(conn);
}
// Explicitly dropping the permit here so that it's only
// released after the connection is.
drop(permit);
});
} else {
warn!(type_name = std::any::type_name::<K>(),
"database connection is being dropped outside of an async context\n\
this means you have stored a connection beyond a request's lifetime\n\
this is not recommended: connections are not valid indefinitely\n\
instead, store a connection pool and get connections as needed");
if let Some(conn) = connection.blocking_lock().take() {
drop(conn);
}
}
// Explicitly drop permit here to release only after dropping connection.
drop(permit);
}
}
impl<K, C: Poolable> Drop for ConnectionPool<K, C> {
fn drop(&mut self) {
// Use spawn_blocking if the Tokio runtime is still available. Otherwise
// the pool will be dropped on the current thread.
let pool = self.pool.take();
// Only use spawn_blocking if the Tokio runtime is still available
if let Ok(handle) = tokio::runtime::Handle::try_current() {
handle.spawn_blocking(move || drop(pool));
}
// Otherwise the pool will be dropped on the current thread
}
}

View File

@ -0,0 +1,25 @@
#![cfg(feature = "diesel_sqlite_pool")]
use rocket::figment::Figment;
use rocket_sync_db_pools::database;
#[database("example")]
struct ExampleDb(diesel::SqliteConnection);
#[test]
fn can_drop_connection_in_sync_context() {
let conn = rocket::execute(async {
let figment = Figment::from(rocket::Config::debug_default())
.merge(("databases.example.url", ":memory:"));
let rocket = rocket::custom(figment)
.attach(ExampleDb::fairing())
.ignite().await
.expect("rocket");
ExampleDb::get_one(&rocket).await
.expect("attach => connection")
});
drop(conn);
}