Merge lp:~stolowski/unity-scope-click/fix-transaction-error into lp:unity-scope-click/devel

[Paweł Stołowski]

Use write-ahead-log pragma of sqlite to fix transaction errors. Throw if transaction fails. Explicitly finish every query.

Use write-ahead-log pragma of sqlite to fix transaction errors. Throw if transaction fails. Explicitly finish every query.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:339
http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-ci/207/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-amd64-ci/182
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-armhf-ci/181
        deb: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-armhf-ci/181/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-i386-ci/181

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-team-unity-scope-click-devel-ci/207/rebuild

review: Approve (continuous-integration)

[dobey (dobey)]

Are there tests that go along with this code? Should they have tests to ensure that finish() is called in all the required cases, to avoid regression?

review: Needs Information

[Alejandro J. Cura (alecu)]

Is there a way to add some kind of testing to this branch? Ideally, it would be a test that makes a transaction fail, and that gets fixed with this branch.

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:340
http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-ci/211/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-amd64-ci/186
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-armhf-ci/185
        deb: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-armhf-ci/185/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-i386-ci/185/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-team-unity-scope-click-devel-ci/211/rebuild

review: Needs Fixing (continuous-integration)

[Paweł Stołowski (stolowski)]

> Is there a way to add some kind of testing to this branch? Ideally, it would
> be a test that makes a transaction fail, and that gets fixed with this branch.

Ok, I've added two tests, one of them is a "stress test" that makes either reading or writing fail without WAL.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:343
http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-ci/212/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-amd64-ci/187
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-armhf-ci/186
        deb: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-armhf-ci/186/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-i386-ci/186

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-team-unity-scope-click-devel-ci/212/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:345
http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-ci/213/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-amd64-ci/188
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-armhf-ci/187
        deb: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-armhf-ci/187/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-i386-ci/187

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-team-unity-scope-click-devel-ci/213/rebuild

review: Approve (continuous-integration)

[Alejandro J. Cura (alecu)]

Branch looks good, and the WAL fix seems reasonable.
The stress test is not so slow in my desktop machine (~5 seconds), but it might be due to the SSD.
In any case, I think we should consider moving that test into the integration tests if it gets slow for other developers.

review: Approve

[dobey (dobey)]

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-autolanding/93/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-amd64-autolanding/78
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-armhf-autolanding/78/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-i386-autolanding/78

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'libclickscope/click/departments-db.cpp'
--- libclickscope/click/departments-db.cpp	2014-07-10 08:57:59 +0000
+++ libclickscope/click/departments-db.cpp	2014-07-17 11:09:40 +0000
@@ -99,6 +99,11 @@
     // them for now.
     // query.exec("PRAGMA foreign_keys = ON");
 
+    // activate write-ahead logging, see http://sqlite.org/wal.html to avoid transaction errors with concurrent reads and writes
+    query.exec("PRAGMA journal_mode=WAL");
+
+    db_.transaction();
+
     // package id -> department id mapping table
     if (!query.exec("CREATE TABLE IF NOT EXISTS pkgmap (pkgid TEXT, deptid TEXT, CONSTRAINT pkey PRIMARY KEY (pkgid, deptid))"))
     {
@@ -130,6 +135,11 @@
     {
         report_db_error(query.lastError(), "Failed to create depts_v view");
     }
+
+    if (!db_.commit())
+    {
+        report_db_error(db_.lastError(), "Failed to commit init transaction");
+    }
 }
 
 void DepartmentsDb::report_db_error(const QSqlError& error, const std::string& message)
@@ -151,9 +161,12 @@
 
         if (select_dept_name_->next())
         {
-            return select_dept_name_->value(0).toString().toStdString();
+            auto const res = select_dept_name_->value(0).toString().toStdString();
+            select_dept_name_->finish();
+            return res;
         }
     }
+    select_dept_name_->finish();
     throw std::logic_error("No name for department " + department_id);
 }
 
@@ -166,9 +179,12 @@
     }
     if (!select_parent_dept_->next())
     {
+        select_dept_name_->finish();
         throw std::logic_error("Unknown department '" + department_id + "'");
     }
-    return select_parent_dept_->value(0).toString().toStdString();
+    auto const res = select_parent_dept_->value(0).toString().toStdString();
+    select_parent_dept_->finish();
+    return res;
 }
 
 std::list<DepartmentsDb::DepartmentInfo> DepartmentsDb::get_children_departments(const std::string& department_id)
@@ -187,6 +203,7 @@
         depts.push_back(inf);
     }
 
+    select_children_depts_->finish();
     return depts;
 }
 
