sync: hook up ServerConnectionManager <> SyncerThread2 and tie up more loose ends

chrome/browser/sync/engine/syncer_thread2.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/sync/engine/syncer_thread2.h"
 
 #include <algorithm>
 
 #include "base/rand_util.h"
 #include "chrome/browser/sync/engine/syncer.h"
@@ skipping 60 matching lines @@
       server_connection_ok_(false),
       delay_provider_(new DelayProvider()),
       syncer_(syncer),
       session_context_(context) {
 }
 
 SyncerThread::~SyncerThread() {
   DCHECK(!thread_.IsRunning());
 }
 
-void SyncerThread::Start(Mode mode) {
-  if (!thread_.IsRunning() && !thread_.Start()) {
-    NOTREACHED() << "Unable to start SyncerThread.";
-    return;
+void SyncerThread::CheckServerConnectionManagerStatus(
+    HttpResponse::ServerConnectionCode code) {
+  // Note, be careful when adding cases here because if the SyncerThread
+  // thinks there is no valid connection as determined by this method, it
+  // will drop out of *all* forward progress sync loops (it won't poll and it
+  // will queue up Talk notifications but not actually call SyncShare) until
+  // some external action causes a ServerConnectionManager to broadcast that
+  // a valid connection has been re-established.
+  if (HttpResponse::CONNECTION_UNAVAILABLE == code ||
+      HttpResponse::SYNC_AUTH_ERROR == code) {
+    server_connection_ok_ = false;
+  } else if (HttpResponse::SERVER_CONNECTION_OK == code) {
+    server_connection_ok_ = true;

[lipalani1, 2011/03/25 23:18:30]

This seems to be a little odd. There is no catch all else condition probably to
leave the variable server_connection_ok_ untouched in that case.

A better approach might be to use the server_reachable_ value that we get as
part of the event listener notification.

[tim (not reviewing), 2011/03/27 19:23:19]

We surface both 'reachable' and 'server up' and 'server broken' in about:sync
and they mean 3 different things.  Not sure what you're proposing? Note,
this was copy/pasted from the existing code in syncer_thread.cc and i'm trying
to keep parity where possible.

+  }
+}
+
+void SyncerThread::Start(Mode mode, ModeChangeCallback* callback) {

[lipalani1, 2011/03/25 23:18:30]

I am assuming start can be called from only one thread(sync_core?) it might
makes sense to add a DCHECK here on the thread that is calling it.
I assuming,based on the lack of locks, it is not designed to be called from
multiple threads.

[tim (not reviewing), 2011/03/27 19:23:19]

Right.  We can't easily add a DCHECK though since the core thread is not a "well
known" browser thread.  I could build something I suppose to stash the
created-on-message loop and dcheck, but I'm not sure that's what you wanted...
One day soon I hope to eliminate core_thread altogether by exposing a
MessageLoopProxy from the syncer thread interface.

+  if (!thread_.IsRunning()) {
+    if (!thread_.Start()) {
+      NOTREACHED() << "Unable to start SyncerThread.";
+      return;
+    }
+    thread_.message_loop()->PostTask(FROM_HERE, NewRunnableMethod(
+        this, &SyncerThread::WatchConnectionManager));
+    thread_.message_loop()->PostTask(FROM_HERE, NewRunnableMethod(
+        this, &SyncerThread::SendInitialSnapshot));
   }
 
   thread_.message_loop()->PostTask(FROM_HERE, NewRunnableMethod(
-      this, &SyncerThread::StartImpl, mode));
+      this, &SyncerThread::StartImpl, mode, make_linked_ptr(callback)));
 }
 
