Use chrome/base synchronization primitives and threads instead of

pthreads in SyncerThread.  The old pthread impl can be used by
specifying --syncer-thread-pthreads for comparison until we
settle fully on the final impl (I have a MessageLoop-based impl
in progress).

The default SyncerThread is as close to the pthreads-impl semantics as
I could get, with one exception: it does not offer a time-out
when calling Stop(), because it greatly simplifies the implementation.
I first implemented it *with* the timeout, and for sake of
experimentation while this is in shuffle I am checking it in
as SyncerThreadTimedStop, available by using --syncer-thread-timed-stop. I'm not sure which we want ultimately, but it's useful to have around when building the MessageLoop based impl.

I had to refactor the interface slightly to allow multiple implementations,
I think it will be quite useful while working on the MessageLoop impl.

Added several tests to SyncerThreadUnittest, which all impls now pass (
just pass the command line flag to try each out).

TEST=SyncerThreadTest, SyncerThreadWithSyncerTest, integration tests

Review URL: http://codereview.chromium.org/214033

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@27117 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 64 insertions, 9 deletions

diff --git a/chrome/test/sync/engine/mock_server_connection.cc b/chrome/test/sync/engine/mock_server_connection.cc
index bd3d90f..c4595bd 100644
--- a/chrome/test/sync/engine/mock_server_connection.cc
+++ b/chrome/test/sync/engine/mock_server_connection.cc
@@ -25,6 +25,7 @@ using sync_pb::CommitResponse_EntryResponse;
 using sync_pb::GetUpdatesMessage;
 using sync_pb::SyncEntity;
 using syncable::DirectoryManager;