@@ -203,6 +220,7 @@
     {
         pkgs.insert(query->value(0).toString().toStdString());
     }
+    query->finish();
     return pkgs;
 }
 
@@ -226,6 +244,7 @@
     // delete package mapping first from any departments
     delete_pkgmap_query_->bindValue(":pkgid", QVariant(QString::fromStdString(package_id)));
     delete_pkgmap_query_->exec();
+    delete_pkgmap_query_->finish();
 
     insert_pkgmap_query_->bindValue(":pkgid", QVariant(QString::fromStdString(package_id)));
     insert_pkgmap_query_->bindValue(":deptid", QVariant(QString::fromStdString(department_id)));
@@ -238,9 +257,12 @@
         report_db_error(insert_pkgmap_query_->lastError(), "Failed to insert into pkgmap");
     }
 
+    insert_pkgmap_query_->finish();
+
     if (!db_.commit())
     {
-        std::cerr << "Failed to commit transaction" << std::endl;
+        db_.rollback();
+        report_db_error(db_.lastError(), "Failed to commit transaction in store_package_mapping");
     }
 }
 
@@ -262,6 +284,7 @@
     {
         report_db_error(insert_dept_id_query_->lastError(), "Failed to insert into depts");
     }
+    insert_dept_id_query_->finish();
 }
 
 void DepartmentsDb::store_department_name(const std::string& department_id, const std::string& locale, const std::string& name)
@@ -284,6 +307,7 @@
     {
         report_db_error(insert_dept_name_query_->lastError(), "Failed to insert into deptnames");
     }
+    insert_dept_name_query_->finish();
 }
 
 int DepartmentsDb::department_mapping_count() const
@@ -350,11 +374,15 @@
         report_db_error(delete_depts_query_->lastError(), "Failed to delete from depts");
     }
 
+    delete_deptnames_query_->finish();
+    delete_depts_query_->finish();
+
     store_departments_(depts, locale);
 
     if (!db_.commit())
     {
-        std::cerr << "Failed to commit transaction" << std::endl;
+        db_.rollback();
+        report_db_error(db_.lastError(), "Failed to commit transaction in store_departments");
     }
 }
 
 
=== modified file 'libclickscope/click/departments-db.h'
--- libclickscope/click/departments-db.h	2014-07-10 08:57:59 +0000
+++ libclickscope/click/departments-db.h	2014-07-17 11:09:40 +0000
@@ -82,7 +82,7 @@
 
     static std::unique_ptr<DepartmentsDb> create_db();
 
-private:
+protected:
     void init_db(const std::string& name);
     void store_departments_(const click::DepartmentList& depts, const std::string& locale);
     static void report_db_error(const QSqlError& error, const std::string& message);
 
=== modified file 'libclickscope/tests/test_departments-db.cpp'
--- libclickscope/tests/test_departments-db.cpp	2014-07-02 13:14:36 +0000
+++ libclickscope/tests/test_departments-db.cpp	2014-07-17 11:09:40 +0000
@@ -36,6 +36,30 @@
 
 using namespace click;
 
+class DepartmentsDbCheck : public DepartmentsDb
+{
+public:
+    DepartmentsDbCheck(const std::string& name)
+        : DepartmentsDb(name)
+    {
+    }
+
+    void check_sql_queries_finished()
+    {
+        EXPECT_FALSE(delete_pkgmap_query_->isActive());
+        EXPECT_FALSE(delete_depts_query_->isActive());
+        EXPECT_FALSE(delete_deptnames_query_->isActive());
+        EXPECT_FALSE(insert_pkgmap_query_->isActive());
+        EXPECT_FALSE(insert_dept_id_query_->isActive());
+        EXPECT_FALSE(insert_dept_name_query_->isActive());
+        EXPECT_FALSE(select_pkgs_by_dept_->isActive());
+        EXPECT_FALSE(select_pkgs_by_dept_recursive_->isActive());
+        EXPECT_FALSE(select_parent_dept_->isActive());
+        EXPECT_FALSE(select_children_depts_->isActive());
+        EXPECT_FALSE(select_dept_name_->isActive());
+    }
+};
+
 class DepartmentsDbTest: public ::testing::Test
 {
 public:
@@ -43,7 +67,7 @@
 
     void SetUp() override
     {
-        db.reset(new DepartmentsDb(db_path));
+        db.reset(new DepartmentsDbCheck(db_path));
         db->store_department_name("tools", "", "Tools");
         db->store_department_name("office", "", "Office");
 
@@ -72,7 +96,18 @@
     }
 
 protected:
-    std::unique_ptr<DepartmentsDb> db;
+    std::unique_ptr<DepartmentsDbCheck> db;
+};
+
+class DepartmentsDbConcurrencyTest: public ::testing::Test
+{
+public:
+    const std::string db_path = TEST_DIR "/departments-db-test2.sqlite";
+
+    void TearDown() override
+    {
+        unlink(db_path.c_str());
+    }
 };
 
 TEST_F(DepartmentsDbTest, testDepartmentNameLookup)
