From 250b2cd7bd2481def4bbc83af7ecfc4f1f6a430c Mon Sep 17 00:00:00 2001 From: Marco Bubke Date: Tue, 29 Aug 2023 15:37:23 +0200 Subject: [PATCH] Sqlite: Introduce implicit transaction Sqlite adds an implicit transaction if no transaction is used. That can be problematic if you use multiple statements but it is okay for one read statement. Write and read write statements should use immediate transactions because the acquire the write lock. Otherwise the write lock is only acquired as you try to write. From there it is much harder to recover. Change-Id: I04b0be7447f2b82b6921738d789c33cbbaa8de6e Reviewed-by: Thomas Hartmann Reviewed-by: Qt CI Patch Build Bot --- src/libs/sqlite/sqlitereadstatement.h | 10 ++-- src/libs/sqlite/sqlitetransaction.h | 30 +++++++++++ .../sqlite/sqlitetransaction-test.cpp | 53 +++++++++++++++++++ 3 files changed, 88 insertions(+), 5 deletions(-) diff --git a/src/libs/sqlite/sqlitereadstatement.h b/src/libs/sqlite/sqlitereadstatement.h index 4f8091598b0..04e4a37cf70 100644 --- a/src/libs/sqlite/sqlitereadstatement.h +++ b/src/libs/sqlite/sqlitereadstatement.h @@ -34,7 +34,7 @@ public: template auto valueWithTransaction(const QueryTypes &...queryValues) { - return withDeferredTransaction(Base::database(), [&] { + return withImplicitTransaction(Base::database(), [&] { return Base::template value(queryValues...); }); } @@ -42,7 +42,7 @@ public: template auto optionalValueWithTransaction(const QueryTypes &...queryValues) { - return withDeferredTransaction(Base::database(), [&] { + return withImplicitTransaction(Base::database(), [&] { return Base::template optionalValue(queryValues...); }); } @@ -50,7 +50,7 @@ public: template auto valuesWithTransaction(const QueryTypes &...queryValues) { - return withDeferredTransaction(Base::database(), [&] { + return withImplicitTransaction(Base::database(), [&] { return Base::template values(queryValues...); }); } @@ -58,7 +58,7 @@ public: template void readCallbackWithTransaction(Callable &&callable, const QueryTypes &...queryValues) { - withDeferredTransaction(Base::database(), [&] { + withImplicitTransaction(Base::database(), [&] { Base::readCallback(std::forward(callable), queryValues...); }); } @@ -66,7 +66,7 @@ public: template void readToWithTransaction(Container &container, const QueryTypes &...queryValues) { - withDeferredTransaction(Base::database(), [&] { Base::readTo(container, queryValues...); }); + withImplicitTransaction(Base::database(), [&] { Base::readTo(container, queryValues...); }); } protected: diff --git a/src/libs/sqlite/sqlitetransaction.h b/src/libs/sqlite/sqlitetransaction.h index 2cc4a7bf5fe..85228eb0507 100644 --- a/src/libs/sqlite/sqlitetransaction.h +++ b/src/libs/sqlite/sqlitetransaction.h @@ -67,6 +67,24 @@ protected: bool m_rollback = false; }; +template +class ImplicitTransaction +{ +public: + using Transaction = TransactionInterface; + + ~ImplicitTransaction() = default; + ImplicitTransaction(TransactionInterface &transactionInterface) + : m_locker(transactionInterface) + {} + + ImplicitTransaction(const ImplicitTransaction &) = delete; + ImplicitTransaction &operator=(const ImplicitTransaction &) = delete; + +protected: + std::unique_lock m_locker; +}; + template class AbstractThrowingSessionTransaction { @@ -201,6 +219,18 @@ auto withTransaction(TransactionInterface &transactionInterface, Callable &&call } } +template +auto withImplicitTransaction(TransactionInterface &transactionInterface, Callable &&callable) +{ + ImplicitTransaction transaction{transactionInterface}; + + if constexpr (std::is_void_v>) { + callable(); + } else { + return callable(); + } +} + template auto withDeferredTransaction(TransactionInterface &transactionInterface, Callable &&callable) { diff --git a/tests/unit/tests/unittests/sqlite/sqlitetransaction-test.cpp b/tests/unit/tests/unittests/sqlite/sqlitetransaction-test.cpp index ac98a258c96..9edbfe87488 100644 --- a/tests/unit/tests/unittests/sqlite/sqlitetransaction-test.cpp +++ b/tests/unit/tests/unittests/sqlite/sqlitetransaction-test.cpp @@ -323,6 +323,59 @@ TEST_F(SqliteTransaction, immediate_session_transaction_begin_throws_and_not_rol ASSERT_ANY_THROW(ImmediateSessionTransaction{mockTransactionBackend}); } +TEST_F(SqliteTransaction, with_implicit_transaction_no_return_does_not_commit) +{ + InSequence s; + + EXPECT_CALL(mockTransactionBackend, lock()); + EXPECT_CALL(mockTransactionBackend, deferredBegin()).Times(0); + EXPECT_CALL(callableMock, Call()); + EXPECT_CALL(mockTransactionBackend, commit()).Times(0); + EXPECT_CALL(mockTransactionBackend, unlock()); + + Sqlite::withImplicitTransaction(mockTransactionBackend, callableMock.AsStdFunction()); +} + +TEST_F(SqliteTransaction, with_implicit_transaction_with_return_does_not_commit) +{ + InSequence s; + + EXPECT_CALL(mockTransactionBackend, lock()); + EXPECT_CALL(mockTransactionBackend, deferredBegin()).Times(0); + EXPECT_CALL(callableWithReturnMock, Call()); + EXPECT_CALL(mockTransactionBackend, commit()).Times(0); + EXPECT_CALL(mockTransactionBackend, unlock()); + + Sqlite::withImplicitTransaction(mockTransactionBackend, callableWithReturnMock.AsStdFunction()); +} + +TEST_F(SqliteTransaction, with_implicit_transaction_returns_value) +{ + auto callable = callableWithReturnMock.AsStdFunction(); + + auto value = Sqlite::withImplicitTransaction(mockTransactionBackend, + callableWithReturnMock.AsStdFunction()); + + ASSERT_THAT(value, Eq(212)); +} + +TEST_F(SqliteTransaction, with_implicit_transaction_do_calls_rollsback_for_exception) +{ + InSequence s; + ON_CALL(callableMock, Call()).WillByDefault(Throw(std::exception{})); + + EXPECT_CALL(mockTransactionBackend, lock()); + EXPECT_CALL(mockTransactionBackend, deferredBegin()).Times(0); + EXPECT_CALL(callableMock, Call()); + EXPECT_CALL(mockTransactionBackend, rollback()).Times(0); + EXPECT_CALL(mockTransactionBackend, unlock()); + + try { + Sqlite::withImplicitTransaction(mockTransactionBackend, callableMock.AsStdFunction()); + } catch (...) { + } +} + TEST_F(SqliteTransaction, with_deferred_transaction_no_return_commit) { InSequence s;