[geary] Revised handling of append/remove upcalls: Closes bgno#721326

[Jim Nelson <jnelson src gnome org>]

commit bd83b8bc3e79ad11bdad846e8c4186d47c9e6652
Author: Jim Nelson <jim yorba org>
Date:   Mon Jan 13 11:49:30 2014 -0800

    Revised handling of append/remove upcalls: Closes bgno#721326
    
    This approach immediately updates the remote_count when an upcall
    is received, as that math is important for determining positional
    addressing if another one immediately follows it.  However, the
    async calls only deal in the remote count *at the time the upcall
    arrived*, in effect allowing for the upcall to be serially processed
    using async blocking calls.

 .../imap-engine/imap-engine-generic-folder.vala    |   93 +++++++++++--------
 .../replay-ops/imap-engine-replay-append.vala      |   11 ++-
 .../replay-ops/imap-engine-replay-removal.vala     |    6 +-
 3 files changed, 66 insertions(+), 44 deletions(-)
---
diff --git a/src/engine/imap-engine/imap-engine-generic-folder.vala 
b/src/engine/imap-engine/imap-engine-generic-folder.vala
index 026a1f4..e93c065 100644
--- a/src/engine/imap-engine/imap-engine-generic-folder.vala
+++ b/src/engine/imap-engine/imap-engine-generic-folder.vala
@@ -723,17 +723,25 @@ private class Geary.ImapEngine.GenericFolder : Geary.AbstractFolder, Geary.Folde
         notify_email_locally_complete(email_ids);
     }
     
-    private void on_remote_appended(int new_remote_count) {
-        debug("%s on_remote_appended: new_remote_count=%d", to_string(), new_remote_count);
+    private void on_remote_appended(int reported_remote_count) {
+        debug("%s on_remote_appended: remote_count=%d reported_remote_count=%d", to_string(), remote_count,
+            reported_remote_count);
+        
+        if (reported_remote_count < 0)
+            return;
         
         // from the new remote total and the old remote total, glean the SequenceNumbers of the
         // new email(s)
         Gee.List<Imap.SequenceNumber> positions = new Gee.ArrayList<Imap.SequenceNumber>();
-        for (int pos = remote_count + 1; pos <= new_remote_count; pos++)
+        for (int pos = remote_count + 1; pos <= reported_remote_count; pos++)
             positions.add(new Imap.SequenceNumber(pos));
         
+        // store the remote count NOW, as further appended messages could arrive before the
+        // ReplayAppend executes
+        remote_count = reported_remote_count;
+        
         if (positions.size > 0)
-            replay_queue.schedule_server_notification(new ReplayAppend(this, positions));
+            replay_queue.schedule_server_notification(new ReplayAppend(this, reported_remote_count, 
positions));
     }
     
     // Need to prefetch at least an EmailIdentifier (and duplicate detection fields) to create a
@@ -743,14 +751,15 @@ private class Geary.ImapEngine.GenericFolder : Geary.AbstractFolder, Geary.Folde
     // which is exactly what we want.
     //
     // This MUST only be called from ReplayAppend.
-    internal async void do_replay_appended_messages(Gee.List<Imap.SequenceNumber> remote_positions) {
+    internal async void do_replay_appended_messages(int reported_remote_count,
+        Gee.List<Imap.SequenceNumber> remote_positions) {
         StringBuilder positions_builder = new StringBuilder("( ");
         foreach (Imap.SequenceNumber remote_position in remote_positions)
             positions_builder.append_printf("%s ", remote_position.to_string());
         positions_builder.append(")");
         
-        debug("%s do_replay_appended_message: remote_count=%d remote_positions=%s", to_string(),
-            remote_count, positions_builder.str);
+        debug("%s do_replay_appended_message: current remote_count=%d reported_remote_count=%d 
remote_positions=%s",
+            to_string(), remote_count, reported_remote_count, positions_builder.str);
         
         if (remote_positions.size == 0)
             return;
@@ -800,15 +809,13 @@ private class Geary.ImapEngine.GenericFolder : Geary.AbstractFolder, Geary.Folde
                 to_string(), err.message);
         }
         
-        // save new remote count internally and in local store
-        // NOTE: use remote_positions size, not created/appended, as the former is a true indication
-        // of the count on the server
-        remote_count += remote_positions.size;
+        // store the reported count, *not* the current count (which is updated outside the of
+        // the queue) to ensure that updates happen serially and reflect committed local changes
         try {
-            yield local_folder.update_remote_selected_message_count(remote_count, null);
+            yield local_folder.update_remote_selected_message_count(reported_remote_count, null);
         } catch (Error err) {
             debug("%s do_replay_appended_message: Unable to save appended remote count %d: %s",
-                to_string(), remote_count, err.message);
+                to_string(), reported_remote_count, err.message);
         }
         
         if (appended.size > 0)