@@ -224,3 +259,126 @@
     EXPECT_EQ(4u, db->package_count());
 }
 
+TEST_F(DepartmentsDbTest, testSqlQueriesFinished)
+{
+    db->get_department_name("games", {"en_EN", ""});
+    db->check_sql_queries_finished();
+
+    db->get_parent_department_id("rpg");
+    db->check_sql_queries_finished();
+
+    db->get_packages_for_department("rpg", false);
+    db->check_sql_queries_finished();
+
+    db->get_packages_for_department("rpg", true);
+    db->check_sql_queries_finished();
+
+    db->get_children_departments("games");
+    db->check_sql_queries_finished();
+
+    db->department_name_count();
+    db->check_sql_queries_finished();
+
+    db->department_mapping_count();
+    db->check_sql_queries_finished();
+
+    db->package_count();
+    db->check_sql_queries_finished();
+
+    db->store_package_mapping("game2", "fps");
+    db->check_sql_queries_finished();
+
+    db->store_department_name("games", "pl_PL", "Gry");
+    db->check_sql_queries_finished();
+
+    db->store_department_mapping("office", "tools");
+    db->check_sql_queries_finished();
+}
+
+// Keep the numbers below at a reasonable level; it takes around 20 seconds
+// to run this test on a i7-2620M with the default values.
+const int NUM_OF_WRITE_OPS = 50;
+const int NUM_OF_READ_OPS = 100;
+
+void populate_departments(DepartmentsDb *db, int repeat_count)
+{
+    click::DepartmentList depts;
+    // generate departments with one subdepartment
+    for (int i = 0; i < 20; i++)
+    {
+        auto const id = std::to_string(i);
+        auto parent = std::make_shared<click::Department>(id, "Department " + id, "href", true);
+        parent->set_subdepartments({std::make_shared<click::Department>(id + "sub", "Subdepartment of " + id, "href", false)});
+        depts.push_back(parent);
+    }
+
+    for (int i = 0; i < repeat_count; i++)
+    {
+        ASSERT_NO_THROW(db->store_departments(depts, ""));
+
+        // generate apps
+        for (int j = 0; j < 50; j++)
+        {
+            auto const id = std::to_string(j);
+            ASSERT_NO_THROW(db->store_package_mapping("app" + id, id));
+        }
+    }
+}
+
+TEST_F(DepartmentsDbConcurrencyTest, ConcurrentReadWrite)
+{
+    // populate the db initially to make sure reader doesn't fail if it's faster than writer
+    {
+        DepartmentsDb db(db_path);
+        populate_departments(&db, 1);
+    }
+
+    pid_t writer_pid = fork();
+    if (writer_pid < 0)
+    {
+        FAIL();
+    }
+    else if (writer_pid == 0) // writer child process
+    {
+        SCOPED_TRACE("writer");
+        std::unique_ptr<DepartmentsDb> db;
+        ASSERT_NO_THROW(db.reset(new DepartmentsDb(db_path)));
+
+        populate_departments(db.get(), NUM_OF_WRITE_OPS);
+
+        exit(0);
+    }
+    else // parent process
+    {
+        pid_t reader_pid = fork();
+        if (reader_pid < 0)
+        {
+            FAIL();
+        }
+        else if (reader_pid == 0) // reader child process
+        {
+            SCOPED_TRACE("reader");
+            std::unique_ptr<DepartmentsDb> db;
+            ASSERT_NO_THROW(db.reset(new DepartmentsDb(db_path)));
+
+            for (int i = 0; i < NUM_OF_READ_OPS; i++)
+            {
+                ASSERT_NO_THROW(db->get_department_name("1", {""}));
+                ASSERT_NO_THROW(db->get_parent_department_id("1"));
+                ASSERT_NO_THROW(db->get_packages_for_department("1", false));
+                ASSERT_NO_THROW(db->get_packages_for_department("1", true));
+                ASSERT_NO_THROW(db->get_children_departments(""));
+                ASSERT_NO_THROW(db->department_name_count());
+                ASSERT_NO_THROW(db->department_mapping_count());
+                ASSERT_NO_THROW(db->package_count());
+            }
+            exit(0);
+        }
+        else // parent process
+        {
+            wait(nullptr);
+            wait(nullptr);
+        }
+    }
+}
+