diff --git a/src/libs/sqlite/sqlitedatabase.cpp b/src/libs/sqlite/sqlitedatabase.cpp index 0da9acfe314..a90c8caff0f 100644 --- a/src/libs/sqlite/sqlitedatabase.cpp +++ b/src/libs/sqlite/sqlitedatabase.cpp @@ -35,13 +35,29 @@ using namespace std::chrono_literals; namespace Sqlite { +class Database::Statements +{ +public: + Statements(Database &database) + : database(database) + {} + +public: + Database &database; + ReadWriteStatement deferredBegin{"BEGIN", database}; + ReadWriteStatement immediateBegin{"BEGIN IMMEDIATE", database}; + ReadWriteStatement exclusiveBegin{"BEGIN EXCLUSIVE", database}; + ReadWriteStatement commitBegin{"COMMIT", database}; + ReadWriteStatement rollbackBegin{"ROLLBACK", database}; +}; + Database::Database() : m_databaseBackend(*this) { } Database::Database(Utils::PathString &&databaseFilePath, JournalMode journalMode) - : Database(std::move(databaseFilePath), 0ms, journalMode) + : Database(std::move(databaseFilePath), 1000ms, journalMode) { } @@ -76,6 +92,7 @@ void Database::open(Utils::PathString &&databaseFilePath) void Database::close() { m_isOpen = false; + deleteTransactionStatements(); m_databaseBackend.close(); } @@ -147,40 +164,45 @@ void Database::initializeTables() void Database::registerTransactionStatements() { - m_deferredBeginStatement = std::make_unique("BEGIN", *this); - m_immediateBeginStatement = std::make_unique("BEGIN IMMEDIATE", *this); - m_exclusiveBeginStatement = std::make_unique("BEGIN EXCLUSIVE", *this); - m_commitBeginStatement = std::make_unique("COMMIT", *this); - m_rollbackBeginStatement = std::make_unique("ROLLBACK", *this); + m_statements = std::make_unique(*this); +} + +void Database::deleteTransactionStatements() +{ + m_statements.reset(); } void Database::deferredBegin() { - m_databaseMutex.lock(); - m_deferredBeginStatement->execute(); + m_statements->deferredBegin.execute(); } void Database::immediateBegin() { - m_databaseMutex.lock(); - m_immediateBeginStatement->execute(); + m_statements->immediateBegin.execute(); } void Database::exclusiveBegin() { - m_databaseMutex.lock(); - m_exclusiveBeginStatement->execute(); + m_statements->exclusiveBegin.execute(); } void Database::commit() { - m_commitBeginStatement->execute(); - m_databaseMutex.unlock(); + m_statements->commitBegin.execute(); } void Database::rollback() { - m_rollbackBeginStatement->execute(); + m_statements->rollbackBegin.execute(); +} + +void Database::lock() +{ + m_databaseMutex.lock(); +} +void Database::unlock() +{ m_databaseMutex.unlock(); } diff --git a/src/libs/sqlite/sqlitedatabase.h b/src/libs/sqlite/sqlitedatabase.h index c1a82b389d7..ad82f6eaa62 100644 --- a/src/libs/sqlite/sqlitedatabase.h +++ b/src/libs/sqlite/sqlitedatabase.h @@ -38,9 +38,10 @@ namespace Sqlite { +using namespace std::chrono_literals; + class ReadStatement; class WriteStatement; -class ReadWriteStatement; class SQLITE_EXPORT Database final : public TransactionInterface { @@ -55,10 +56,10 @@ public: Database(); Database(Utils::PathString &&databaseFilePath, - JournalMode journalMode=JournalMode::Delete); + JournalMode journalMode=JournalMode::Wal); Database(Utils::PathString &&databaseFilePath, - std::chrono::milliseconds busyTimeout = {}, - JournalMode journalMode=JournalMode::Delete); + std::chrono::milliseconds busyTimeout = 1000ms, + JournalMode journalMode=JournalMode::Wal); ~Database(); Database(const Database &) = delete; @@ -106,27 +107,28 @@ public: return m_databaseBackend.totalChangesCount(); } +private: void deferredBegin(); void immediateBegin(); void exclusiveBegin(); void commit(); void rollback(); + void lock(); + void unlock(); -private: void initializeTables(); void registerTransactionStatements(); + void deleteTransactionStatements(); std::mutex &databaseMutex() { return m_databaseMutex; } + class Statements; + private: Utils::PathString m_databaseFilePath; DatabaseBackend m_databaseBackend; std::vector m_sqliteTables; - std::unique_ptr m_deferredBeginStatement; - std::unique_ptr m_immediateBeginStatement; - std::unique_ptr m_exclusiveBeginStatement; - std::unique_ptr m_commitBeginStatement; - std::unique_ptr m_rollbackBeginStatement; std::mutex m_databaseMutex; + std::unique_ptr m_statements; std::chrono::milliseconds m_busyTimeout; JournalMode m_journalMode = JournalMode::Wal; OpenMode m_openMode = OpenMode::ReadWrite; diff --git a/src/libs/sqlite/sqlitetransaction.h b/src/libs/sqlite/sqlitetransaction.h index 43d91e54705..0c053a00375 100644 --- a/src/libs/sqlite/sqlitetransaction.h +++ b/src/libs/sqlite/sqlitetransaction.h @@ -47,6 +47,8 @@ public: virtual void exclusiveBegin() = 0; virtual void commit() = 0; virtual void rollback() = 0; + virtual void lock() = 0; + virtual void unlock() = 0; protected: ~TransactionInterface() = default; @@ -74,6 +76,7 @@ protected: protected: TransactionInterface &m_interface; + std::lock_guard m_locker{m_interface}; bool m_isAlreadyCommited = false; bool m_rollback = false; }; diff --git a/tests/unit/unittest/filepathstorage-test.cpp b/tests/unit/unittest/filepathstorage-test.cpp index f20ee522251..2692f9f79d0 100644 --- a/tests/unit/unittest/filepathstorage-test.cpp +++ b/tests/unit/unittest/filepathstorage-test.cpp @@ -419,11 +419,15 @@ TEST_F(FilePathStorage, RestartFetchSourceNameIfTheDatabaseIsBusyInBegin) { InSequence s; + EXPECT_CALL(mockDatabase, lock()); EXPECT_CALL(mockDatabase, deferredBegin()).WillOnce(Throw(Sqlite::StatementIsBusy("busy"))); EXPECT_CALL(mockDatabase, rollback()).Times(0); + EXPECT_CALL(mockDatabase, unlock()); + EXPECT_CALL(mockDatabase, lock()); EXPECT_CALL(mockDatabase, deferredBegin()); EXPECT_CALL(selectSourceNameFromSourcesBySourceId, valueReturnSmallString(42)); EXPECT_CALL(mockDatabase, commit()); + EXPECT_CALL(mockDatabase, unlock()); storage.fetchSourceName(42); } @@ -432,10 +436,15 @@ TEST_F(FilePathStorage, RestartFetchDirectoryPathIfTheDatabaseIsBusyInBegin) { InSequence s; + EXPECT_CALL(mockDatabase, lock()); EXPECT_CALL(mockDatabase, deferredBegin()).WillOnce(Throw(Sqlite::StatementIsBusy("busy"))); EXPECT_CALL(mockDatabase, rollback()).Times(0); + EXPECT_CALL(mockDatabase, unlock()); + EXPECT_CALL(mockDatabase, lock()); EXPECT_CALL(mockDatabase, deferredBegin()); - EXPECT_CALL(selectDirectoryPathFromDirectoriesByDirectoryId, valueReturnPathString(5)); EXPECT_CALL(mockDatabase, commit()); + EXPECT_CALL(selectDirectoryPathFromDirectoriesByDirectoryId, valueReturnPathString(5)); + EXPECT_CALL(mockDatabase, commit()); + EXPECT_CALL(mockDatabase, unlock()); storage.fetchDirectoryPath(5); } @@ -444,11 +453,15 @@ TEST_F(FilePathStorage, RestartFetchAllDirectoriesIfBeginIsBusy) { InSequence s; + EXPECT_CALL(mockDatabase, lock()); EXPECT_CALL(mockDatabase, deferredBegin()).WillOnce(Throw(Sqlite::StatementIsBusy("busy"))); EXPECT_CALL(mockDatabase, rollback()).Times(0); + EXPECT_CALL(mockDatabase, unlock()); + EXPECT_CALL(mockDatabase, lock()); EXPECT_CALL(mockDatabase, deferredBegin()); EXPECT_CALL(selectAllDirectories, valuesReturnStdVectorDirectory(256)); EXPECT_CALL(mockDatabase, commit()); + EXPECT_CALL(mockDatabase, unlock()); storage.fetchAllDirectories(); } @@ -457,11 +470,15 @@ TEST_F(FilePathStorage, RestartFetchAllSourcesIfBeginIsBusy) { InSequence s; + EXPECT_CALL(mockDatabase, lock()); EXPECT_CALL(mockDatabase, deferredBegin()).WillOnce(Throw(Sqlite::StatementIsBusy("busy"))); EXPECT_CALL(mockDatabase, rollback()).Times(0); + EXPECT_CALL(mockDatabase, unlock()); + EXPECT_CALL(mockDatabase, lock()); EXPECT_CALL(mockDatabase, deferredBegin()); EXPECT_CALL(selectAllSources, valuesReturnStdVectorSource(8192)); EXPECT_CALL(mockDatabase, commit()); + EXPECT_CALL(mockDatabase, unlock()); storage.fetchAllSources(); } diff --git a/tests/unit/unittest/mocksqlitetransactionbackend.h b/tests/unit/unittest/mocksqlitetransactionbackend.h index c367404530e..0cfeee2892d 100644 --- a/tests/unit/unittest/mocksqlitetransactionbackend.h +++ b/tests/unit/unittest/mocksqlitetransactionbackend.h @@ -38,4 +38,6 @@ public: MOCK_METHOD0(exclusiveBegin, void ()); MOCK_METHOD0(commit, void ()); MOCK_METHOD0(rollback, void ()); + MOCK_METHOD0(lock, void ()); + MOCK_METHOD0(unlock, void ()); }; diff --git a/tests/unit/unittest/sqlitedatabase-test.cpp b/tests/unit/unittest/sqlitedatabase-test.cpp index ca389f58fb4..8ccc2f652c6 100644 --- a/tests/unit/unittest/sqlitedatabase-test.cpp +++ b/tests/unit/unittest/sqlitedatabase-test.cpp @@ -47,12 +47,27 @@ using Sqlite::Table; class SqliteDatabase : public ::testing::Test { protected: - void SetUp() override; - void TearDown() override; + void SetUp() override + { + database.setJournalMode(JournalMode::Memory); + database.setDatabaseFilePath(databaseFilePath); + auto &table = database.addTable(); + table.setName("test"); + table.addColumn("name"); + + database.open(); + } + + void TearDown() override + { + if (database.isOpen()) + database.close(); + } SpyDummy spyDummy; - QString databaseFilePath = QStringLiteral(":memory:"); + QString databaseFilePath{":memory:"}; Sqlite::Database database; + Sqlite::TransactionInterface &transactionInterface = database; }; TEST_F(SqliteDatabase, SetDatabaseFilePath) @@ -142,53 +157,37 @@ TEST_F(SqliteDatabase, LastRowId) TEST_F(SqliteDatabase, DeferredBegin) { - ASSERT_ANY_THROW(database.deferredBegin()); + ASSERT_NO_THROW(transactionInterface.deferredBegin()); - database.commit(); + transactionInterface.commit(); } TEST_F(SqliteDatabase, ImmediateBegin) { - ASSERT_ANY_THROW(database.immediateBegin()); + ASSERT_NO_THROW(transactionInterface.immediateBegin()); - database.commit(); + transactionInterface.commit(); } TEST_F(SqliteDatabase, ExclusiveBegin) { - ASSERT_ANY_THROW(database.exclusiveBegin()); + ASSERT_NO_THROW(transactionInterface.exclusiveBegin()); - database.commit(); + transactionInterface.commit(); } TEST_F(SqliteDatabase, Commit) { - database.deferredBegin(); + transactionInterface.deferredBegin(); - ASSERT_ANY_THROW(database.commit()); + ASSERT_NO_THROW(transactionInterface.commit()); } TEST_F(SqliteDatabase, Rollback) { - database.deferredBegin(); + transactionInterface.deferredBegin(); - ASSERT_ANY_THROW(database.rollback()); + ASSERT_NO_THROW(transactionInterface.rollback()); } -void SqliteDatabase::SetUp() -{ - database.setJournalMode(JournalMode::Memory); - database.setDatabaseFilePath(databaseFilePath); - auto &table = database.addTable(); - table.setName("test"); - table.addColumn("name"); - - database.open(); -} - -void SqliteDatabase::TearDown() -{ - if (database.isOpen()) - database.close(); -} } diff --git a/tests/unit/unittest/sqlitetransaction-test.cpp b/tests/unit/unittest/sqlitetransaction-test.cpp index 96da6454621..dc3e5bba70b 100644 --- a/tests/unit/unittest/sqlitetransaction-test.cpp +++ b/tests/unit/unittest/sqlitetransaction-test.cpp @@ -48,8 +48,12 @@ protected: TEST_F(SqliteTransaction, DeferredTransactionCommit) { + InSequence s; + + EXPECT_CALL(mockTransactionBackend, lock()); EXPECT_CALL(mockTransactionBackend, deferredBegin()); EXPECT_CALL(mockTransactionBackend, commit()); + EXPECT_CALL(mockTransactionBackend, unlock()); DeferredTransaction transaction{mockTransactionBackend}; transaction.commit(); @@ -57,8 +61,12 @@ TEST_F(SqliteTransaction, DeferredTransactionCommit) TEST_F(SqliteTransaction, DeferredTransactionCommitCallsInterface) { + InSequence s; + + EXPECT_CALL(mockTransactionBackend, lock()); EXPECT_CALL(mockTransactionBackend, deferredBegin()); EXPECT_CALL(mockTransactionBackend, commit()); + EXPECT_CALL(mockTransactionBackend, unlock()); DeferredTransaction transaction{mockTransactionBackend}; transaction.commit(); @@ -66,16 +74,24 @@ TEST_F(SqliteTransaction, DeferredTransactionCommitCallsInterface) TEST_F(SqliteTransaction, DeferredTransactionRollBack) { + InSequence s; + + EXPECT_CALL(mockTransactionBackend, lock()); EXPECT_CALL(mockTransactionBackend, deferredBegin()); EXPECT_CALL(mockTransactionBackend, rollback()); + EXPECT_CALL(mockTransactionBackend, unlock()); DeferredTransaction transaction{mockTransactionBackend}; } TEST_F(SqliteTransaction, ImmediateTransactionCommit) { + InSequence s; + + EXPECT_CALL(mockTransactionBackend, lock()); EXPECT_CALL(mockTransactionBackend, immediateBegin()); EXPECT_CALL(mockTransactionBackend, commit()); + EXPECT_CALL(mockTransactionBackend, unlock()); ImmediateTransaction transaction{mockTransactionBackend}; transaction.commit(); @@ -83,16 +99,24 @@ TEST_F(SqliteTransaction, ImmediateTransactionCommit) TEST_F(SqliteTransaction, ImmediateTransactionRollBack) { + InSequence s; + + EXPECT_CALL(mockTransactionBackend, lock()); EXPECT_CALL(mockTransactionBackend, immediateBegin()); EXPECT_CALL(mockTransactionBackend, rollback()); + EXPECT_CALL(mockTransactionBackend, unlock()); ImmediateTransaction transaction{mockTransactionBackend}; } TEST_F(SqliteTransaction, ExclusiveTransactionCommit) { + InSequence s; + + EXPECT_CALL(mockTransactionBackend, lock()); EXPECT_CALL(mockTransactionBackend, exclusiveBegin()); EXPECT_CALL(mockTransactionBackend, commit()); + EXPECT_CALL(mockTransactionBackend, unlock()); ExclusiveTransaction transaction{mockTransactionBackend}; transaction.commit(); @@ -100,16 +124,24 @@ TEST_F(SqliteTransaction, ExclusiveTransactionCommit) TEST_F(SqliteTransaction, ExclusiveTransactionRollBack) { + InSequence s; + + EXPECT_CALL(mockTransactionBackend, lock()); EXPECT_CALL(mockTransactionBackend, exclusiveBegin()); EXPECT_CALL(mockTransactionBackend, rollback()); + EXPECT_CALL(mockTransactionBackend, unlock()); ExclusiveTransaction transaction{mockTransactionBackend}; } TEST_F(SqliteTransaction, DeferredNonThrowingDestructorTransactionCommit) { + InSequence s; + + EXPECT_CALL(mockTransactionBackend, lock()); EXPECT_CALL(mockTransactionBackend, deferredBegin()); EXPECT_CALL(mockTransactionBackend, commit()); + EXPECT_CALL(mockTransactionBackend, unlock()); DeferredNonThrowingDestructorTransaction transaction{mockTransactionBackend}; transaction.commit(); @@ -117,8 +149,12 @@ TEST_F(SqliteTransaction, DeferredNonThrowingDestructorTransactionCommit) TEST_F(SqliteTransaction, DeferredNonThrowingDestructorTransactionCommitCallsInterface) { + InSequence s; + + EXPECT_CALL(mockTransactionBackend, lock()); EXPECT_CALL(mockTransactionBackend, deferredBegin()); EXPECT_CALL(mockTransactionBackend, commit()); + EXPECT_CALL(mockTransactionBackend, unlock()); DeferredNonThrowingDestructorTransaction transaction{mockTransactionBackend}; transaction.commit(); @@ -126,16 +162,24 @@ TEST_F(SqliteTransaction, DeferredNonThrowingDestructorTransactionCommitCallsInt TEST_F(SqliteTransaction, DeferredNonThrowingDestructorTransactionRollBack) { + InSequence s; + + EXPECT_CALL(mockTransactionBackend, lock()); EXPECT_CALL(mockTransactionBackend, deferredBegin()); EXPECT_CALL(mockTransactionBackend, rollback()); + EXPECT_CALL(mockTransactionBackend, unlock()); DeferredNonThrowingDestructorTransaction transaction{mockTransactionBackend}; } TEST_F(SqliteTransaction, ImmediateNonThrowingDestructorTransactionCommit) { + InSequence s; + + EXPECT_CALL(mockTransactionBackend, lock()); EXPECT_CALL(mockTransactionBackend, immediateBegin()); EXPECT_CALL(mockTransactionBackend, commit()); + EXPECT_CALL(mockTransactionBackend, unlock()); ImmediateNonThrowingDestructorTransaction transaction{mockTransactionBackend}; transaction.commit(); @@ -143,16 +187,24 @@ TEST_F(SqliteTransaction, ImmediateNonThrowingDestructorTransactionCommit) TEST_F(SqliteTransaction, ImmediateNonThrowingDestructorTransactionRollBack) { + InSequence s; + + EXPECT_CALL(mockTransactionBackend, lock()); EXPECT_CALL(mockTransactionBackend, immediateBegin()); EXPECT_CALL(mockTransactionBackend, rollback()); + EXPECT_CALL(mockTransactionBackend, unlock()); ImmediateNonThrowingDestructorTransaction transaction{mockTransactionBackend}; } TEST_F(SqliteTransaction, ExclusiveNonThrowingDestructorTransactionCommit) { + InSequence s; + + EXPECT_CALL(mockTransactionBackend, lock()); EXPECT_CALL(mockTransactionBackend, exclusiveBegin()); EXPECT_CALL(mockTransactionBackend, commit()); + EXPECT_CALL(mockTransactionBackend, unlock()); ExclusiveNonThrowingDestructorTransaction transaction{mockTransactionBackend}; transaction.commit(); @@ -160,8 +212,12 @@ TEST_F(SqliteTransaction, ExclusiveNonThrowingDestructorTransactionCommit) TEST_F(SqliteTransaction, ExclusiveTNonThrowingDestructorransactionRollBack) { + InSequence s; + + EXPECT_CALL(mockTransactionBackend, lock()); EXPECT_CALL(mockTransactionBackend, exclusiveBegin()); EXPECT_CALL(mockTransactionBackend, rollback()); + EXPECT_CALL(mockTransactionBackend, unlock()); ExclusiveNonThrowingDestructorTransaction transaction{mockTransactionBackend}; } @@ -195,30 +251,40 @@ TEST_F(SqliteTransaction, ExclusiveTransactionBeginThrows) TEST_F(SqliteTransaction, DeferredTransactionBeginThrowsAndNotRollback) { + InSequence s; + + EXPECT_CALL(mockTransactionBackend, lock()); EXPECT_CALL(mockTransactionBackend, deferredBegin()) .WillOnce(Throw(Sqlite::Exception("foo"))); - EXPECT_CALL(mockTransactionBackend, rollback()).Times(0); + EXPECT_CALL(mockTransactionBackend, unlock()); ASSERT_ANY_THROW(DeferredTransaction{mockTransactionBackend}); } TEST_F(SqliteTransaction, ImmediateTransactionBeginThrowsAndNotRollback) { + InSequence s; + + EXPECT_CALL(mockTransactionBackend, lock()); EXPECT_CALL(mockTransactionBackend, immediateBegin()) .WillOnce(Throw(Sqlite::Exception("foo"))); - EXPECT_CALL(mockTransactionBackend, rollback()).Times(0); + EXPECT_CALL(mockTransactionBackend, unlock()); + ASSERT_ANY_THROW(ImmediateTransaction{mockTransactionBackend}); } TEST_F(SqliteTransaction, ExclusiveTransactionBeginThrowsAndNotRollback) { + InSequence s; + + EXPECT_CALL(mockTransactionBackend, lock()); EXPECT_CALL(mockTransactionBackend, exclusiveBegin()) .WillOnce(Throw(Sqlite::Exception("foo"))); - EXPECT_CALL(mockTransactionBackend, rollback()).Times(0); + EXPECT_CALL(mockTransactionBackend, unlock()); ASSERT_ANY_THROW(ExclusiveTransaction{mockTransactionBackend}); }