@@ -817,25 +824,39 @@ private class Geary.ImapEngine.GenericFolder : Geary.AbstractFolder, Geary.Folde
         if (created.size > 0)
             notify_email_locally_appended(created);
         
-        notify_email_count_changed(remote_count, CountChangeReason.APPENDED);
+        notify_email_count_changed(reported_remote_count, CountChangeReason.APPENDED);
         
-        debug("%s do_replay_appended_message: completed remote_count=%d", to_string(), remote_count);
+        debug("%s do_replay_appended_message: completed, current remote_count=%d reported_remote_count=%d",
+            to_string(), remote_count, reported_remote_count);
     }
     
-    private void on_remote_removed(Imap.SequenceNumber position, int new_remote_count) {
-        debug("%s on_remote_removed: position=%s new_remote_count=%d", to_string(), position.to_string(),
-            new_remote_count);
+    private void on_remote_removed(Imap.SequenceNumber position, int reported_remote_count) {
+        debug("%s on_remote_removed: remote_count=%d position=%s reported_remote_count=%d", to_string(),
+            remote_count, position.to_string(), reported_remote_count);
+        
+        if (reported_remote_count < 0)
+            return;
         
         // notify of removal to all pending replay operations
         replay_queue.notify_remote_removed_position(position);
         
-        replay_queue.schedule_server_notification(new ReplayRemoval(this, position));
+        // update remote count NOW, as further appended and removed messages can arrive before
+        // ReplayRemoval executes
+        //
+        // something to note at this point: the ExpungeEmail operation marks messages as removed,
+        // then signals they're removed and reports an adjusted count in its replay_local_async().
+        // remote_count is *not* updated, which is why it's safe to do that here without worry.
+        // similarly, signals are only fired here if marked, so the same EmailIdentifier isn't
+        // reported twice
+        remote_count = reported_remote_count;
+        
+        replay_queue.schedule_server_notification(new ReplayRemoval(this, reported_remote_count, position));
     }
     
     // This MUST only be called from ReplayRemoval.
-    internal async void do_replay_removed_message(Imap.SequenceNumber remote_position) {
-        debug("%s do_replay_removed_message: remote_position=%d remote_count=%d",
-            to_string(), remote_position.value, remote_count);
+    internal async void do_replay_removed_message(int reported_remote_count, Imap.SequenceNumber 
remote_position) {
+        debug("%s do_replay_removed_message: current remote_count=%d remote_position=%d 
reported_remote_count=%d",
+            to_string(), remote_count, remote_position.value, reported_remote_count);
         
         if (!remote_position.is_valid()) {
             debug("%s do_replay_removed_message: ignoring, invalid remote position or count",
@@ -853,7 +874,7 @@ private class Geary.ImapEngine.GenericFolder : Geary.AbstractFolder, Geary.Folde
             // from server's point of view, not client's
             local_count = yield local_folder.get_email_count_async(
                 ImapDB.Folder.ListFlags.INCLUDE_MARKED_FOR_REMOVE, null);
-            local_position = remote_position.value - (remote_count - local_count);
+            local_position = remote_position.value - (reported_remote_count + 1 - local_count);
             
             // zero or negative means the message exists beyond the local vector's range, so
             // nothing to do there
@@ -887,8 +908,8 @@ private class Geary.ImapEngine.GenericFolder : Geary.AbstractFolder, Geary.Folde
             replay_queue.notify_remote_removed_ids(new 
Collection.SingleItem<ImapDB.EmailIdentifier>(owned_id));
         } else {
             debug("%s do_replay_removed_message: remote_position=%d unknown in local store "
-                + "(remote_count=%d local_position=%d local_count=%d)",
-                to_string(), remote_position.value, remote_count, local_position, local_count);
+                + "(reported_remote_count=%d local_position=%d local_count=%d)",
+                to_string(), remote_position.value, reported_remote_count, local_position, local_count);
         }
         
         // for debugging
@@ -901,16 +922,10 @@ private class Geary.ImapEngine.GenericFolder : Geary.AbstractFolder, Geary.Folde
                 err.message);
         }
         
-        // something to note at this point: the ExpungeEmail operation marks messages as removed,
-        // then signals they're removed and reports an adjusted count in its replay_local_async().
-        // remote_count is *not* updated, which is why it's safe to do that here without worry.
-        // similarly, signals are only fired here if marked, so the same EmailIdentifier isn't
-        // reported twice
-        
-        // save new remote count internally and in local store
-        remote_count = Numeric.int_floor(remote_count - 1, 0);
+        // as with on_remote_appended(), only update in local store inside a queue operation, to
+        // ensure serial commits
         try {
-            yield local_folder.update_remote_selected_message_count(remote_count, null);
+            yield local_folder.update_remote_selected_message_count(reported_remote_count, null);
         } catch (Error err) {
             debug("%s do_replay_removed_message: unable to save removed remote count: %s", to_string(),
                 err.message);
@@ -921,12 +936,12 @@ private class Geary.ImapEngine.GenericFolder : Geary.AbstractFolder, Geary.Folde
             notify_email_removed(new Collection.SingleItem<Geary.EmailIdentifier>(owned_id));
         
         if (!marked)
-            notify_email_count_changed(remote_count, CountChangeReason.REMOVED);
+            notify_email_count_changed(reported_remote_count, CountChangeReason.REMOVED);
         
-        debug("%s do_replay_remove_message: completed "
-            + "(remote_count=%d local_count=%d starting local_count=%d remote_position=%d local_position=%d 
marked=%s)",
-            to_string(), remote_count, new_local_count, local_count, remote_position.value, local_position,
-            marked.to_string());
+        debug("%s do_replay_remove_message: completed, current remote_count=%d "
+            + "(reported_remote_count=%d local_count=%d starting local_count=%d remote_position=%d 
local_position=%d marked=%s)",
+            to_string(), remote_count, reported_remote_count, new_local_count, local_count, 
remote_position.value,
+            local_position, marked.to_string());
     }
     
     private void on_remote_disconnected(Imap.ClientSession.DisconnectReason reason) {
diff --git a/src/engine/imap-engine/replay-ops/imap-engine-replay-append.vala 
b/src/engine/imap-engine/replay-ops/imap-engine-replay-append.vala
index 695aa2e..32de08b 100644
--- a/src/engine/imap-engine/replay-ops/imap-engine-replay-append.vala
+++ b/src/engine/imap-engine/replay-ops/imap-engine-replay-append.vala
@@ -6,12 +6,14 @@
 
 private class Geary.ImapEngine.ReplayAppend : Geary.ImapEngine.ReceiveReplayOperation {
     public GenericFolder owner;
+    public int remote_count;
     public Gee.List<Imap.SequenceNumber> positions;
     
-    public ReplayAppend(GenericFolder owner, Gee.List<Imap.SequenceNumber> positions) {
+    public ReplayAppend(GenericFolder owner, int remote_count, Gee.List<Imap.SequenceNumber> positions) {
         base ("Append");
         
         this.owner = owner;
+        this.remote_count = remote_count;
         this.positions = positions;
     }
     
@@ -30,6 +32,9 @@ private class Geary.ImapEngine.ReplayAppend : Geary.ImapEngine.ReceiveReplayOper
         }
         
         positions = new_positions;
+        
+        // DON'T update remote_count, it is intended to report the remote count at the time the
+        // appended messages arrived
     }
     
     public override void notify_remote_removed_ids(Gee.Collection<ImapDB.EmailIdentifier> ids) {
@@ -40,13 +45,13 @@ private class Geary.ImapEngine.ReplayAppend : Geary.ImapEngine.ReceiveReplayOper
     
     public override async ReplayOperation.Status replay_local_async() {
         if (positions.size > 0)
-            yield owner.do_replay_appended_messages(positions);
+            yield owner.do_replay_appended_messages(remote_count, positions);
         
         return ReplayOperation.Status.COMPLETED;
     }
     
     public override string describe_state() {
-        return "positions.size=%d".printf(positions.size);
+        return "remote_count=%d positions.size=%d".printf(remote_count, positions.size);
     }
 }
 
diff --git a/src/engine/imap-engine/replay-ops/imap-engine-replay-removal.vala 
b/src/engine/imap-engine/replay-ops/imap-engine-replay-removal.vala
index 3822080..be4a24f 100644
--- a/src/engine/imap-engine/replay-ops/imap-engine-replay-removal.vala
+++ b/src/engine/imap-engine/replay-ops/imap-engine-replay-removal.vala
@@ -6,12 +6,14 @@
 
 private class Geary.ImapEngine.ReplayRemoval : Geary.ImapEngine.ReceiveReplayOperation {
     public GenericFolder owner;
+    public int remote_count;
     public Imap.SequenceNumber position;
     
-    public ReplayRemoval(GenericFolder owner, Imap.SequenceNumber position) {
+    public ReplayRemoval(GenericFolder owner, int remote_count, Imap.SequenceNumber position) {
         base ("Removal");
         
         this.owner = owner;
+        this.remote_count = remote_count;
         this.position = position;
     }
     
@@ -29,7 +31,7 @@ private class Geary.ImapEngine.ReplayRemoval : Geary.ImapEngine.ReceiveReplayOpe
     }
     
     public override async ReplayOperation.Status replay_local_async() throws Error {
-        yield owner.do_replay_removed_message(position);
+        yield owner.do_replay_removed_message(remote_count, position);
         
         return ReplayOperation.Status.COMPLETED;
     }