Trim code from sync's ServerConnectionManager

chrome/browser/sync/engine/sync_scheduler.cc

 // Copyright (c) 2012 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/sync_scheduler.h"
 
 #include <algorithm>
 #include <cstring>
 
 #include "base/bind.h"
@@ skipping 176 matching lines @@
       started_(false),
       syncer_short_poll_interval_seconds_(
           TimeDelta::FromSeconds(kDefaultShortPollIntervalSeconds)),
       syncer_long_poll_interval_seconds_(
           TimeDelta::FromSeconds(kDefaultLongPollIntervalSeconds)),
       sessions_commit_delay_(
           TimeDelta::FromSeconds(kDefaultSessionsCommitDelaySeconds)),
       mode_(NORMAL_MODE),
       // Start with assuming everything is fine with the connection.
       // At the end of the sync cycle we would have the correct status.
-      server_connection_ok_(true),
       connection_code_(HttpResponse::SERVER_CONNECTION_OK),
       delay_provider_(new DelayProvider()),
       syncer_(syncer),
       session_context_(context) {
   DCHECK(sync_loop_);
 }
 
 SyncScheduler::~SyncScheduler() {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   StopImpl(base::Closure());
@@ skipping 15 matching lines @@
 
 void SyncScheduler::OnConnectionStatusChange() {
   if (HttpResponse::CONNECTION_UNAVAILABLE  == connection_code_) {
     // Optimistically assume that the connection is fixed and try
     // connecting.
     OnServerConnectionErrorFixed();
   }
 }
 
 void SyncScheduler::OnServerConnectionErrorFixed() {
-  DCHECK(!server_connection_ok_);
   connection_code_ = HttpResponse::SERVER_CONNECTION_OK;
-  server_connection_ok_ = true;
   PostTask(FROM_HERE, "DoCanaryJob",
            base::Bind(&SyncScheduler::DoCanaryJob,
                       weak_ptr_factory_.GetWeakPtr()));
 
 }
 
 void SyncScheduler::UpdateServerConnectionManagerStatus(
     HttpResponse::ServerConnectionCode code) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   SDVLOG(2) << "New server connection code: "
             << HttpResponse::GetServerConnectionCodeString(code);
-  bool old_server_connection_ok = server_connection_ok_;
 
   connection_code_ = code;
 
   // Note, be careful when adding cases here because if the SyncScheduler
   // 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;
     SDVLOG(2) << "Sync auth error or unavailable connection: "
               << "server connection is down";
   } else if (HttpResponse::SERVER_CONNECTION_OK == code) {
-    server_connection_ok_ = true;
     SDVLOG(2) << "Sync server connection is ok: "
               << "server connection is up, doing canary job";
   }
-
-  if (old_server_connection_ok != server_connection_ok_) {
-    const char* transition =
-        server_connection_ok_ ? "down -> up" : "up -> down";
-    SDVLOG(2) << "Server connection changed: " << transition;

[lipalani1, 2012/02/11 00:12:10]

There is some value to this log statement. You can still implement it based on
the connection code alone. if it is difficult(i dont think it is.) then it is
fine to remove it.

[rlarocque, 2012/02/11 01:31:36]

This log statement exists specifically to tell us about status changes.  That
made sense when we actually did something in response to status changes, but we
no longer take any action based on this.  

The logs in the if statement above will tell us what the status is now.  I don't
see why we would need to add logs specifically for tracking cases where that
status changes.  If you *really* need to see the transitions, you can find them
by grepping the logs.

-  }
 }
 
 void SyncScheduler::Start(Mode mode, const base::Closure& callback) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   std::string thread_name = MessageLoop::current()->thread_name();
   if (thread_name.empty())
     thread_name = "<Main thread>";
   SDVLOG(2) << "Start called from thread "
             << thread_name << " with mode " << GetModeString(mode);
   if (!started_) {
@@ skipping 95 matching lines @@
   // We are in normal mode.
   DCHECK_EQ(mode_, NORMAL_MODE);
   DCHECK_NE(job.purpose, SyncSessionJob::CONFIGURATION);
 
   // Freshness condition
   if (job.scheduled_start < last_sync_session_end_time_) {
     SDVLOG(2) << "Dropping job because of freshness";
     return DROP;
   }
 
-  if (server_connection_ok_)
+  if (!session_context_->connection_manager()->HasInvalidAuthToken())

[rlarocque, 2012/02/10 23:16:06]

You might be thinking that this condition is not the same as the old one.  You
would be right.  

We only reach this if statement when we're not in backoff.  If we were, we would
have hit the DecideWhileInWaitInterval() branch on line 355.  

So this if statement concerns the case where we're not in backoff, but
!server_connection_ok_.  That's a really strange state to be in.  I suppose that
could happen if a non-retryable job failed (ie. a POLL after issue 112954 is
fixed, CONFIGURE jobs).

I think the new logic will do a better job of detecting cases where we don't
want to even try to connect to the server.  Having no valid auth token is the
only scenario I can think of where it makes sense to not even try connecting to
the server, despite not being in backoff.  I'd prefer to err on the side of
CONTINUE rather than SAVE or DROP the job, since we'll usually up going into
backoff if it fails.  

[lipalani1, 2012/02/11 00:12:10]

Yep it was designed to capture cases where this variable was updated outside of
sync. It is fine to sync and figure out whether we have the connection.
On 2012/02/10 23:16:06, rlarocque wrote:

     return CONTINUE;
 
-  SDVLOG(2) << "Bad server connection. Using that to decide on job.";
+  SDVLOG(2) << "No valid auth token. Using that to decide on job.";
   return job.purpose == SyncSessionJob::NUDGE ? SAVE : DROP;
 }
 
 void SyncScheduler::InitOrCoalescePendingJob(const SyncSessionJob& job) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   DCHECK(job.purpose != SyncSessionJob::CONFIGURATION);
   if (pending_nudge_.get() == NULL) {
     SDVLOG(2) << "Creating a pending nudge job";
     SyncSession* s = job.session.get();
     scoped_ptr<SyncSession> session(new SyncSession(s->context(),
@@ skipping 443 matching lines @@
     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;
 
-  // Now update the status of the connection from SCM. We need this
-  // to decide whether we need to save/run future jobs. The notifications
-  // from SCM are not reliable.
+  // Now update the status of the connection from SCM. We need this to decide
+  // whether we need to save/run future jobs. The notifications from SCM are not
+  // reliable.
+  //
   // TODO(rlarocque): crbug.com/110954
-  // We should get rid of the notifications and
-  // it is probably not needed to maintain this status variable
-  // in 2 places. We should query it directly from SCM when needed.
-  // But that would need little more refactoring(including a method to
-  // query if the auth token is invalid) from SCM side.
+  // We should get rid of the notifications and it is probably not needed to
+  // maintain this status variable in 2 places. We should query it directly from
+  // SCM when needed.
   ServerConnectionManager* scm = session_context_->connection_manager();
   UpdateServerConnectionManagerStatus(scm->server_status());
 
   UpdateCarryoverSessionState(job);
   if (IsSyncingCurrentlySilenced()) {
     SDVLOG(2) << "We are currently throttled; not scheduling the next sync.";
     // TODO(sync): Investigate whether we need to check job.purpose
     // here; see DCHECKs in SaveJob().  (See http://crbug.com/90868.)
     SaveJob(job);
     return;  // Nothing to do.
@@ skipping 326 matching lines @@
 
 #undef SDVLOG_LOC
 
 #undef SDVLOG
 
 #undef SLOG
 
 #undef ENUM_CASE
 
 }  // browser_sync