Fix the false-positive detection of commit errors

chrome/browser/sync/engine/sync_scheduler.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/sync_scheduler.h"
 
 #include <algorithm>
 #include <cstring>
 
 #include "base/compiler_specific.h"
@@ skipping 795 matching lines @@
     return;  // Nothing to do.
   }
 
   SVLOG(2) << "Updating the next polling time after SyncMain";
   ScheduleNextSync(job);
 }
 
 void SyncScheduler::ScheduleNextSync(const SyncSessionJob& old_job) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   DCHECK(!old_job.session->HasMoreToSync());
-  // Note: |num_server_changes_remaining| > 0 here implies that we received a
-  // broken response while trying to download all updates, because the Syncer
-  // will loop until this value is exhausted. Also, if unsynced_handles exist
-  // but HasMoreToSync is false, this implies that the Syncer determined no
-  // forward progress was possible at this time (an error, such as an HTTP
-  // 500, is likely to have occurred during commit).
-  int num_server_changes_remaining =
-      old_job.session->status_controller()->num_server_changes_remaining();
-  size_t num_unsynced_handles =
-      old_job.session->status_controller()->unsynced_handles().size();
-  const bool work_to_do =
-      num_server_changes_remaining > 0 || num_unsynced_handles > 0;
-  SVLOG(2) << "num server changes remaining: " << num_server_changes_remaining
-           << ", num unsynced handles: " << num_unsynced_handles
-           << ", syncer has work to do: " << work_to_do;
 
   AdjustPolling(&old_job);
 
   // TODO(tim): Old impl had special code if notifications disabled. Needed?
-  if (!work_to_do) {
+  if (!old_job.session->WasUnableToMakeProgress()) {

[tim (not reviewing), 2011/09/20 16:21:57]

With the old "SyncerThread" this would have been broken as we would have dropped
nudges queued up while this session was in progress due to the freshness
condition check (which we still have in ShouldRunJob).  But since we'll now only
compute the scheduled_start time at ScheduleNudgeImpl *and* we always coalesce,
I think we're okay.  The freshness check was only useful if you could have
multiple nudges queued up (and we tried to implement it that way with the
scheduler, but for various reasons it didn't fly.)

The condition may still be used for the POLL path, but I'm thinking it is
useless for nudges now; it has to be useless if we know we do not want to check
for unsynced items here.

[rlarocque, 2011/09/20 20:10:03]

I don't follow.  How does this change affect the way we handle queued nudges?  

[tim (not reviewing), 2011/09/20 20:32:12]

I don't think it does. This was to encourage you to look at the code in
ShouldRunJob and consider how this change might affect the |freshness_condition|
check. Because if a change occurs while this session is in progress, and a nudge
is scheduled but we drop that nudge due to the freshness condition, then your
change would break that flow.

[lipalani1, 2011/09/20 23:15:56]

When this job is running no new job(nudge) can come in as they all are in the
same thread.(even when they were in different threads the behaviour would be the
same but complicated to explain :))
So this is the right behavior and ShouldRunJob assumes this behavior. But
Richard should look at it just for fun!
On 2011/09/20 20:32:12, timsteele wrote:

[lipalani1, 2011/09/20 23:18:09]

One more thing, an aside(for Richard): Nudge collapsing is useful when you want
to sync something in x seconds from now and many requests come in that period.
In which case we collapse everything and execute at the xth second.

However when a job has started we dont collapase anything after.
On 2011/09/20 23:15:56, lipalani1 wrote:

     // Success implies backoff relief.  Note that if this was a
     // "one-off" job (i.e. purpose ==
     // SyncSessionJob::{CLEAR_USER_DATA,CLEANUP_DISABLED_TYPES}), if
-    // there was work_to_do before it ran this wont have changed, as
+    // there was work to do before it ran this wont have changed, as
     // jobs like this don't run a full sync cycle.  So we don't need
     // special code here.
     wait_interval_.reset();
     SVLOG(2) << "Job succeeded so not scheduling more jobs";
     return;
   }
 
   // We are in backoff mode and our time did not run out. That means we had
   // a local change, notification from server or a network connection change
   // notification. In any case set had_nudge = true so we dont retry next
@@ skipping 10 matching lines @@
     wait_interval_->had_nudge = true;
     // Old job did not finish. So make it the pending job.
     InitOrCoalescePendingJob(old_job);
     // Resume waiting.
     RestartWaiting();
   } else if (old_job.session->source().updates_source ==
              GetUpdatesCallerInfo::SYNC_CYCLE_CONTINUATION) {
     SVLOG(2) << "Job failed with source continuation";
     // We don't seem to have made forward progress. Start or extend backoff.
     HandleConsecutiveContinuationError(old_job);
   } else {

[rlarocque, 2011/09/20 20:35:21]

FYI: This branch is practically unreachable.  I'm considering deleting it as
part of this patch.  

The else if condition above (... updates_source == SYNC_CYCLE_CONTINUATION) is
almost always true because we set the source to SYNC_CYCLE_CONTINUATION just a
few steps into the state machine.  Unless we kick out of SyncShare() very early,
that condition will evaluate to true.  

     SVLOG(2) << "Failed. Schedule a job with continuation as source";
     // We weren't continuing and we aren't in backoff.  Schedule a normal
     // continuation.
     if (old_job.purpose == SyncSessionJob::CONFIGURATION) {
       ScheduleConfigImpl(old_job.session->routing_info(),
           old_job.session->workers(),
           GetUpdatesFromNudgeSource(NUDGE_SOURCE_CONTINUATION));
     } else  {
       // For all other purposes(nudge and poll) we schedule a retry nudge.
       ScheduleNudgeImpl(TimeDelta::FromSeconds(0),
@@ skipping 274 matching lines @@
 
 #undef SVLOG_LOC
 
 #undef SVLOG
 
 #undef SLOG
 
 #undef ENUM_CASE
 
 }  // browser_sync