Dangling transactions when pooled connections dropped

[dae]

Setup

Versions

• Diesel_async: 0.3.1

Feature Flags

• diesel_async: deadpool, postgres

Problem Description

Firstly, thank you for your work on this crate and on Diesel itself! They have been very nice to work with compared to using tokio_postgres directly.

I gather you're aware of this issue already (https://www.reddit.com/r/rust/comments/zn9ut0/announcing_dieselasync_020/), but as dangling transactions can cause deadlocks and data loss, I'm wondering whether it might be possible to improve the current behaviour, or at least document it more prominently if that is not possible.

As a quick-and-dirty solution, would something like the following make sense, where the connection is dropped from the pool (with implicit rollback) when it is recycled with an active transaction?

diff --git a/src/pg/mod.rs b/src/pg/mod.rs
index 6a4832b..b16a0a0 100644
--- a/src/pg/mod.rs
+++ b/src/pg/mod.rs
@@ -475,6 +475,15 @@ async fn lookup_type(
 
 #[cfg(any(feature = "deadpool", feature = "bb8", feature = "mobc"))]
 impl crate::pooled_connection::PoolableConnection for AsyncPgConnection {}
+impl crate::pooled_connection::PoolableConnection for AsyncPgConnection {
+    fn is_broken(&mut self) -> bool {
+        match self.transaction_state().status.transaction_depth() {
+            Ok(Some(_)) => true,
+            Ok(None) => false,
+            Err(_) => true,
+        }
+    }
+}
 
 #[cfg(test)]
 pub mod tests {
diff --git a/src/pooled_connection/mod.rs b/src/pooled_connection/mod.rs
index ae5abdf..9c35d06 100644
--- a/src/pooled_connection/mod.rs
+++ b/src/pooled_connection/mod.rs
@@ -258,7 +258,7 @@ pub trait PoolableConnection: AsyncConnection {
     /// [ManageConnection::has_broken] for details.
     ///
     /// The default implementation does not consider any connection as broken
-    fn is_broken(&self) -> bool {
+    fn is_broken(&mut self) -> bool {
         false
     }
 }

Alternatively, could an approach like tokio_postgres uses work instead?
https://github.com/sfackler/rust-postgres/pull/835/files?

Steps to reproduce

Anything that causes a transaction future to be dropped before being polled to completion, eg the calling future terminating due to client disconnection, or things like timeouts

            let mut pg = pool.get().await?;
            tokio::time::timeout(std::time::Duration::from_secs(1),
                pg.transaction::<_, Error, _>(|pg| async move {
                    sql_query("some sql").execute(pg).await?;
                    tokio::time::sleep(std::time::Duration::from_secs(2)).await;
                    Ok(())
                }.scope_boxed())
            ).await.unwrap_err();

Checklist

• I have already looked over the issue tracker for similar possible closed issues.

• This issue can be reproduced on Rust's stable channel. (Your issue will be
  closed if this is not the case)
• This issue can be reproduced without requiring a third party crate (except deadpool)

[weiznich]

Thanks for opening this issue. I must admit that I thought that part of code was already in place, so it seems like I just missed to do that. Which means I would definitely accept a PR for that, although I would rather change the default is_broken() implementation to do that for all connection implementations.

I'm also interested in other more targeted solutions for this, but I've failed to find something yet that's not coupled to a specific runtime or that would possibly block on drop or swallow errors. I think the postgres approach needs access to the underlying runtime to work. I'm also not user how they handle errors there or guarantee that it happens "fast" after dropping the future (which is somewhat important, so that the relevant future is not open for that long).

[dae]

Turns out there was an existing function which checks for this and was just not hooked up - will push through a PR.

[dae]

Solving it in a generic way does seem tricky. One possible idea for a DB-neutral solution would be a newtype that wraps the various pool implementations, and stores a vec of connections needing cleanup. Each time pool.get() is called, it could use that async context to clean up any of those objects, so if the pool is used with reasonable frequency, the objects in a bad state shouldn't stick around for long. I think deadpool provides a way to take a connection out of the pool, but don't know about bb8 or mobc, and it would add some extra complexity.

[dae]

(sorry, meant to close+re-open the PR to re-run the tests)

[bmwill]

I stumbled upon this issue when I was looking at the docs for AsyncConnection::transaction in order to understand if this warning was still applicable or if this issue being closed means its resolved (and that the docs are just outdated).

would you be able to clarify if this warning could be removed now or if it is still applicable?

Edit: weiznich apologies for pinging you for support in a closed issue, i wasn't aware of the policies you have for maintaining this project.

On closer inspection it seems the warning is probably still applicable for connections not maintained by a connection pool.

[weiznich]

@bmwill I generally do not provide support in old closed issues, especially not if I'm pinged while asking a support question. Use the support forum to ask such questions.