-void SyncerThread::StartImpl(Mode mode) {
+void SyncerThread::SendInitialSnapshot() {
+  DCHECK_EQ(MessageLoop::current(), thread_.message_loop());
+  scoped_ptr<SyncSession> dummy(new SyncSession(session_context_.get(), this,
+      SyncSourceInfo(), ModelSafeRoutingInfo(),
+      std::vector<ModelSafeWorker*>()));
+  SyncEngineEvent event(SyncEngineEvent::STATUS_CHANGED);
+  sessions::SyncSessionSnapshot snapshot(dummy->TakeSnapshot());
+  event.snapshot = &snapshot;
+  session_context_->NotifyListeners(event);
+}
+
+void SyncerThread::WatchConnectionManager() {
+  ServerConnectionManager* scm = session_context_->connection_manager();
+  CheckServerConnectionManagerStatus(scm->server_status());

[lipalani1, 2011/03/25 23:18:30]

The name(CheckServerConnectionManagerStatus) seems a little confusing here. This
actually sets the server_connect_ok_ right? what does it check?

[tim (not reviewing), 2011/03/27 19:23:19]

It checks the passed-in value to figure out what to set the value to.  Agree it
could have a better name (again this was copied from syncer_thread). 
UpdateServerConnectionStatus?  That's a bit ambiguous too but maybe better.

+  scm->AddListener(this);
+}
+
+void SyncerThread::StartImpl(Mode mode,
+                             linked_ptr<ModeChangeCallback> callback) {
   DCHECK_EQ(MessageLoop::current(), thread_.message_loop());
   DCHECK(!session_context_->account_name().empty());
   DCHECK(syncer_.get());
   mode_ = mode;
   AdjustPolling(NULL);  // Will kick start poll timer if needed.
+  if (callback.get())
+    callback->Run();

[lipalani1, 2011/03/25 23:18:30]

This callback seems to be never used? what is the purpose of this? Future use?

[tim (not reviewing), 2011/03/27 19:23:19]

See StartConfigurationMode in syncapi -- we use this when the user changes the
set of active data types to alert the DTM when it's safe to proceed with
associations.

 }
 
 bool SyncerThread::ShouldRunJob(SyncSessionJobPurpose purpose,
     const TimeTicks& scheduled_start) {
   DCHECK_EQ(MessageLoop::current(), thread_.message_loop());
 
   // Check wait interval.
   if (wait_interval_.get()) {
     // TODO(tim): Consider different handling for CLEAR_USER_DATA (i.e. permit
     // when throttled).
@@ skipping 107 matching lines @@
   SyncSession* session = new SyncSession(session_context_.get(), this,
       SyncSourceInfo(), ModelSafeRoutingInfo(),
       std::vector<ModelSafeWorker*>());
   ScheduleSyncSessionJob(TimeDelta::FromSeconds(0), CLEAR_USER_DATA, session);
 }
 
 void SyncerThread::ScheduleNudgeImpl(const TimeDelta& delay,
     NudgeSource source, const ModelTypePayloadMap& types_with_payloads) {
   DCHECK_EQ(MessageLoop::current(), thread_.message_loop());
   TimeTicks rough_start = TimeTicks::Now() + delay;
+  if (!ShouldRunJob(NUDGE, rough_start)) {
+    LOG(WARNING) << "Dropping nudge at scheduling time, source = "
+                 << source;
+    return;
+  }

[lipalani1, 2011/03/25 23:18:30]

Is this needed here? When we run the job we check shouldrunjob once more and in
fact at that time we may be able to run this job...

[tim (not reviewing), 2011/03/27 19:23:19]

I originally thought the same, but it turns out it doesn't  make sense to
construct a session when we're in CONFIGURATION mode because we're configuring
things like the model safe routing info.

As a result, we drop some nudges during config.  So this was what my commet in
the CL email was about.  We need to track if we dropped a nudge during config. 
We could either just run it, or optimize by checking if there are unsynced items
or a pending notification.

 
   // Note we currently nudge for all types regardless of the ones incurring
   // the nudge.  Doing different would throw off some syncer commands like
   // CleanupDisabledTypes.  We may want to change this in the future.
   ModelSafeRoutingInfo routes;
   std::vector<ModelSafeWorker*> workers;
   session_context_->registrar()->GetModelSafeRoutingInfo(&routes);
   session_context_->registrar()->GetWorkers(&workers);
   SyncSourceInfo info(GetUpdatesFromNudgeSource(source),
                       types_with_payloads);
@@ skipping 108 matching lines @@
     case POLL:
       *start = SYNCER_BEGIN;
       return;
     default:
       NOTREACHED();
   }
 }
 
 void SyncerThread::DoSyncSessionJob(const SyncSessionJob& job) {
   DCHECK_EQ(MessageLoop::current(), thread_.message_loop());
+  if (!ShouldRunJob(job.purpose, job.scheduled_start))
+    return;

[lipalani1, 2011/03/25 23:18:30]

We need some kind of logging here.
I am assuming the following answers are no. But can you confirm?
1. when clear user data command is issued would we ever drop it?

[tim (not reviewing), 2011/03/27 19:23:19]

good question -- see bug 71616.  We currently do drop it but mainly for
consistency with other impl and simplicity for now, need to fix in future.

I can add a LOG WARNING here too.

 
   if (job.purpose == NUDGE) {
     DCHECK(pending_nudge_.get());
     if (pending_nudge_->session != job.session)
       return;  // Another nudge must have been scheduled in in the meantime.
     pending_nudge_.reset();
   }
 
   SyncerStep begin(SYNCER_BEGIN);
   SyncerStep end(SYNCER_END);
   SetSyncerStepsForPurpose(job.purpose, &begin, &end);
 
   bool has_more_to_sync = true;
-  bool did_job = false;
   while (ShouldRunJob(job.purpose, job.scheduled_start) && has_more_to_sync) {
     VLOG(1) << "SyncerThread: Calling SyncShare.";
-    did_job = true;
     // Synchronously perform the sync session from this thread.
     syncer_->SyncShare(job.session.get(), begin, end);
     has_more_to_sync = job.session->HasMoreToSync();
     if (has_more_to_sync)
       job.session->ResetTransientState();
   }
   VLOG(1) << "SyncerThread: Done SyncShare looping.";
-  if (did_job)
-    FinishSyncSessionJob(job);
+  FinishSyncSessionJob(job);
+}
+
+void SyncerThread::UpdateCarryoverSessionState(const SyncSessionJob& old_job) {
+  if (old_job.purpose == CONFIGURATION) {
+    // Whatever types were part of a configuration task will have had updates
+    // downloaded.  For that reason, we make sure they get recorded in the
+    // event that they get disabled at a later time.
+    ModelSafeRoutingInfo r(session_context_->previous_session_routing_info());
+    if (!r.empty()) {
+      ModelSafeRoutingInfo temp_r;
+      ModelSafeRoutingInfo old_info(old_job.session->routing_info());
+      std::set_union(r.begin(), r.end(), old_info.begin(), old_info.end(),
+          std::insert_iterator<ModelSafeRoutingInfo>(temp_r, temp_r.begin()));
+      session_context_->set_previous_session_routing_info(temp_r);
+    }
+  } else {
+    session_context_->set_previous_session_routing_info(
+        old_job.session->routing_info());
+  }
 }
 
 void SyncerThread::FinishSyncSessionJob(const SyncSessionJob& job) {
   DCHECK_EQ(MessageLoop::current(), thread_.message_loop());
   // Update timing information for how often datatypes are triggering nudges.
   base::TimeTicks now = TimeTicks::Now();
   if (!last_sync_session_end_time_.is_null()) {
     ModelTypePayloadMap::const_iterator iter;
     for (iter = job.session->source().types.begin();
          iter != job.session->source().types.end();
          ++iter) {
       syncable::PostTimeToTypeHistogram(iter->first,
                                         now - last_sync_session_end_time_);
     }
   }
   last_sync_session_end_time_ = now;
+  UpdateCarryoverSessionState(job);
   if (IsSyncingCurrentlySilenced())
     return;  // Nothing to do.
 
   VLOG(1) << "Updating the next polling time after SyncMain";
   ScheduleNextSync(job);
 }
 
 void SyncerThread::ScheduleNextSync(const SyncSessionJob& old_job) {
   DCHECK_EQ(MessageLoop::current(), thread_.message_loop());
   DCHECK(!old_job.session->HasMoreToSync());
@@ skipping 95 matching lines @@
 
   // Cap the backoff interval.
   backoff_s = std::max(static_cast<int64>(1),
                        std::min(backoff_s, kMaxBackoffSeconds));
 
   return TimeDelta::FromSeconds(backoff_s);
 }
 
 void SyncerThread::Stop() {
   syncer_->RequestEarlyExit();  // Safe to call from any thread.
+  session_context_->connection_manager()->RemoveListener(this);
   thread_.Stop();
-  Notify(SyncEngineEvent::SYNCER_THREAD_EXITING);
 }
 
 void SyncerThread::DoCanaryJob() {
   DCHECK(pending_nudge_.get());
   wait_interval_->had_nudge = false;
   SyncSessionJob copy = {pending_nudge_->purpose,
                          pending_nudge_->scheduled_start,
                          pending_nudge_->session};
   DoSyncSessionJob(copy);
 }
@@ skipping 49 matching lines @@
   DCHECK_EQ(MessageLoop::current(), thread_.message_loop());
   syncer_long_poll_interval_seconds_ = new_interval;
 }
 
 void SyncerThread::OnShouldStopSyncingPermanently() {
   syncer_->RequestEarlyExit();  // Thread-safe.
   Notify(SyncEngineEvent::STOP_SYNCING_PERMANENTLY);
 }
 
 void SyncerThread::OnServerConnectionEvent(
-    const ServerConnectionEvent& event) {
-  NOTIMPLEMENTED();
+    const ServerConnectionEvent2& event) {
+  CheckServerConnectionManagerStatus(event.connection_code);
 }
 
 void SyncerThread::set_notifications_enabled(bool notifications_enabled) {
   session_context_->set_notifications_enabled(notifications_enabled);
 }
 
 }  // s3
 }  // browser_sync