[Sync] - Implement isolated model association.

chrome/browser/sync/glue/model_association_manager.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 <algorithm>
 #include <functional>
 
 #include "base/debug/trace_event.h"
 
 #include "base/logging.h"
 #include "base/message_loop.h"
 #include "base/metrics/histogram.h"
 
 #include "chrome/browser/sync/glue/model_association_manager.h"
 
 #include "content/public/browser/browser_thread.h"
 
 using content::BrowserThread;
 using syncable::ModelTypeSet;
 
 namespace browser_sync {
+const int64 kDataTypeLoadWaitTimeInSeconds = 120;
 namespace {
 
 static const syncable::ModelType kStartOrder[] = {
   syncable::NIGORI,  //  Listed for completeness.
   syncable::BOOKMARKS,
   syncable::PREFERENCES,
   syncable::AUTOFILL,
   syncable::AUTOFILL_PROFILE,
   syncable::EXTENSION_SETTINGS,
   syncable::EXTENSIONS,
@@ skipping 48 matching lines @@
   // Build a ModelType -> order map for sorting.
   for (int i = 0; i < static_cast<int>(arraysize(kStartOrder)); i++)
     start_order_[kStartOrder[i]] = i;
 }
 
 ModelAssociationManager::~ModelAssociationManager() {
 }
 
 void ModelAssociationManager::Initialize(
     syncable::ModelTypeSet desired_types) {
   DCHECK(state_ == IDLE);

[tim (not reviewing), 2012/05/21 23:18:57]

DCHECK_EQ

[lipalani1, 2012/05/22 01:23:58]

Done.

   needs_start_.clear();
   needs_stop_.clear();
   failed_datatypes_info_.clear();
   desired_types_ = desired_types;
   state_ = INITIAILIZED_TO_CONFIGURE;
 
+  // Stop the types that are still loading from the previous configuration.
+  // If they are enabled we will start them here once again.
+
+  for (std::vector<DataTypeController*>::const_iterator it =
+           pending_model_load_.begin();
+       it != pending_model_load_.end();
+       ++it) {
+    (*it)->Stop();
+  }
+
+  pending_model_load_.clear();
+  waiting_to_associate_.clear();
+  currently_associating_ = NULL;
+
+
   // We need to calculate our |needs_start_| and |needs_stop_| list.
   GetControllersNeedingStart(&needs_start_);
   // Sort these according to kStartOrder.
   std::sort(needs_start_.begin(),
             needs_start_.end(),
             SortComparator(&start_order_));
 
   // Add any data type controllers into that needs_stop_ list that are
   // currently MODEL_STARTING, ASSOCIATING, RUNNING or DISABLED.
   for (DataTypeController::TypeMap::const_iterator it = controllers_->begin();
@@ skipping 64 matching lines @@
     if (dtc->state() != DataTypeController::NOT_RUNNING &&
         dtc->state() != DataTypeController::STOPPING) {
       dtc->Stop();
       DVLOG(1) << "Stopped " << dtc->name();
     }
   }
 
   if (need_to_call_model_association_done) {
     DataTypeManager::ConfigureResult result(DataTypeManager::ABORTED,
                                             desired_types_,
-                                            failed_datatypes_info_);
+                                            failed_datatypes_info_,
+                                            syncable::ModelTypeSet());
     result_processor_->OnModelAssociationDone(result);
   }
 
   failed_datatypes_info_.clear();
 }
 
 bool ModelAssociationManager::GetControllersNeedingStart(
     std::vector<DataTypeController*>* needs_start) {
   // Add any data type controllers into the needs_start_ list that are
   // currently NOT_RUNNING or STOPPING.
@@ skipping 10 matching lines @@
         needs_start->push_back(dtc->second.get());
       if (dtc->second->state() == DataTypeController::DISABLED) {
         DVLOG(1) << "Found " << syncable::ModelTypeToString(dtc->second->type())
                  << " in disabled state.";
       }
     }
   }
   return found_any;
 }
 
