From df2dcdabac154f10121accc399a740b9f4dc3125 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Fri, 16 Dec 2022 14:11:51 +0100 Subject: [PATCH] Prevent returning connections to pool with a positive transaction depth (#1283) Mark transaction as closed *only* after commit/rollback succeeds. Previously, `open` on the transaction would be set to `false` prior to attempting to commit or rollback the transaction. When the operation failed, for example, due to a serialization failure with a serializable isolation level, this would leave the transaction in an inconsistent state, where it thought it was closed but really it was still open. The connection would then be returned to the connection pool with a transaction depth of 1, causing a savepoint to be erroneously created the next time a transaction was created for the connection. By waiting to set `open` to `false` until the commit/rollback succeeds, a failure to do either will result in us correctly rolling back the transaction when dropping it, ensuring that the connection is returned to the pool with a transaction depth of 0. Note that this is consistent with how `sqlx` handles transactions. We attempted to write a test, but had a very difficult time forcing postgres to fail to commit a transaction. We found that it would block our requests instead when creating conflicting updates, and we couldn't find any information about when it blocks vs when transaction commits fail. Co-Authored-By: Nathan Sobo Co-authored-by: Nathan Sobo --- src/database/transaction.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/database/transaction.rs b/src/database/transaction.rs index bfb6f2bb..224f6f92 100644 --- a/src/database/transaction.rs +++ b/src/database/transaction.rs @@ -176,7 +176,6 @@ impl DatabaseTransaction { #[instrument(level = "trace")] #[allow(unreachable_code)] pub async fn commit(mut self) -> Result<(), DbErr> { - self.open = false; match *self.conn.lock().await { #[cfg(feature = "sqlx-mysql")] InnerConnection::MySql(ref mut c) => { @@ -201,6 +200,7 @@ impl DatabaseTransaction { c.commit(); } } + self.open = false; Ok(()) } @@ -208,7 +208,6 @@ impl DatabaseTransaction { #[instrument(level = "trace")] #[allow(unreachable_code)] pub async fn rollback(mut self) -> Result<(), DbErr> { - self.open = false; match *self.conn.lock().await { #[cfg(feature = "sqlx-mysql")] InnerConnection::MySql(ref mut c) => { @@ -233,6 +232,7 @@ impl DatabaseTransaction { c.rollback(); } } + self.open = false; Ok(()) }