From 2c01d032db151413ee87ca58bd9dae457bc73f0f Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Mon, 21 Jan 2019 15:26:45 +1030 Subject: [PATCH 1/7] Bump deb build to test stage in gitlab ci config This puts it at the stame stage as the flatpak package and skips one build that will always fail if the Ubuntu one fails for legit reasons. --- .gitlab-ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 528972c4..1b3928a8 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -59,7 +59,7 @@ ubuntu: - $INSTALL_CMD deb-package: - stage: build + stage: test image: ubuntu:rolling before_script: - apt-get update From 92bca47d435e64031dc0424a162be9569e22c88e Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Tue, 22 Jan 2019 00:45:30 +1100 Subject: [PATCH 2/7] Close thread pool if present in Geary.Database dtor Fixes running unit tests on a single CPU host. --- src/engine/db/db-database.vala | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/engine/db/db-database.vala b/src/engine/db/db-database.vala index 90f8f5c2..a13a8777 100644 --- a/src/engine/db/db-database.vala +++ b/src/engine/db/db-database.vala @@ -84,9 +84,9 @@ public class Geary.Db.Database : Geary.Db.Context { } ~Database() { - // Not thrilled about using lock in a dtor - lock (outstanding_async_jobs) { - assert(outstanding_async_jobs == 0); + // Not thrilled about long-running tasks in a dtor + if (this.thread_pool != null) { + GLib.ThreadPool.free((owned) this.thread_pool, true, true); } } From 5f73eb94b25c919491d77b9c505edd7653a5fa97 Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Tue, 22 Jan 2019 00:54:18 +1100 Subject: [PATCH 3/7] Use standard recursive delete in ImapDB.DatabaseTest --- test/engine/imap-db/imap-db-database-test.vala | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/test/engine/imap-db/imap-db-database-test.vala b/test/engine/imap-db/imap-db-database-test.vala index 892d2815..f7c55728 100644 --- a/test/engine/imap-db/imap-db-database-test.vala +++ b/test/engine/imap-db/imap-db-database-test.vala @@ -42,8 +42,7 @@ class Geary.ImapDB.DatabaseTest : TestCase { // Need to close it again to stop the GC process running db.close(); - db.file.delete(); - tmp_dir.delete(); + delete_file(tmp_dir); } public void upgrade_0_6() throws Error { @@ -121,11 +120,7 @@ class Geary.ImapDB.DatabaseTest : TestCase { // Need to close it again to stop the GC process running db.close(); - Files.recursive_delete_async.begin( - tmp_dir, GLib.Priority.DEFAULT, null, - (obj, res) => { async_complete(res); } - ); - Files.recursive_delete_async.end(async_result()); + delete_file(tmp_dir); } From 8a057a20860efec762fff8a2adbd9dab3dbddda9 Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Tue, 22 Jan 2019 00:55:38 +1100 Subject: [PATCH 4/7] Fix name of temp dir used in ImapDB.AccountTest --- test/engine/imap-db/imap-db-account-test.vala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/engine/imap-db/imap-db-account-test.vala b/test/engine/imap-db/imap-db-account-test.vala index d70e44e5..768416bc 100644 --- a/test/engine/imap-db/imap-db-account-test.vala +++ b/test/engine/imap-db/imap-db-account-test.vala @@ -30,7 +30,7 @@ class Geary.ImapDB.AccountTest : TestCase { public override void set_up() throws GLib.Error { this.tmp_dir = GLib.File.new_for_path( - GLib.DirUtils.make_tmp("geary-db-database-test-XXXXXX") + GLib.DirUtils.make_tmp("geary-imap-db-account-test-XXXXXX") ); this.config = new Geary.AccountInformation( From 719bbca9018c38315ec80c0743eb60a21a017f81 Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Tue, 22 Jan 2019 01:45:02 +1100 Subject: [PATCH 5/7] Ensure ImapDB.DatabaseTest and AccountTest dispose of their db instances Make sure the database these instances are/have are destroyed before deleting the database file, so it is definitely not being kept open. --- test/engine/imap-db/imap-db-account-test.vala | 1 + .../engine/imap-db/imap-db-database-test.vala | 32 ++++++++++--------- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/test/engine/imap-db/imap-db-account-test.vala b/test/engine/imap-db/imap-db-account-test.vala index 768416bc..c6aac242 100644 --- a/test/engine/imap-db/imap-db-account-test.vala +++ b/test/engine/imap-db/imap-db-account-test.vala @@ -59,6 +59,7 @@ class Geary.ImapDB.AccountTest : TestCase { (obj, ret) => { async_complete(ret); } ); this.account.close_async.end(async_result()); + this.account = null; delete_file(this.tmp_dir); this.tmp_dir = null; diff --git a/test/engine/imap-db/imap-db-database-test.vala b/test/engine/imap-db/imap-db-database-test.vala index f7c55728..1f823d6b 100644 --- a/test/engine/imap-db/imap-db-database-test.vala +++ b/test/engine/imap-db/imap-db-database-test.vala @@ -9,21 +9,31 @@ class Geary.ImapDB.DatabaseTest : TestCase { + private GLib.File? tmp_dir = null; + + public DatabaseTest() { base("Geary.ImapDb.DatabaseTest"); add_test("open_new", open_new); add_test("upgrade_0_6", upgrade_0_6); } - public void open_new() throws Error { - GLib.File tmp_dir = GLib.File.new_for_path( - GLib.DirUtils.make_tmp("geary-db-database-test-XXXXXX") + public override void set_up() throws GLib.Error { + this.tmp_dir = GLib.File.new_for_path( + GLib.DirUtils.make_tmp("geary-imap-db-database-test-XXXXXX") ); + } + public override void tear_down() throws GLib.Error { + delete_file(this.tmp_dir); + this.tmp_dir = null; + } + + public void open_new() throws Error { Database db = new Database( - tmp_dir.get_child("test.db"), + this.tmp_dir.get_child("test.db"), GLib.File.new_for_path(_SOURCE_ROOT_DIR).get_child("sql"), - tmp_dir.get_child("attachments"), + this.tmp_dir.get_child("attachments"), new Geary.SimpleProgressMonitor(Geary.ProgressType.DB_UPGRADE), new Geary.SimpleProgressMonitor(Geary.ProgressType.DB_VACUUM), "test@example.com" @@ -41,15 +51,9 @@ class Geary.ImapDB.DatabaseTest : TestCase { // Need to close it again to stop the GC process running db.close(); - - delete_file(tmp_dir); } public void upgrade_0_6() throws Error { - GLib.File tmp_dir = GLib.File.new_for_path( - GLib.DirUtils.make_tmp("geary-db-database-test-XXXXXX") - ); - // Since the upgrade process also messes around with // attachments on disk which we want to be able to test, we // need to have a complete-ish database and attachments @@ -64,11 +68,11 @@ class Geary.ImapDB.DatabaseTest : TestCase { GLib.File db_archive = GLib.File .new_for_uri(RESOURCE_URI) .resolve_relative_path(DB_0_6_RESOURCE); - GLib.File db_dir = tmp_dir.get_child(DB_0_6_DIR); + GLib.File db_dir = this.tmp_dir.get_child(DB_0_6_DIR); GLib.File db_file = db_dir.get_child("geary.db"); GLib.File attachments_dir = db_dir.get_child("attachments"); - unpack_archive(db_archive, tmp_dir); + unpack_archive(db_archive, this.tmp_dir); // This number is the id of the last known message in the // database @@ -119,8 +123,6 @@ class Geary.ImapDB.DatabaseTest : TestCase { // Need to close it again to stop the GC process running db.close(); - - delete_file(tmp_dir); } From d4700afeb80f793631b690c6b778f7720b744727 Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Tue, 22 Jan 2019 10:34:39 +1100 Subject: [PATCH 6/7] Don't reccommend db garbage collection if there are no messages This prevents the GC running during (most) unit tests, which might also be causing intermittent hangs in ImapDB.AccountTest and DatabaseTest. --- src/engine/imap-db/imap-db-gc.vala | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/src/engine/imap-db/imap-db-gc.vala b/src/engine/imap-db/imap-db-gc.vala index ce52e990..5f21bcbe 100644 --- a/src/engine/imap-db/imap-db-gc.vala +++ b/src/engine/imap-db/imap-db-gc.vala @@ -107,11 +107,15 @@ private class Geary.ImapDB.GC { (last_vacuum_time != null) ? last_vacuum_time.to_string() : "never", reaped_messages_since_last_vacuum, free_page_bytes.to_string()); - + RecommendedOperation op = RecommendedOperation.NONE; - DateTime now = new DateTime.now_local(); - + if (!yield has_message_rows(cancellable)) { + // No message rows exist, so don't bother vacuuming + return op; + } + // Reap every REAP_DAYS_SPAN unless never executed, in which case run now + DateTime now = new DateTime.now_local(); int64 days; if (last_reap_time == null) { // null means reaping has never executed @@ -613,7 +617,24 @@ private class Geary.ImapDB.GC { return deleted; } - + + private async bool has_message_rows(GLib.Cancellable? cancellable) { + bool ret = false; + yield db.exec_transaction_async(Db.TransactionType.RO, (cx) => { + Db.Result result = cx.query( + "SELECT count(*) FROM MessageTable LIMIT 1" + ); + + Db.TransactionOutcome txn_ret = FAILURE; + if (!result.finished) { + txn_ret = SUCCESS; + ret = result.int64_at(0) > 0; + } + return txn_ret; + }, cancellable); + return ret; + } + private async void fetch_gc_info_async(Cancellable? cancellable, out DateTime? last_reap_time, out DateTime? last_vacuum_time, out int reaped_messages_since_last_vacuum, out int64 free_page_bytes) throws Error { From 7be69db7f9e2aad2d16d042b4466a1f23254399c Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Tue, 22 Jan 2019 11:03:42 +1100 Subject: [PATCH 7/7] Ensure the ImapDB.Database garbage collector exits in close_async The gc will prevent unit tests from completing and hence causing timeouts, so ensure it finishes before returning when closing the db. --- src/engine/imap-db/imap-db-database.vala | 41 +++++++++++++++--------- src/engine/imap-db/imap-db-gc.vala | 2 +- 2 files changed, 26 insertions(+), 17 deletions(-) diff --git a/src/engine/imap-db/imap-db-database.vala b/src/engine/imap-db/imap-db-database.vala index 807b6c06..f00ff910 100644 --- a/src/engine/imap-db/imap-db-database.vala +++ b/src/engine/imap-db/imap-db-database.vala @@ -17,6 +17,8 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { private ProgressMonitor vacuum_monitor; private string account_owner_email; private bool new_db = false; + + private GC? gc = null; private Cancellable gc_cancellable = new Cancellable(); public Database(GLib.File db_file, @@ -45,21 +47,23 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { // called if (cancellable != null) cancellable.cancelled.connect(cancel_gc); - + // Create new garbage collection object for this database - GC gc = new GC(this, Priority.LOW); - + this.gc = new GC(this, Priority.LOW); + // Get recommendations on what GC operations should be executed - GC.RecommendedOperation recommended = yield gc.should_run_async(gc_cancellable); - + GC.RecommendedOperation recommended = yield this.gc.should_run_async( + gc_cancellable + ); + // VACUUM needs to execute in the foreground with the user given a busy prompt (and cannot // be run at the same time as REAP) if ((recommended & GC.RecommendedOperation.VACUUM) != 0) { if (!vacuum_monitor.is_in_progress) vacuum_monitor.notify_start(); - + try { - yield gc.vacuum_async(gc_cancellable); + yield this.gc.vacuum_async(gc_cancellable); } catch (Error err) { message( "Vacuum of IMAP database %s failed: %s", this.path, err.message @@ -70,35 +74,40 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { vacuum_monitor.notify_finish(); } } - + // REAP can run in the background while the application is executing if ((recommended & GC.RecommendedOperation.REAP) != 0) { // run in the background and allow application to continue running - gc.reap_async.begin(gc_cancellable, on_reap_async_completed); + this.gc.reap_async.begin(gc_cancellable, on_reap_async_completed); } - + if (cancellable != null) cancellable.cancelled.disconnect(cancel_gc); } - + private void on_reap_async_completed(Object? object, AsyncResult result) { - GC gc = (GC) object; try { - gc.reap_async.end(result); + this.gc.reap_async.end(result); } catch (Error err) { message("Garbage collection of IMAP database %s failed: %s", this.path, err.message); } + + this.gc = null; } - + private void cancel_gc() { gc_cancellable.cancel(); gc_cancellable = new Cancellable(); } - + public override void close(Cancellable? cancellable) throws Error { + // Ensure GC shuts down before returning cancel_gc(); - + while (this.gc != null && this.gc.is_running) { + GLib.MainContext.default().iteration(false); + } + base.close(cancellable); } diff --git a/src/engine/imap-db/imap-db-gc.vala b/src/engine/imap-db/imap-db-gc.vala index 5f21bcbe..dd170850 100644 --- a/src/engine/imap-db/imap-db-gc.vala +++ b/src/engine/imap-db/imap-db-gc.vala @@ -81,7 +81,7 @@ private class Geary.ImapDB.GC { */ public bool is_running { get; private set; default = false; } - private ImapDB.Database db; + private weak ImapDB.Database db; private int priority; public GC(ImapDB.Database db, int priority) {