sync: Expose sync functionality as functions

sync/engine/syncer.cc

Index: sync/engine/syncer.cc
diff --git a/sync/engine/syncer.cc b/sync/engine/syncer.cc
index 070f9abfdf7a2bbda97a8fb93e134d4f551140fd..d6b36a64aefc375254a9ecc22295c3da6a0557da 100644
--- a/sync/engine/syncer.cc
+++ b/sync/engine/syncer.cc
@@ -15,13 +15,13 @@
 #include "sync/engine/build_commit_command.h"
 #include "sync/engine/commit.h"
 #include "sync/engine/conflict_resolver.h"
-#include "sync/engine/download_updates_command.h"
+#include "sync/engine/download.h"
 #include "sync/engine/net/server_connection_manager.h"
 #include "sync/engine/process_commit_response_command.h"
-#include "sync/engine/process_updates_command.h"
-#include "sync/engine/store_timestamps_command.h"
 #include "sync/engine/syncer_types.h"
 #include "sync/internal_api/public/base/unique_position.h"
+#include "sync/internal_api/public/util/syncer_error.h"
+#include "sync/sessions/nudge_tracker.h"
 #include "sync/syncable/mutable_entry.h"
 #include "sync/syncable/syncable-inl.h"
 
@@ -31,35 +31,17 @@ using sync_pb::ClientCommand;
 
 namespace syncer {
 
+// TODO(akalin): We may want to propagate this switch up
+// eventually.
+#if defined(OS_ANDROID) || defined(OS_IOS)
+static const bool kCreateMobileBookmarksFolder = true;
+#else
+static const bool kCreateMobileBookmarksFolder = false;
+#endif
+
 using sessions::StatusController;
 using sessions::SyncSession;
-using syncable::IS_UNAPPLIED_UPDATE;
-using syncable::SERVER_CTIME;
-using syncable::SERVER_IS_DEL;
-using syncable::SERVER_IS_DIR;
-using syncable::SERVER_MTIME;
-using syncable::SERVER_NON_UNIQUE_NAME;
-using syncable::SERVER_PARENT_ID;
-using syncable::SERVER_SPECIFICS;
-using syncable::SERVER_UNIQUE_POSITION;
-using syncable::SERVER_VERSION;
-
-#define ENUM_CASE(x) case x: return #x
-const char* SyncerStepToString(const SyncerStep step)
-{
-  switch (step) {
-    ENUM_CASE(SYNCER_BEGIN);
-    ENUM_CASE(DOWNLOAD_UPDATES);
-    ENUM_CASE(PROCESS_UPDATES);
-    ENUM_CASE(STORE_TIMESTAMPS);
-    ENUM_CASE(APPLY_UPDATES);
-    ENUM_CASE(COMMIT);
-    ENUM_CASE(SYNCER_END);
-  }
-  NOTREACHED();
-  return "";
-}
-#undef ENUM_CASE
+using sessions::NudgeTracker;
 
 Syncer::Syncer()
     : early_exit_requested_(false) {
@@ -77,121 +59,103 @@ void Syncer::RequestEarlyExit() {
   early_exit_requested_ = true;
 }
 
-bool Syncer::SyncShare(sessions::SyncSession* session,
-                       SyncerStep first_step,
-                       SyncerStep last_step) {
-  session->mutable_status_controller()->UpdateStartTime();
-  SyncerStep current_step = first_step;
-
-  SyncerStep next_step = current_step;
-  while (!ExitRequested()) {
-    TRACE_EVENT1("sync", "SyncerStateMachine",
-                 "state", SyncerStepToString(current_step));
-    DVLOG(1) << "Syncer step:" << SyncerStepToString(current_step);
-
-    switch (current_step) {
-      case SYNCER_BEGIN:
-        session->SendEventNotification(SyncEngineEvent::SYNC_CYCLE_BEGIN);
-
-        next_step = DOWNLOAD_UPDATES;
-        break;
-      case DOWNLOAD_UPDATES: {
-        // TODO(akalin): We may want to propagate this switch up
-        // eventually.
-#if defined(OS_ANDROID) || defined(OS_IOS)
-        const bool kCreateMobileBookmarksFolder = true;
-#else
-        const bool kCreateMobileBookmarksFolder = false;
-#endif
-        DownloadUpdatesCommand download_updates(kCreateMobileBookmarksFolder);
-        session->mutable_status_controller()->set_last_download_updates_result(
-            download_updates.Execute(session));
-        next_step = PROCESS_UPDATES;
-        break;
-      }
-      case PROCESS_UPDATES: {
-        ProcessUpdatesCommand process_updates;
-        process_updates.Execute(session);
-        next_step = STORE_TIMESTAMPS;
-        break;
-      }
-      case STORE_TIMESTAMPS: {
-        StoreTimestampsCommand store_timestamps;
-        store_timestamps.Execute(session);
-        session->SendEventNotification(SyncEngineEvent::STATUS_CHANGED);
-        // We download all of the updates before attempting to apply them.
-        if (!session->status_controller().download_updates_succeeded()) {
-          // We may have downloaded some updates, but if the latest download
-          // attempt failed then we don't have all the updates.  We'll leave
-          // it to a retry job to pick up where we left off.
-          last_step = SYNCER_END; // Necessary for CONFIGURATION mode.
-          next_step = SYNCER_END;
-          DVLOG(1) << "Aborting sync cycle due to download updates failure";
-        } else if (!session->status_controller()
-                       .ServerSaysNothingMoreToDownload()) {
-          next_step = DOWNLOAD_UPDATES;
-        } else {
-          next_step = APPLY_UPDATES;
-        }
-        break;
-      }
-      case APPLY_UPDATES: {
-        // These include encryption updates that should be applied early.
-        ApplyControlDataUpdates(session);
-
-        ApplyUpdatesAndResolveConflictsCommand apply_updates;
-        apply_updates.Execute(session);
-
-        session->context()->set_hierarchy_conflict_detected(
-            session->status_controller().num_hierarchy_conflicts() > 0);
-
-        session->SendEventNotification(SyncEngineEvent::STATUS_CHANGED);
-        if (last_step == APPLY_UPDATES) {
-          // We're in configuration mode, but we still need to run the
-          // SYNCER_END step.
-          last_step = SYNCER_END;
-          next_step = SYNCER_END;
-        } else {
-          next_step = COMMIT;
-        }
-        break;
-      }
-      case COMMIT: {
-        session->mutable_status_controller()->set_commit_result(
-            BuildAndPostCommits(this, session));
-        next_step = SYNCER_END;
-        break;
-      }
-      case SYNCER_END: {
-        session->SendEventNotification(SyncEngineEvent::SYNC_CYCLE_ENDED);
-        next_step = SYNCER_END;
-        break;
-      }
-      default:
-        LOG(ERROR) << "Unknown command: " << current_step;
-    }  // switch
-    DVLOG(2) << "last step: " << SyncerStepToString(last_step) << ", "
-             << "current step: " << SyncerStepToString(current_step) << ", "
-             << "next step: " << SyncerStepToString(next_step) << ", "
-             << "snapshot: " << session->TakeSnapshot().ToString();
-    if (last_step == current_step)
+bool Syncer::NormalSyncShare(SyncSession* session,
+                             ModelTypeSet request_types,
+                             const NudgeTracker& nudge_tracker) {
+  session->SendEventNotification(SyncEngineEvent::SYNC_CYCLE_BEGIN);
+
+  VLOG(1) << "Downloading types " << ModelTypeSetToString(request_types);
+  while (!session->status_controller().ServerSaysNothingMoreToDownload()) {

[tim (not reviewing), 2013/06/21 17:41:38]

I think the general paradigm of "check ExitRequested before every major syncer
step" was useful at the very least to defend against new steps being added in
the future that block shutdown and may not be as performant as the ones we have
today. Previously, you added a SyncerCommand for a new step and that bail-out
was handled for you with code that has worked and has never changed since the
inception of syncer.cc. 

It also gave a tighter level of isolation than we have now from bugs being
introduced in a particular step (or, for example, in the SCM::Abort code, which
has happened before) from creating shutdown hangs (e.g on stable).

There's a small complexity benefit to be had in the boilerplate code today by
removing it. But it makes the framework slightly less defensive against
inadvertently slow code or bugs in the future. 

Do you see other pros/cons?

[rlarocque, 2013/06/21 18:30:36]

IMO, we never should have defaulted to checking ExitRequested at every step.

Every early exit code path is a potential source of bugs.  Each one must be
carefully considered to make sure that it's safe to stop processing at that
point.  The rarely hit paths are especially dangerous because they could slip
through testing and cause problems only after we deploy the code to millions of
users.  

As for the "pro" side, I'd argue that most of these checks have negligible
benefits.  The early exit during blocking network I/O makes sense.  However, the
other checks are almost useless.

Let's say our goal is to reduce worst case shutdown blocking time.  Today, the
current worst case is probably when we have lots of updates to apply, and the
shutdown request arrives just after the pre-APPLY_UPDATES ExitRequested().  In
this very rare case, we'll spend tens of milliseconds applying updates (~90ms in
a test on my dev machine).  

My changes will affect this metric.  The new worst case would be if the exit
request arrives just after the last download updates.  In that case, we would
have to pay the cost of PROCESS_UPDATES + STORE_TIMESTAMPS + APPLY_UPDATES. 
However, the time for PROCESS_UPDATES + STORE_TIMESTAMPS is around 4ms.  That's
not a big enough change for anyone to notice.

If we think tens of milliseconds is too long to block during shutdown, then we
should fix it.  However, I don't see how one could argue that 90ms of blocking
is OK, but 95ms is not.

+    SyncerError download_result = syncer::NormalDownloadUpdates(
+        session,
+        kCreateMobileBookmarksFolder,
+        request_types,
+        nudge_tracker);
+
+    session->mutable_status_controller()->set_last_download_updates_result(
+        download_result);
+    if (download_result != SYNCER_OK)

[tim (not reviewing), 2013/06/21 17:24:00]

If ExitRequested is true, we can / should note that we aborted by returning
'false' to the caller.

[rlarocque, 2013/06/21 18:30:36]

Good point.  I was trying to follow the same convention as the old code, but
forgot that the loop condition is (!ExitRequested()).  It would be more
backwards compatible to 'return !ExitRequested()'.

+      return true;
+  }
+  ApplyUpdates(session);
+
+  if (ExitRequested())
+    return false;
+
+  VLOG(1) << "Committing from types " << ModelTypeSetToString(request_types);
+  SyncerError commit_result = BuildAndPostCommits(request_types, this, session);
+  session->mutable_status_controller()->set_commit_result(commit_result);
+
+  if (ExitRequested()) {
+    return false;
+  } else {
+    session->SendEventNotification(SyncEngineEvent::SYNC_CYCLE_ENDED);
+    return true;
+  }
+}
+
+bool Syncer::ConfigureSyncShare(SyncSession* session,
+                                ModelTypeSet request_types) {
+  session->SendEventNotification(SyncEngineEvent::SYNC_CYCLE_BEGIN);
+
+  while (!session->status_controller().ServerSaysNothingMoreToDownload()) {
+    SyncerError download_result = ConfigureDownloadUpdates(
+        session,
+        kCreateMobileBookmarksFolder,
+        session->source(),
+        request_types);
+    session->mutable_status_controller()->set_last_download_updates_result(
+        download_result);
+    if (download_result != SYNCER_OK)
       return true;
-    current_step = next_step;
-  }  // while
-  return false;
+  }
+  ApplyUpdates(session);
+
+  if (ExitRequested()) {
+    return false;
+  } else {
+    session->SendEventNotification(SyncEngineEvent::SYNC_CYCLE_ENDED);
+    return true;
+  }
 }
 
-void CopyServerFields(syncable::Entry* src, syncable::MutableEntry* dest) {
-  dest->Put(SERVER_NON_UNIQUE_NAME, src->Get(SERVER_NON_UNIQUE_NAME));
-  dest->Put(SERVER_PARENT_ID, src->Get(SERVER_PARENT_ID));
-  dest->Put(SERVER_MTIME, src->Get(SERVER_MTIME));
-  dest->Put(SERVER_CTIME, src->Get(SERVER_CTIME));
-  dest->Put(SERVER_VERSION, src->Get(SERVER_VERSION));
-  dest->Put(SERVER_IS_DIR, src->Get(SERVER_IS_DIR));
-  dest->Put(SERVER_IS_DEL, src->Get(SERVER_IS_DEL));
-  dest->Put(IS_UNAPPLIED_UPDATE, src->Get(IS_UNAPPLIED_UPDATE));
-  dest->Put(SERVER_SPECIFICS, src->Get(SERVER_SPECIFICS));
-  dest->Put(SERVER_UNIQUE_POSITION, src->Get(SERVER_UNIQUE_POSITION));
+bool Syncer::PollSyncShare(SyncSession* session,
+                           ModelTypeSet request_types) {
+  session->SendEventNotification(SyncEngineEvent::SYNC_CYCLE_BEGIN);
+
+  while (!session->status_controller().ServerSaysNothingMoreToDownload()) {
+    SyncerError download_result = PollDownloadUpdates(
+        session,
+        kCreateMobileBookmarksFolder,
+        request_types);
+    session->mutable_status_controller()->set_last_download_updates_result(
+        download_result);
+    if (download_result != SYNCER_OK)
+      return true;
+  }
+  ApplyUpdates(session);
+
+  if (ExitRequested()) {
+    return false;
+  } else {
+    session->SendEventNotification(SyncEngineEvent::SYNC_CYCLE_ENDED);
+    return true;
+  }
+  return SYNCER_OK;

[tim (not reviewing), 2013/06/21 17:24:00]

I don't think you meant to leave this here.  I also think all three SyncShare
variants start and end the exact same way, and if we're making the syncer aware
of 'normal' 'config' anyway, I wonder why we wouldn't keep that in common so
it's only written / modified once

SyncShare(..., variant) {
  SendEventNotification(BEGIN);
  switch (variant) 
    NORMAL:
      NormalSyncShare(..)
    CONFIGURE:
    ...
      
  if (ExitRequested())
    false
  else 
    SendEventNotification(ENDED)
}

[rlarocque, 2013/06/21 18:30:36]

The SyncShare variants have different parameters.  It would be difficult to
share code in that way while still passing through the right parameters for each
type of SyncShare.

+}
+
+void Syncer::ApplyUpdates(SyncSession* session) {
+  TRACE_EVENT0("sync", "ApplyUpdates");
+

[tim (not reviewing), 2013/06/21 17:24:00]

What do you think about adding DCHECK(!ExitRequested()) here? I see no reason to
force any extra work at shutdown, it doesn't look like we were doing it
previously.

[rlarocque, 2013/06/21 18:30:36]

I don't think a DCHECK() is would be safe...  I'll assume you meant "if
(ExitRequested()) { return; }".

If we get here, that means that an early exit was not requested at the time that
our last DownloadUpdates() finished.  That was (at worst) ~5ms ago.  I don't
think it's worth checking again.

[tim (not reviewing), 2013/06/21 21:24:03]

Keep in mind that 90 milliseconds (as quoted above) on your state-of-the-art
powerhouse dev machine could grow considerably on slower hardware, crappy
phones, etc.  And few things can be as aggravating as a browser that won't
disappear despite frantic X-clicking. I can't make a case that sync is worthy of
holding shutdown for even 90ms longer.

Another data point: Chrome OS sends a SIGTERM if the browser hasn't shut down in
3 seconds (used to be 3, may have changed). Deciding to never abort operations
that we know may iterate over the entire set of synced items even when we know
it's time to (and safe to) quit only encroaches on that ceiling. Should the
timeout become more aggressive, and should we ever add more work to a sync cycle
(both of which I would argue *will happen* in time), having a well defined
boundary of steps where it is safe to "stop processing" is a good feature. We
have a reasonable de-coupling now with download + process + apply, and I think
it would be far more work to tease that apart later.

Lastly, aside from that, in my experience over the years we have seen
significant slowdowns from time to time in the various sync steps. Especially
with ApplyUpdates, and frankly, at shutdown, IMO the earlier we can stop 
needing all the various threads around to dispatch to, the better.

Does anyone else have any feelings on this?  I'd prefer keeping the bail-out at
the very least in the ApplyUpdates case.

+  ApplyControlDataUpdates(session);
+
+  ApplyUpdatesAndResolveConflictsCommand apply_updates;
+  apply_updates.Execute(session);
+
+  session->context()->set_hierarchy_conflict_detected(
+      session->status_controller().num_hierarchy_conflicts() > 0);
+
+  session->SendEventNotification(SyncEngineEvent::STATUS_CHANGED);
 }
 
 }  // namespace syncer