+void ModelAssociationManager::HandleFailedTypes(
+    DataTypeController::StartResult result,
+    const SyncError& error) {
+  failed_datatypes_info_.push_back(error);
+  LOG(ERROR) << "Failed to associate models for "
+             << syncable::ModelTypeToString(error.type());
+  UMA_HISTOGRAM_ENUMERATION("Sync.ConfigureFailed",
+                            error.type(),
+                            syncable::MODEL_TYPE_COUNT);
+
+}
+
 void ModelAssociationManager::TypeStartCallback(
     DataTypeController::StartResult result,
     const SyncError& error) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (state_ == ABORT) {
     // Now that we have finished with the current type we can stop
     // if abort was called.
     state_ = IDLE;
     return;
   }
 
   DCHECK(state_ == CONFIGURING);
 
   // We are done with this type. Clear it.
   DataTypeController* started_dtc = currently_associating_;
   currently_associating_ = NULL;
 
   if (result == DataTypeController::ASSOCIATION_FAILED) {
-    failed_datatypes_info_.push_back(error);
-    LOG(ERROR) << "Failed to associate models for "
-               << syncable::ModelTypeToString(error.type());
-    UMA_HISTOGRAM_ENUMERATION("Sync.ConfigureFailed",
-                              error.type(),
-                              syncable::MODEL_TYPE_COUNT);
+    HandleFailedTypes(result, error);
   }
 
   // If the type started normally, continue to the next type.
   // If the type is waiting for the cryptographer, continue to the next type.
   // Once the cryptographer is ready, we'll attempt to restart this type.
   // If this type encountered a type specific error continue to the next type.
   if (result == DataTypeController::NEEDS_CRYPTO ||
       result == DataTypeController::OK ||
       result == DataTypeController::OK_FIRST_RUN ||
       result == DataTypeController::ASSOCIATION_FAILED) {
@@ skipping 20 matching lines @@
   }
 
   std::list<SyncError> errors;
   errors.push_back(error);
 
   // Put our state to idle.
   state_ = IDLE;
 
   DataTypeManager::ConfigureResult configure_result(configure_status,
                                                     desired_types_,
-                                                    errors);
+                                                    errors,
+                                                    syncable::ModelTypeSet());
   result_processor_->OnModelAssociationDone(configure_result);
 }
 
 void ModelAssociationManager::LoadModelForNextType() {
   if (!needs_start_.empty()) {
     DVLOG(1) << "Starting " << needs_start_[0]->name();
 
     DataTypeController* dtc = needs_start_[0];
     needs_start_.erase(needs_start_.begin());
     // Move from |needs_start_| to |pending_model_load_|.
-    pending_model_load_.push_back(dtc);
+    pending_model_load_.insert(pending_model_load_.begin(), dtc);
+    timer_.Start(FROM_HERE,
+                 base::TimeDelta::FromSeconds(kDataTypeLoadWaitTimeInSeconds),
+                 this,
+                 &ModelAssociationManager::ModelLoadTimedOut);
     dtc->LoadModels(base::Bind(
         &ModelAssociationManager::ModelLoadCallback,
         weak_ptr_factory_.GetWeakPtr()));
+
     return;
   }
 
   // If all controllers have their |LoadModels| invoked then pass onto
   // |StartAssociatingNextType|.
   StartAssociatingNextType();
 }
 
 void ModelAssociationManager::ModelLoadCallback(
     syncable::ModelType type, SyncError error) {
-  DCHECK_EQ(state_, CONFIGURING);
-
-  for (std::vector<DataTypeController*>::iterator it =
-           pending_model_load_.begin();
-        it != pending_model_load_.end();
-        ++it) {
-    if ((*it)->type() == type) {
-      DataTypeController* dtc = *it;
-      pending_model_load_.erase(it);
-      if (!error.IsSet()) {
-        waiting_to_associate_.push_back(dtc);
-        StartAssociatingNextType();
-     } else {
-        // Treat it like a regular error.
-        DCHECK(currently_associating_ == NULL);
-        currently_associating_ = dtc;
-        TypeStartCallback(DataTypeController::ASSOCIATION_FAILED, error);
-     }
-     return;
+  if (state_ == CONFIGURING) {
+    for (std::vector<DataTypeController*>::iterator it =
+             pending_model_load_.begin();
+          it != pending_model_load_.end();
+          ++it) {
+      if ((*it)->type() == type) {
+        // If the type that got loaded was the type that was started

[tim (not reviewing), 2012/05/21 23:18:57]

'If the type that just finished loading was the last type to start, we can
safely stop the timer.'

is that what you're trying to say?  And this is true because the types run
serially and not in parallel -- I'd mention that as a final sentence on your
comment.

[lipalani1, 2012/05/22 01:23:58]

Done.

+        // last, then we could stop the timer.
+        if (it == pending_model_load_.begin()) {
+          timer_.Stop();
+        }
+        DataTypeController* dtc = *it;
+        pending_model_load_.erase(it);
+        if (!error.IsSet()) {
+          waiting_to_associate_.push_back(dtc);
+          StartAssociatingNextType();
+       } else {
+         // Treat it like a regular error.
+         HandleFailedTypes(DataTypeController::ASSOCIATION_FAILED, error);
+       }
+       return;
+      }
     }
+    NOTREACHED();
+    return;
+  }
+  else {
+    // We got a callback on type that we started loading during the

[tim (not reviewing), 2012/05/21 23:18:57]

"This data type finished loading after the deadline imposed by the originating
configuration cycle.  Inform the DataTypeManager that the type has loaded, so
that association may begin."

- don't use DTM / abbreviations in comments, helps for grep etc.

[lipalani1, 2012/05/22 01:23:58]

Done.

+    // previous configuration. Inform the DTM that the type has loaded.
+    result_processor_->OnTypesLoaded();
   }
 
-  NOTREACHED();
 }
 
 
 void ModelAssociationManager::StartAssociatingNextType() {
   DCHECK(state_ == CONFIGURING);
   DCHECK(currently_associating_ == NULL);
   if (!waiting_to_associate_.empty()) {
     DVLOG(1) << "Starting " << waiting_to_associate_[0]->name();
     TRACE_EVENT_BEGIN1("sync", "ModelAssociation",
                        "DataType",
@@ skipping 10 matching lines @@
   state_ = IDLE;
   // Do a fresh calculation to see if controllers need starting to account for
   // things like encryption, which may still need to be sorted out before we
   // can announce we're "Done" configuration entirely.
   if (GetControllersNeedingStart(NULL)) {
     DVLOG(1) << "GetControllersNeedingStart returned true. DTM blocked";
 
     DataTypeManager::ConfigureResult configure_result(
         DataTypeManager::CONFIGURE_BLOCKED,
         desired_types_,
-        failed_datatypes_info_);
+        failed_datatypes_info_,
+        syncable::ModelTypeSet());
     state_ = IDLE;
     result_processor_->OnModelAssociationDone(configure_result);
     return;
   }
 
   DataTypeManager::ConfigureStatus configure_status = DataTypeManager::OK;
-  if (!failed_datatypes_info_.empty()) {
+  if (!failed_datatypes_info_.empty() ||
+      !GetTypesWaitingToLoad().Empty()) {
     configure_status = DataTypeManager::PARTIAL_SUCCESS;
   }
   DataTypeManager::ConfigureResult result(configure_status,
                                           desired_types_,
-                                          failed_datatypes_info_);
+                                          failed_datatypes_info_,
+                                          GetTypesWaitingToLoad());
   result_processor_->OnModelAssociationDone(result);
   return;
 }
 
+void ModelAssociationManager::ModelLoadTimedOut() {

[tim (not reviewing), 2012/05/21 23:18:57]

Is this method actually needed?

[lipalani1, 2012/05/22 01:23:58]

Just for clarity purpose... Compiler probably optimizes this any way.
On 2012/05/21 23:18:57, timsteele wrote:

[tim (not reviewing), 2012/05/22 16:50:05]

Hmm, to me the extra indirection makes it less clear.  It's also easier for
someone to sneak in code that does something different in the timeout case, and
I'd like that to be a Big Event / called out in a future code review.

[lipalani1, 2012/05/22 20:20:46]

Done.

+  LoadModelForNextType();
+}
+
+syncable::ModelTypeSet ModelAssociationManager::GetTypesWaitingToLoad() {
+  syncable::ModelTypeSet result;
+  for (std::vector<DataTypeController*>::const_iterator it =
+           pending_model_load_.begin();
+       it != pending_model_load_.end();
+       ++it) {
+    result.Put((*it)->type());
+  }
+  return result;
+}
+
+void ModelAssociationManager::TestSimulateDataTypeLoadTimeout() {

[tim (not reviewing), 2012/05/21 23:18:57]

I think there is a way to do this that doesn't require adding test only
functionality like this and the model_association_manager() accessor, explained
below.  Note otherwise however the style guide stipulates that this method and
the accessor should be suffixed with "ForTesting", to ensure compatibility with
presubmit script.

One better option to remove the accessor by injecting ModelAssociationManager
into the DataTypeManager. From there, I would suggest one of the following

- Have ModelAssociationManager take a Timer on construction, since this is what
you ultimately want control over in your test (common enough). You can use
timer.user_task and timer.stop to get what you want.
- Stick SimulateDataTypeLoadTimeout on a test-only subclass that you would
inject from your test.

Another option is to use FRIEND_TEST_ALL_PREFIXES.  It appears only one test
needs this special behavior, but this is less ideal (especially given that you
have to dig through  a class to get here! I think injection is the right
answer.)

[lipalani1, 2012/05/22 01:23:58]

I think the eventual direction you are advocating is moving towards dependancy
injection. Which I think is cool and all of our code should adhere to.

We will have to change few components including PSS to inject the MAM class into
DTM. (and this is true of most components we have). I think modifying those
components is better as a different change list. and should be done with DI as
the goal. 

The other option is to make the timer injectable from DTM. But the problem with
that is test cases in DTM would still need an accessor.

For now I have taken the suggestion of user_task_ and created a test only
accessor for timer. (The reason this is better is: Accessors  gets us half way
to DI. Instead of injecting we are extracting. However the code that manipulates
the object that is being extracted(or injected) would remain the same even after
the move to DI. Another advantage of this is that we are only using the public
methods of the object extracted(injected) which is slightly preferable than
writing derived classes.)
On 2012/05/21 23:18:57, timsteele wrote:

[tim (not reviewing), 2012/05/22 16:50:05]

It sounds like we're in agreement that it would be better to inject the
parameters, so I guess if you're not doing it here this new code should be
accompanied by a TODO and a bug on file tracking the fact that it is incomplete.

In the meantime, the accessor for model_association_manager() should be called
GetModelAssociationManagerForTesting().  My main concern is other components
enforcing an unnecessary dependency / requirement that the DataTypeManager
exposes ModelAssociationManager.

[lipalani1, 2012/05/22 20:20:46]

I have addressed this. Let me know what you think.
On 2012/05/22 16:50:05, timsteele wrote:

+  timer_.Stop();
+  ModelLoadTimedOut();
+}
+
+
+
 }  // namespace browser_sync