Sqlite: Add locking for sqlite transactions

If you use the database in a multi-threaded environment, you must always
your statements in transactions.

For read-only statements, you use DeferredTransaction and write statements
you use ImmediateTransaction. If you mix read and write statements you have
to use ImmediateTransaction. Don't use DeferredTransaction because it leads
to undefined behavior.

Change-Id: Ida298a20f33423c8da09e768a7b658dd8e918f46
Reviewed-by: Tim Jenssen <tim.jenssen@qt.io>
Reviewed-by: Marco Bubke <marco.bubke@qt.io>

src/libs/sqlite/sqlitedatabase.h

@@ -31,6 +31,7 @@
 
 #include <utils/smallstring.h>
 
+#include <mutex>
 #include <vector>
 
 namespace Sqlite {
@@ -38,11 +39,13 @@ namespace Sqlite {
 class SQLITE_EXPORT Database
 {
     template <typename Database>
-    friend class SqliteAbstractTransaction;
+    friend class AbstractTransaction;
-    friend class SqliteStatement;
+    friend class Statement;
-    friend class SqliteBackend;
+    friend class Backend;
 
 public:
+    using MutexType = std::mutex;
+
     Database();
     Database(Utils::PathString &&databaseFilePath);
 
@@ -79,12 +82,13 @@ public:
 
 private:
     void initializeTables();
-
+    std::mutex &databaseMutex() { return m_databaseMutex; }
 
 private:
+    Utils::PathString m_databaseFilePath;
     DatabaseBackend m_databaseBackend;
     std::vector<Table> m_sqliteTables;
-    Utils::PathString m_databaseFilePath;
+    std::mutex m_databaseMutex;
     JournalMode m_journalMode = JournalMode::Wal;
     OpenMode m_openMode = OpenMode::ReadWrite;
     bool m_isOpen = false;

src/libs/sqlite/sqlitetransaction.h

@@ -27,6 +27,8 @@
 
 #include "sqliteglobal.h"
 
+#include <mutex>
+
 namespace Sqlite {
 
 class DatabaseBackend;
@@ -50,11 +52,13 @@ public:
 
 protected:
     AbstractTransaction(Database &database)
-        : m_database(database)
+        : m_databaseLock(database.databaseMutex()),
+          m_database(database)
     {
     }
 
 private:
+    std::lock_guard<typename Database::MutexType> m_databaseLock;
     Database &m_database;
     bool m_isAlreadyCommited = false;
 };

tests/unit/unittest/mockmutex.h

@@ -0,0 +1,37 @@
+/****************************************************************************
+**
+** Copyright (C) 2017 The Qt Company Ltd.
+** Contact: https://www.qt.io/licensing/
+**
+** This file is part of Qt Creator.
+**
+** Commercial License Usage
+** Licensees holding valid commercial Qt licenses may use this file in
+** accordance with the commercial license agreement provided with the
+** Software or, alternatively, in accordance with the terms contained in
+** a written agreement between you and The Qt Company. For licensing terms
+** and conditions see https://www.qt.io/terms-conditions. For further
+** information use the contact form at https://www.qt.io/contact-us.
+**
+** GNU General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU
+** General Public License version 3 as published by the Free Software
+** Foundation with exceptions as appearing in the file LICENSE.GPL3-EXCEPT
+** included in the packaging of this file. Please review the following
+** information to ensure the GNU General Public License requirements will
+** be met: https://www.gnu.org/licenses/gpl-3.0.html.
+**
+****************************************************************************/
+
+#pragma once
+
+#include "googletest.h"
+
+class MockMutex
+{
+public:
+    using MutexType = MockMutex;
+
+    MOCK_METHOD0(lock, void ());
+    MOCK_METHOD0(unlock, void ());
+};

tests/unit/unittest/mocksqlitedatabase.h

@@ -27,6 +27,8 @@
 
 #include "googletest.h"
 
+#include "mockmutex.h"
+
 #include <sqlitetable.h>
 
 #include <utils/smallstringview.h>
@@ -34,8 +36,19 @@
 class MockSqliteDatabase
 {
 public:
+    using MutexType = MockMutex;
+
+    MockSqliteDatabase() = default;
+    MockSqliteDatabase(const MockMutex &mockMutex)
+    {
+        ON_CALL(*this, databaseMutex())
+            .WillByDefault(ReturnRef(const_cast<MockMutex &>(mockMutex)));
+    }
+
     MOCK_METHOD1(execute,
                  void (Utils::SmallStringView sqlStatement));
 
+    MOCK_METHOD0(databaseMutex,
+                 MockMutex &());
 };
 

tests/unit/unittest/sqlitetransaction-test.cpp

@@ -0,0 +1,115 @@
+/****************************************************************************
+**
+** Copyright (C) 2017 The Qt Company Ltd.
+** Contact: https://www.qt.io/licensing/
+**
+** This file is part of Qt Creator.
+**
+** Commercial License Usage
+** Licensees holding valid commercial Qt licenses may use this file in
+** accordance with the commercial license agreement provided with the
+** Software or, alternatively, in accordance with the terms contained in
+** a written agreement between you and The Qt Company. For licensing terms
+** and conditions see https://www.qt.io/terms-conditions. For further
+** information use the contact form at https://www.qt.io/contact-us.
+**
+** GNU General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU
+** General Public License version 3 as published by the Free Software
+** Foundation with exceptions as appearing in the file LICENSE.GPL3-EXCEPT
+** included in the packaging of this file. Please review the following
+** information to ensure the GNU General Public License requirements will
+** be met: https://www.gnu.org/licenses/gpl-3.0.html.
+**
+****************************************************************************/
+
+#include "googletest.h"
+
+#include <sqlitetransaction.h>
+#include <mocksqlitedatabase.h>
+
+namespace {
+
+using DeferredTransaction = Sqlite::DeferredTransaction<MockSqliteDatabase>;
+using ImmediateTransaction = Sqlite::ImmediateTransaction<MockSqliteDatabase>;
+using ExclusiveTransaction = Sqlite::ExclusiveTransaction<MockSqliteDatabase>;
+
+class SqliteTransaction : public testing::Test
+{
+protected:
+    MockMutex mockMutex;
+    MockSqliteDatabase mockDatabase{mockMutex};
+};
+
+TEST_F(SqliteTransaction, DeferredTransactionCommit)
+{
+    EXPECT_CALL(mockDatabase, databaseMutex());
+    EXPECT_CALL(mockMutex, lock());
+    EXPECT_CALL(mockDatabase, execute(Eq("BEGIN")));
+    EXPECT_CALL(mockDatabase, execute(Eq("COMMIT")));
+    EXPECT_CALL(mockMutex, unlock());
+
+    DeferredTransaction transaction{mockDatabase};
+    transaction.commit();
+}
+
+TEST_F(SqliteTransaction, DeferredTransactionRollBack)
+{
+    EXPECT_CALL(mockDatabase, databaseMutex());
+    EXPECT_CALL(mockMutex, lock());
+    EXPECT_CALL(mockDatabase, execute(Eq("BEGIN")));
+    EXPECT_CALL(mockDatabase, execute(Eq("ROLLBACK")));
+    EXPECT_CALL(mockMutex, unlock());
+
+    DeferredTransaction transaction{mockDatabase};
+}
+
+TEST_F(SqliteTransaction, ImmediateTransactionCommit)
+{
+    EXPECT_CALL(mockDatabase, databaseMutex());
+    EXPECT_CALL(mockMutex, lock());
+    EXPECT_CALL(mockDatabase, execute(Eq("BEGIN IMMEDIATE")));
+    EXPECT_CALL(mockDatabase, execute(Eq("COMMIT")));
+    EXPECT_CALL(mockMutex, unlock());
+
+    ImmediateTransaction transaction{mockDatabase};
+    transaction.commit();
+}
+
+TEST_F(SqliteTransaction, ImmediateTransactionRollBack)
+{
+    EXPECT_CALL(mockDatabase, databaseMutex());
+    EXPECT_CALL(mockMutex, lock());
+    EXPECT_CALL(mockDatabase, execute(Eq("BEGIN IMMEDIATE")));
+    EXPECT_CALL(mockDatabase, execute(Eq("ROLLBACK")));
+    EXPECT_CALL(mockMutex, unlock());
+
+    ImmediateTransaction transaction{mockDatabase};
+}
+
+TEST_F(SqliteTransaction, ExclusiveTransactionCommit)
+{
+    EXPECT_CALL(mockDatabase, databaseMutex());
+    EXPECT_CALL(mockMutex, lock());
+    EXPECT_CALL(mockDatabase, execute(Eq("BEGIN EXCLUSIVE")));
+    EXPECT_CALL(mockDatabase, execute(Eq("COMMIT")));
+    EXPECT_CALL(mockMutex, unlock());
+
+    ExclusiveTransaction transaction{mockDatabase};
+    transaction.commit();
+}
+
+TEST_F(SqliteTransaction, ExclusiveTransactionRollBack)
+{
+    EXPECT_CALL(mockDatabase, databaseMutex());
+    EXPECT_CALL(mockMutex, lock());
+    EXPECT_CALL(mockDatabase, execute(Eq("BEGIN EXCLUSIVE")));
+    EXPECT_CALL(mockDatabase, execute(Eq("ROLLBACK")));
+    EXPECT_CALL(mockMutex, unlock());
+
+    ExclusiveTransaction transaction{mockDatabase};
+}
+
+}
+
+

tests/unit/unittest/storagesqlitestatementfactory-test.cpp

@@ -42,7 +42,8 @@ using Sqlite::Table;
 class StorageSqliteStatementFactory : public testing::Test
 {
 protected:
-    NiceMock<MockSqliteDatabase> mockDatabase;
+    NiceMock<MockMutex> mockMutex;
+    NiceMock<MockSqliteDatabase> mockDatabase{mockMutex};
     StatementFactory factory{mockDatabase};
 };
 
@@ -50,10 +51,12 @@ TEST_F(StorageSqliteStatementFactory, AddSymbolsTable)
 {
     InSequence s;
 
+    EXPECT_CALL(mockMutex, lock());
     EXPECT_CALL(mockDatabase, execute(Eq("BEGIN IMMEDIATE")));
     EXPECT_CALL(mockDatabase, execute(Eq("CREATE TABLE IF NOT EXISTS symbols(symbolId INTEGER PRIMARY KEY, usr TEXT, symbolName TEXT)")));
     EXPECT_CALL(mockDatabase, execute(Eq("CREATE INDEX IF NOT EXISTS index_symbols_usr ON symbols(usr)")));
     EXPECT_CALL(mockDatabase, execute(Eq("COMMIT")));
+    EXPECT_CALL(mockMutex, unlock());
 
     factory.createSymbolsTable();
 }
@@ -62,10 +65,12 @@ TEST_F(StorageSqliteStatementFactory, AddLocationsTable)
 {
     InSequence s;
 
+    EXPECT_CALL(mockMutex, lock());
     EXPECT_CALL(mockDatabase, execute(Eq("BEGIN IMMEDIATE")));
     EXPECT_CALL(mockDatabase, execute(Eq("CREATE TABLE IF NOT EXISTS locations(symbolId INTEGER, line INTEGER, column INTEGER, sourceId INTEGER)")));
     EXPECT_CALL(mockDatabase, execute(Eq("CREATE INDEX IF NOT EXISTS index_locations_sourceId ON locations(sourceId)")));
     EXPECT_CALL(mockDatabase, execute(Eq("COMMIT")));
+    EXPECT_CALL(mockMutex, unlock());
 
     factory.createLocationsTable();
 }
@@ -74,9 +79,11 @@ TEST_F(StorageSqliteStatementFactory, AddSourcesTable)
 {
     InSequence s;
 
+    EXPECT_CALL(mockMutex, lock());
     EXPECT_CALL(mockDatabase, execute(Eq("BEGIN IMMEDIATE")));
     EXPECT_CALL(mockDatabase, execute(Eq("CREATE TABLE IF NOT EXISTS sources(sourceId INTEGER PRIMARY KEY, sourcePath TEXT)")));
     EXPECT_CALL(mockDatabase, execute(Eq("COMMIT")));
+    EXPECT_CALL(mockMutex, unlock());
 
     factory.createSourcesTable();
 }
@@ -85,11 +92,13 @@ TEST_F(StorageSqliteStatementFactory, AddNewSymbolsTable)
 {
     InSequence s;
 
+    EXPECT_CALL(mockMutex, lock());
     EXPECT_CALL(mockDatabase, execute(Eq("BEGIN IMMEDIATE")));
     EXPECT_CALL(mockDatabase, execute(Eq("CREATE TEMPORARY TABLE newSymbols(temporarySymbolId INTEGER PRIMARY KEY, symbolId INTEGER, usr TEXT, symbolName TEXT)")));
     EXPECT_CALL(mockDatabase, execute(Eq("CREATE INDEX IF NOT EXISTS index_newSymbols_usr_symbolName ON newSymbols(usr, symbolName)")));
     EXPECT_CALL(mockDatabase, execute(Eq("CREATE INDEX IF NOT EXISTS index_newSymbols_symbolId ON newSymbols(symbolId)")));
     EXPECT_CALL(mockDatabase, execute(Eq("COMMIT")));
+    EXPECT_CALL(mockMutex, unlock());
 
     factory.createNewSymbolsTable();
 }
@@ -99,10 +108,12 @@ TEST_F(StorageSqliteStatementFactory, AddNewLocationsTable)
 {
     InSequence s;
 
+    EXPECT_CALL(mockMutex, lock());
     EXPECT_CALL(mockDatabase, execute(Eq("BEGIN IMMEDIATE")));
     EXPECT_CALL(mockDatabase, execute(Eq("CREATE TEMPORARY TABLE newLocations(temporarySymbolId INTEGER, symbolId INTEGER, line INTEGER, column INTEGER, sourceId INTEGER)")));
     EXPECT_CALL(mockDatabase, execute(Eq("CREATE INDEX IF NOT EXISTS index_newLocations_sourceId ON newLocations(sourceId)")));
     EXPECT_CALL(mockDatabase, execute(Eq("COMMIT")));
+    EXPECT_CALL(mockMutex, unlock());
 
     factory.createNewLocationsTable();
 }
@@ -111,6 +122,8 @@ TEST_F(StorageSqliteStatementFactory, AddTablesInConstructor)
 {
     EXPECT_CALL(mockDatabase, execute(Eq("BEGIN IMMEDIATE"))).Times(5);
     EXPECT_CALL(mockDatabase, execute(Eq("COMMIT"))).Times(5);
+    EXPECT_CALL(mockMutex, lock()).Times(5);
+    EXPECT_CALL(mockMutex, unlock()).Times(5);
 
     EXPECT_CALL(mockDatabase, execute(Eq("CREATE TABLE IF NOT EXISTS symbols(symbolId INTEGER PRIMARY KEY, usr TEXT, symbolName TEXT)")));
     EXPECT_CALL(mockDatabase, execute(Eq("CREATE INDEX IF NOT EXISTS index_symbols_usr ON symbols(usr)")));

tests/unit/unittest/symbolstorage-test.cpp

@@ -59,9 +59,9 @@ protected:
 
 protected:
     FilePathCache<std::mutex> filePathCache;
-    NiceMock<MockSqliteDatabase> mockDatabase;
+    NiceMock<MockMutex> mockMutex;
+    NiceMock<MockSqliteDatabase> mockDatabase{mockMutex};
     StatementFactory statementFactory{mockDatabase};
-
     MockSqliteWriteStatement &insertSymbolsToNewSymbolsStatement = statementFactory.insertSymbolsToNewSymbolsStatement;
     MockSqliteWriteStatement &insertLocationsToNewLocationsStatement = statementFactory.insertLocationsToNewLocationsStatement;
     MockSqliteWriteStatement &insertSourcesStatement = statementFactory.insertSourcesStatement;
@@ -172,6 +172,7 @@ TEST_F(SymbolStorage, AddSymbolsAndSourceLocationsCallsWrite)
 {
     InSequence sequence;
 
+    EXPECT_CALL(mockMutex, lock());
     EXPECT_CALL(mockDatabase, execute(Eq("BEGIN IMMEDIATE")));
     EXPECT_CALL(insertSymbolsToNewSymbolsStatement, write(_, _, _)).Times(2);
     EXPECT_CALL(insertLocationsToNewLocationsStatement, write(1, 42, 23, 3));
@@ -188,6 +189,7 @@ TEST_F(SymbolStorage, AddSymbolsAndSourceLocationsCallsWrite)
     EXPECT_CALL(deleteNewSymbolsTableStatement, execute());
     EXPECT_CALL(deleteNewLocationsTableStatement, execute());
     EXPECT_CALL(mockDatabase, execute(Eq("COMMIT")));
+    EXPECT_CALL(mockMutex, unlock());
 
     storage.addSymbolsAndSourceLocations(symbolEntries, sourceLocations);
 }

tests/unit/unittest/unittest.pro

@@ -74,7 +74,8 @@ SOURCES += \
     symbolquery-test.cpp \
     storagesqlitestatementfactory-test.cpp \
     querysqlitestatementfactory-test.cpp \
-    sqliteindex-test.cpp
+    sqliteindex-test.cpp \
+    sqlitetransaction-test.cpp
 
 !isEmpty(LIBCLANG_LIBS) {
 SOURCES += \
@@ -201,7 +202,8 @@ HEADERS += \
     mocksqlitereadstatement.h \
     google-using-declarations.h \
     mocksymbolindexing.h \
-    sqliteteststatement.h
+    sqliteteststatement.h \
+    mockmutex.h
 
 !isEmpty(LIBCLANG_LIBS) {
 HEADERS += \