From 5870905db6588121e259d0927de35e838dd2df7e Mon Sep 17 00:00:00 2001 From: Marco Bubke Date: Wed, 28 Mar 2018 16:25:01 +0200 Subject: [PATCH] Clang: Handle a busy database in the PCH plugin This can be always happen for write statements. It fixes the wrong behavior of the transaction that it tried to rollback if begin fails. If begin fails the transaction never started so there is nothing to rollback. Change-Id: I8a03162257fa22a0bb66ccb844f90c6afbc7db64 Reviewed-by: Ivan Donchevskii --- src/libs/sqlite/sqlitetransaction.h | 21 ++++- .../precompiledheaderstorage.h | 23 ++++- .../precompiledheaderstorage-test.cpp | 33 +++++++ .../unit/unittest/sqlitetransaction-test.cpp | 90 +++++++++++++++++++ 4 files changed, 162 insertions(+), 5 deletions(-) diff --git a/src/libs/sqlite/sqlitetransaction.h b/src/libs/sqlite/sqlitetransaction.h index ce15fa96620..43d91e54705 100644 --- a/src/libs/sqlite/sqlitetransaction.h +++ b/src/libs/sqlite/sqlitetransaction.h @@ -71,9 +71,11 @@ protected: { } + protected: TransactionInterface &m_interface; bool m_isAlreadyCommited = false; + bool m_rollback = false; }; class AbstractThrowingTransaction : public AbstractTransaction @@ -84,7 +86,7 @@ public: ~AbstractThrowingTransaction() noexcept(false) { try { - if (!m_isAlreadyCommited) + if (m_rollback) m_interface.rollback(); } catch (...) { if (!std::uncaught_exception()) @@ -107,7 +109,7 @@ public: ~AbstractNonThrowingDestructorTransaction() { try { - if (!m_isAlreadyCommited) + if (m_rollback) m_interface.rollback(); } catch (...) { } @@ -129,6 +131,11 @@ public: { interface.deferredBegin(); } + + ~BasicDeferredTransaction() + { + BaseTransaction::m_rollback = !BaseTransaction::m_isAlreadyCommited; + } }; using DeferredTransaction = BasicDeferredTransaction; @@ -143,6 +150,11 @@ public: { interface.immediateBegin(); } + + ~BasicImmediateTransaction() + { + BaseTransaction::m_rollback = !BaseTransaction::m_isAlreadyCommited; + } }; using ImmediateTransaction = BasicImmediateTransaction; @@ -157,6 +169,11 @@ public: { interface.exclusiveBegin(); } + + ~BasicExclusiveTransaction() + { + BaseTransaction::m_rollback = !BaseTransaction::m_isAlreadyCommited; + } }; using ExclusiveTransaction = BasicExclusiveTransaction; diff --git a/src/plugins/clangpchmanager/precompiledheaderstorage.h b/src/plugins/clangpchmanager/precompiledheaderstorage.h index 682e9c3492e..4ad57e980a5 100644 --- a/src/plugins/clangpchmanager/precompiledheaderstorage.h +++ b/src/plugins/clangpchmanager/precompiledheaderstorage.h @@ -28,6 +28,7 @@ #include "precompiledheaderstorageinterface.h" #include +#include #include @@ -50,13 +51,29 @@ public: Utils::SmallStringView pchPath, long long pchBuildTime) override { - m_insertProjectPartStatement.write(projectPartName); - m_insertPrecompiledHeaderStatement .write(projectPartName, pchPath, pchBuildTime); + try { + Sqlite::ImmediateTransaction transaction{m_database}; + + m_insertProjectPartStatement.write(projectPartName); + m_insertPrecompiledHeaderStatement .write(projectPartName, pchPath, pchBuildTime); + + transaction.commit(); + } catch (const Sqlite::StatementIsBusy) { + insertPrecompiledHeader(projectPartName, pchPath, pchBuildTime); + } } void deletePrecompiledHeader(Utils::SmallStringView projectPartName) override { - m_deletePrecompiledHeaderStatement.write(projectPartName); + try { + Sqlite::ImmediateTransaction transaction{m_database}; + + m_deletePrecompiledHeaderStatement.write(projectPartName); + + transaction.commit(); + } catch (const Sqlite::StatementIsBusy) { + deletePrecompiledHeader(projectPartName); + } } diff --git a/tests/unit/unittest/precompiledheaderstorage-test.cpp b/tests/unit/unittest/precompiledheaderstorage-test.cpp index 2871e3fd112..86f46156cf1 100644 --- a/tests/unit/unittest/precompiledheaderstorage-test.cpp +++ b/tests/unit/unittest/precompiledheaderstorage-test.cpp @@ -30,6 +30,7 @@ #include #include #include +#include namespace { @@ -59,11 +60,29 @@ TEST_F(PrecompiledHeaderStorage, InsertPrecompiledHeader) { InSequence s; + EXPECT_CALL(database, immediateBegin()); EXPECT_CALL(insertProjectPartStatement, write(TypedEq("project1"))); EXPECT_CALL(insertPrecompiledHeaderStatement, write(TypedEq("project1"), TypedEq("/path/to/pch"), TypedEq(22))); + EXPECT_CALL(database, commit()); + + storage.insertPrecompiledHeader("project1", "/path/to/pch", 22); +} + +TEST_F(PrecompiledHeaderStorage, InsertPrecompiledHeaderStatementIsBusy) +{ + InSequence s; + + EXPECT_CALL(database, immediateBegin()).WillOnce(Throw(Sqlite::StatementIsBusy("busy"))); + EXPECT_CALL(database, immediateBegin()); + EXPECT_CALL(insertProjectPartStatement, write(TypedEq("project1"))); + EXPECT_CALL(insertPrecompiledHeaderStatement, + write(TypedEq("project1"), + TypedEq("/path/to/pch"), + TypedEq(22))); + EXPECT_CALL(database, commit()); storage.insertPrecompiledHeader("project1", "/path/to/pch", 22); } @@ -72,7 +91,21 @@ TEST_F(PrecompiledHeaderStorage, DeletePrecompiledHeader) { InSequence s; + EXPECT_CALL(database, immediateBegin()); EXPECT_CALL(deletePrecompiledHeaderStatement, write(TypedEq("project1"))); + EXPECT_CALL(database, commit()); + + storage.deletePrecompiledHeader("project1"); +} + +TEST_F(PrecompiledHeaderStorage, DeletePrecompiledHeaderStatementIsBusy) +{ + InSequence s; + + EXPECT_CALL(database, immediateBegin()).WillOnce(Throw(Sqlite::StatementIsBusy("busy"))); + EXPECT_CALL(database, immediateBegin()); + EXPECT_CALL(deletePrecompiledHeaderStatement, write(TypedEq("project1"))); + EXPECT_CALL(database, commit()); storage.deletePrecompiledHeader("project1"); } diff --git a/tests/unit/unittest/sqlitetransaction-test.cpp b/tests/unit/unittest/sqlitetransaction-test.cpp index 2b05fd6d847..96da6454621 100644 --- a/tests/unit/unittest/sqlitetransaction-test.cpp +++ b/tests/unit/unittest/sqlitetransaction-test.cpp @@ -106,6 +106,66 @@ TEST_F(SqliteTransaction, ExclusiveTransactionRollBack) ExclusiveTransaction transaction{mockTransactionBackend}; } +TEST_F(SqliteTransaction, DeferredNonThrowingDestructorTransactionCommit) +{ + EXPECT_CALL(mockTransactionBackend, deferredBegin()); + EXPECT_CALL(mockTransactionBackend, commit()); + + DeferredNonThrowingDestructorTransaction transaction{mockTransactionBackend}; + transaction.commit(); +} + +TEST_F(SqliteTransaction, DeferredNonThrowingDestructorTransactionCommitCallsInterface) +{ + EXPECT_CALL(mockTransactionBackend, deferredBegin()); + EXPECT_CALL(mockTransactionBackend, commit()); + + DeferredNonThrowingDestructorTransaction transaction{mockTransactionBackend}; + transaction.commit(); +} + +TEST_F(SqliteTransaction, DeferredNonThrowingDestructorTransactionRollBack) +{ + EXPECT_CALL(mockTransactionBackend, deferredBegin()); + EXPECT_CALL(mockTransactionBackend, rollback()); + + DeferredNonThrowingDestructorTransaction transaction{mockTransactionBackend}; +} + +TEST_F(SqliteTransaction, ImmediateNonThrowingDestructorTransactionCommit) +{ + EXPECT_CALL(mockTransactionBackend, immediateBegin()); + EXPECT_CALL(mockTransactionBackend, commit()); + + ImmediateNonThrowingDestructorTransaction transaction{mockTransactionBackend}; + transaction.commit(); +} + +TEST_F(SqliteTransaction, ImmediateNonThrowingDestructorTransactionRollBack) +{ + EXPECT_CALL(mockTransactionBackend, immediateBegin()); + EXPECT_CALL(mockTransactionBackend, rollback()); + + ImmediateNonThrowingDestructorTransaction transaction{mockTransactionBackend}; +} + +TEST_F(SqliteTransaction, ExclusiveNonThrowingDestructorTransactionCommit) +{ + EXPECT_CALL(mockTransactionBackend, exclusiveBegin()); + EXPECT_CALL(mockTransactionBackend, commit()); + + ExclusiveNonThrowingDestructorTransaction transaction{mockTransactionBackend}; + transaction.commit(); +} + +TEST_F(SqliteTransaction, ExclusiveTNonThrowingDestructorransactionRollBack) +{ + EXPECT_CALL(mockTransactionBackend, exclusiveBegin()); + EXPECT_CALL(mockTransactionBackend, rollback()); + + ExclusiveNonThrowingDestructorTransaction transaction{mockTransactionBackend}; +} + TEST_F(SqliteTransaction, DeferredTransactionBeginThrows) { ON_CALL(mockTransactionBackend, deferredBegin()) @@ -133,6 +193,36 @@ TEST_F(SqliteTransaction, ExclusiveTransactionBeginThrows) Sqlite::Exception); } +TEST_F(SqliteTransaction, DeferredTransactionBeginThrowsAndNotRollback) +{ + EXPECT_CALL(mockTransactionBackend, deferredBegin()) + .WillOnce(Throw(Sqlite::Exception("foo"))); + + EXPECT_CALL(mockTransactionBackend, rollback()).Times(0); + + ASSERT_ANY_THROW(DeferredTransaction{mockTransactionBackend}); +} + +TEST_F(SqliteTransaction, ImmediateTransactionBeginThrowsAndNotRollback) +{ + EXPECT_CALL(mockTransactionBackend, immediateBegin()) + .WillOnce(Throw(Sqlite::Exception("foo"))); + + EXPECT_CALL(mockTransactionBackend, rollback()).Times(0); + + ASSERT_ANY_THROW(ImmediateTransaction{mockTransactionBackend}); +} + +TEST_F(SqliteTransaction, ExclusiveTransactionBeginThrowsAndNotRollback) +{ + EXPECT_CALL(mockTransactionBackend, exclusiveBegin()) + .WillOnce(Throw(Sqlite::Exception("foo"))); + + EXPECT_CALL(mockTransactionBackend, rollback()).Times(0); + + ASSERT_ANY_THROW(ExclusiveTransaction{mockTransactionBackend}); +} + TEST_F(SqliteTransaction, TransactionCommitThrows) { ON_CALL(mockTransactionBackend, commit())