+using syncable::ScopedDirLookup;
 using syncable::WriteTransaction;
 
 MockConnectionManager::MockConnectionManager(DirectoryManager* dirmgr,
@@ -38,12 +39,13 @@ MockConnectionManager::MockConnectionManager(DirectoryManager* dirmgr,
       client_stuck_(false),
       commit_time_rename_prepended_string_(""),
       fail_next_postbuffer_(false),
-      directory_(dirmgr, name),
+      directory_manager_(dirmgr),
+      directory_name_(name),
       mid_commit_callback_function_(NULL),
+      mid_commit_observer_(NULL),
       client_command_(NULL),
       next_position_in_parent_(2) {
     server_reachable_ = true;
-    CHECK(directory_.good());
 };
 
 MockConnectionManager::~MockConnectionManager() {
@@ -60,9 +62,18 @@ void MockConnectionManager::SetMidCommitCallbackFunction(
   mid_commit_callback_function_ = callback;
 }
 
+void MockConnectionManager::SetMidCommitObserver(
+    MockConnectionManager::MidCommitObserver* observer) {
+    mid_commit_observer_ = observer;
+}
+
 bool MockConnectionManager::PostBufferToPath(const PostBufferParams* params,
                                              const string& path,
                                              const string& auth_token) {
+
+  ScopedDirLookup directory(directory_manager_, directory_name_);
+  CHECK(directory.good());
+
   ClientToServerMessage post;
   CHECK(post.ParseFromString(params->buffer_in));
   client_stuck_ = post.sync_problem_detected();
@@ -73,7 +84,7 @@ bool MockConnectionManager::PostBufferToPath(const PostBufferParams* params,
   // network. As we can't test this we do the next best thing and hang here
   // when there's an issue.
   {
-    WriteTransaction wt(directory_, syncable::UNITTEST, __FILE__, __LINE__);
+    WriteTransaction wt(directory, syncable::UNITTEST, __FILE__, __LINE__);
   }
   if (fail_next_postbuffer_) {
     fail_next_postbuffer_ = false;
@@ -105,9 +116,12 @@ bool MockConnectionManager::PostBufferToPath(const PostBufferParams* params,
   }
   response.SerializeToString(params->buffer_out);
   if (mid_commit_callback_function_) {
-    if (mid_commit_callback_function_(directory_))
+    if (mid_commit_callback_function_(directory))
       mid_commit_callback_function_ = 0;
   }
+  if (mid_commit_observer_) {
+    mid_commit_observer_->Observe();
+  }
   return result;
 }
 
diff --git a/chrome/test/sync/engine/mock_server_connection.h b/chrome/test/sync/engine/mock_server_connection.h
index e5573e0..8c0cca7 100644
--- a/chrome/test/sync/engine/mock_server_connection.h
+++ b/chrome/test/sync/engine/mock_server_connection.h
@@ -31,6 +31,10 @@ class MockConnectionManager : public browser_sync::ServerConnectionManager {
   // activity would normally take place. This aids simulation of race
   // conditions.
   typedef bool (*TestCallbackFunction)(syncable::Directory* dir);
+  class MidCommitObserver {
+   public:
+    virtual void Observe() = 0;
+  };
 
   MockConnectionManager(syncable::DirectoryManager* dirmgr, PathString name);
   virtual ~MockConnectionManager();
@@ -45,6 +49,7 @@ class MockConnectionManager : public browser_sync::ServerConnectionManager {
 
   // Control of commit response.
   void SetMidCommitCallbackFunction(TestCallbackFunction callback);
+  void SetMidCommitObserver(MidCommitObserver* observer);
 
   // Set this if you want commit to perform commit time rename. Will request
   // that the client renames all commited entries, prepending this string.
@@ -177,11 +182,13 @@ class MockConnectionManager : public browser_sync::ServerConnectionManager {
   bool fail_next_postbuffer_;
 
   // Our directory.
-  syncable::ScopedDirLookup directory_;
+  syncable::DirectoryManager* directory_manager_;
+  PathString directory_name_;
 
   // The updates we'll return to the next request.
   sync_pb::GetUpdatesResponse updates_;
   TestCallbackFunction mid_commit_callback_function_;
+  MidCommitObserver* mid_commit_observer_;
 
   scoped_ptr<sync_pb::ClientCommand> client_command_;
 
diff --git a/chrome/test/sync/engine/test_directory_setter_upper.cc b/chrome/test/sync/engine/test_directory_setter_upper.cc
index 468875a..0a7b32d 100644
--- a/chrome/test/sync/engine/test_directory_setter_upper.cc
+++ b/chrome/test/sync/engine/test_directory_setter_upper.cc
@@ -20,12 +20,15 @@ TestDirectorySetterUpper::TestDirectorySetterUpper() : name_(PSTR("Test")) {}
 
 TestDirectorySetterUpper::~TestDirectorySetterUpper() {}
 
-void TestDirectorySetterUpper::SetUp() {
+void TestDirectorySetterUpper::Init() {
   PathString test_data_dir_ = PSTR(".");
   manager_.reset(new DirectoryManager(test_data_dir_));
   file_path_ = manager_->GetSyncDataDatabasePath();
   PathRemove(file_path_.c_str());
+}
 
+void TestDirectorySetterUpper::SetUp() {
+  Init();
   ASSERT_TRUE(manager()->Open(name()));
 }
 
@@ -64,4 +67,19 @@ void TestDirectorySetterUpper::RunInvariantCheck(const ScopedDirLookup& dir) {
   }
 }
 
+void ManuallyOpenedTestDirectorySetterUpper::SetUp() {
+  Init();
+}
+
+void ManuallyOpenedTestDirectorySetterUpper::Open() {
+  ASSERT_TRUE(manager()->Open(name()));
+  was_opened_ = true;
+}
+
+void ManuallyOpenedTestDirectorySetterUpper::TearDown() {
+  if (was_opened_) {
+    TestDirectorySetterUpper::TearDown();
+  }
+}
+
 }  // namespace browser_sync
diff --git a/chrome/test/sync/engine/test_directory_setter_upper.h b/chrome/test/sync/engine/test_directory_setter_upper.h
index 0cac7e1..0cfa3e6 100644
--- a/chrome/test/sync/engine/test_directory_setter_upper.h
+++ b/chrome/test/sync/engine/test_directory_setter_upper.h
@@ -44,20 +44,23 @@ namespace browser_sync {
 class TestDirectorySetterUpper {
  public:
   TestDirectorySetterUpper();
-  ~TestDirectorySetterUpper();
+  virtual ~TestDirectorySetterUpper();
 
   // Create a DirectoryManager instance and use it to open the directory.
   // Clears any existing database backing files that might exist on disk.
-  void SetUp();
+  virtual void SetUp();
 
   // Undo everything done by SetUp(): close the directory and delete the
   // backing files. Before closing the directory, this will run the directory
   // invariant checks and perform the SaveChanges action on the directory.
-  void TearDown();
+  virtual void TearDown();
 
   syncable::DirectoryManager* manager() const { return manager_.get(); }
   const PathString& name() const { return name_; }
 
+ protected:
+  virtual void Init();
+
  private:
   void RunInvariantCheck(const syncable::ScopedDirLookup& dir);
 
@@ -66,6 +69,19 @@ class TestDirectorySetterUpper {
   PathString file_path_;
 };
 
+// A variant of the above where SetUp does not actually open the directory.
+// You must manually invoke Open().  This is useful if you are writing a test
+// that depends on the DirectoryManager::OPENED event.
+class ManuallyOpenedTestDirectorySetterUpper : public TestDirectorySetterUpper {
+ public:
+  ManuallyOpenedTestDirectorySetterUpper() : was_opened_(false) {}
+  virtual void SetUp();
+  virtual void TearDown();
+  void Open();
+ private:
+  bool was_opened_;
+};
+
 }  // namespace browser_sync
 
 #endif  // CHROME_TEST_SYNC_ENGINE_TEST_DIRECTORY_SETTER_UPPER_H_