Skip to content

Commit

Permalink
sync: Support nudges from non-blocking sync types
Browse files Browse the repository at this point in the history
Implements support for receiving nudges from the non-blocking sync
engine.  When a non-blocking sync type requests a commit, it will also
send a request to the sync scheduler asking it to schedule
a sync cycle for some time in the future.

Adds some of the code required to support refresh requests, but does not
include an interface to allow clients of the non-blocking sync API to
access it.

Adds basic support for the initial download nudge.  When a non-blocking
type starts syncing for the first time, it sends a request to the
scheduler asking it to download any data available on the server.  This
allows it to complete initial sync quickly and without putting the
scheduler into configure mode.  For now, this looks like
a refresh request in the sync protocol.  This will be changed in
a future CL.

BUG=351005

Review URL: https://codereview.chromium.org/375023002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@282439 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
rlarocque@chromium.org committed Jul 10, 2014
1 parent b9f4c68 commit 1eed4f1
Show file tree
Hide file tree
Showing 17 changed files with 259 additions and 35 deletions.
9 changes: 8 additions & 1 deletion sync/engine/model_type_sync_worker_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,18 @@ namespace syncer {
ModelTypeSyncWorkerImpl::ModelTypeSyncWorkerImpl(
ModelType type,
const DataTypeState& initial_state,
NudgeHandler* nudge_handler,
scoped_ptr<ModelTypeSyncProxy> type_sync_proxy)
: type_(type),
data_type_state_(initial_state),
type_sync_proxy_(type_sync_proxy.Pass()),
nudge_handler_(nudge_handler),
entities_deleter_(&entities_),
weak_ptr_factory_(this) {
// Request an initial sync if it hasn't been completed yet.
if (!data_type_state_.initial_sync_done) {
nudge_handler_->NudgeForInitialDownload(type_);
}
}

ModelTypeSyncWorkerImpl::~ModelTypeSyncWorkerImpl() {
Expand Down Expand Up @@ -217,7 +223,8 @@ void ModelTypeSyncWorkerImpl::StorePendingCommit(
request.specifics);
}

// TODO: Nudge SyncScheduler.
if (CanCommitItems())
nudge_handler_->NudgeForCommit(type_);
}

void ModelTypeSyncWorkerImpl::OnCommitResponse(
Expand Down
5 changes: 5 additions & 0 deletions sync/engine/model_type_sync_worker_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "sync/engine/commit_contributor.h"
#include "sync/engine/model_type_sync_worker.h"
#include "sync/engine/non_blocking_sync_common.h"
#include "sync/engine/nudge_handler.h"
#include "sync/engine/update_handler.h"
#include "sync/internal_api/public/base/model_type.h"
#include "sync/protocol/sync.pb.h"
Expand Down Expand Up @@ -52,6 +53,7 @@ class SYNC_EXPORT ModelTypeSyncWorkerImpl : public UpdateHandler,
public:
ModelTypeSyncWorkerImpl(ModelType type,
const DataTypeState& initial_state,
NudgeHandler* nudge_handler,
scoped_ptr<ModelTypeSyncProxy> type_sync_proxy);
virtual ~ModelTypeSyncWorkerImpl();

Expand Down Expand Up @@ -109,6 +111,9 @@ class SYNC_EXPORT ModelTypeSyncWorkerImpl : public UpdateHandler,
// This is NULL when no proxy is connected..
scoped_ptr<ModelTypeSyncProxy> type_sync_proxy_;

// Interface used to access and send nudges to the sync scheduler. Not owned.
NudgeHandler* nudge_handler_;

// A map of per-entity information known to this object.
//
// When commits are pending, their information is stored here. This
Expand Down
32 changes: 31 additions & 1 deletion sync/engine/model_type_sync_worker_impl_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "sync/sessions/status_controller.h"
#include "sync/syncable/syncable_util.h"
#include "sync/test/engine/mock_model_type_sync_proxy.h"
#include "sync/test/engine/mock_nudge_handler.h"
#include "sync/test/engine/single_type_mock_server.h"

#include "testing/gtest/include/gtest/gtest.h"
Expand Down Expand Up @@ -132,6 +133,12 @@ class ModelTypeSyncWorkerImplTest : public ::testing::Test {
CommitResponseData GetCommitResponseOnModelThread(
const std::string& tag) const;

// Returns the number of commit nudges sent to the mock nudge handler.
int GetNumCommitNudges() const;

// Returns the number of initial sync nudges sent to the mock nudge handler.
int GetNumInitialDownloadNudges() const;

// Helpers for building various messages and structures.
static std::string GenerateTagHash(const std::string& tag);
static sync_pb::EntitySpecifics GenerateSpecifics(const std::string& tag,
Expand All @@ -149,6 +156,10 @@ class ModelTypeSyncWorkerImplTest : public ::testing::Test {
// a single UpdateHandler and CommitContributor pair. In this test
// harness, the |worker_| is both of them.
SingleTypeMockServer mock_server_;

// A mock to track the number of times the ModelTypeSyncWorker requests to
// sync.
MockNudgeHandler mock_nudge_handler_;
};

ModelTypeSyncWorkerImplTest::ModelTypeSyncWorkerImplTest()
Expand Down Expand Up @@ -178,6 +189,8 @@ void ModelTypeSyncWorkerImplTest::NormalInitialize() {
initial_state.initial_sync_done = true;

InitializeWithState(initial_state);

mock_nudge_handler_.ClearCounters();
}

void ModelTypeSyncWorkerImplTest::InitializeWithState(
Expand All @@ -188,7 +201,8 @@ void ModelTypeSyncWorkerImplTest::InitializeWithState(
mock_type_sync_proxy_ = new MockModelTypeSyncProxy();
scoped_ptr<ModelTypeSyncProxy> proxy(mock_type_sync_proxy_);

worker_.reset(new ModelTypeSyncWorkerImpl(kModelType, state, proxy.Pass()));
worker_.reset(new ModelTypeSyncWorkerImpl(
kModelType, state, &mock_nudge_handler_, proxy.Pass()));
}

void ModelTypeSyncWorkerImplTest::CommitRequest(const std::string& name,
Expand Down Expand Up @@ -379,6 +393,14 @@ CommitResponseData ModelTypeSyncWorkerImplTest::GetCommitResponseOnModelThread(
return mock_type_sync_proxy_->GetCommitResponse(tag_hash);
}

int ModelTypeSyncWorkerImplTest::GetNumCommitNudges() const {
return mock_nudge_handler_.GetNumCommitNudges();
}

int ModelTypeSyncWorkerImplTest::GetNumInitialDownloadNudges() const {
return mock_nudge_handler_.GetNumInitialDownloadNudges();
}

std::string ModelTypeSyncWorkerImplTest::GenerateTagHash(
const std::string& tag) {
const std::string& client_tag_hash =
Expand Down Expand Up @@ -412,6 +434,8 @@ TEST_F(ModelTypeSyncWorkerImplTest, SimpleCommit) {

CommitRequest("tag1", "value1");

EXPECT_EQ(1, GetNumCommitNudges());

ASSERT_TRUE(WillCommit());
DoSuccessfulCommit();

Expand Down Expand Up @@ -454,6 +478,7 @@ TEST_F(ModelTypeSyncWorkerImplTest, SimpleDelete) {
// We can't delete an entity that was never committed.
// Step 1 is to create and commit a new entity.
CommitRequest("tag1", "value1");
EXPECT_EQ(1, GetNumCommitNudges());
ASSERT_TRUE(WillCommit());
DoSuccessfulCommit();

Expand Down Expand Up @@ -502,16 +527,19 @@ TEST_F(ModelTypeSyncWorkerImplTest, NoDeleteUncommitted) {
// Request the commit of a new, never-before-seen item.
CommitRequest("tag1", "value1");
EXPECT_TRUE(WillCommit());
EXPECT_EQ(1, GetNumCommitNudges());

// Request a deletion of that item before we've had a chance to commit it.
DeleteRequest("tag1");
EXPECT_FALSE(WillCommit());
EXPECT_EQ(2, GetNumCommitNudges());
}

// Verifies the sending of an "initial sync done" signal.
TEST_F(ModelTypeSyncWorkerImplTest, SendInitialSyncDone) {
FirstInitialize(); // Initialize with no saved sync state.
EXPECT_EQ(0U, GetNumModelThreadUpdateResponses());
EXPECT_EQ(1, GetNumInitialDownloadNudges());

// Receive an update response that contains only the type root node.
TriggerTypeRootUpdateFromServer();
Expand All @@ -538,6 +566,7 @@ TEST_F(ModelTypeSyncWorkerImplTest, TwoNewItemsCommittedSeparately) {

// Commit the first of two entities.
CommitRequest("tag1", "value1");
EXPECT_EQ(1, GetNumCommitNudges());
ASSERT_TRUE(WillCommit());
DoSuccessfulCommit();
ASSERT_EQ(1U, GetNumCommitMessagesOnServer());
Expand All @@ -548,6 +577,7 @@ TEST_F(ModelTypeSyncWorkerImplTest, TwoNewItemsCommittedSeparately) {

// Commit the second of two entities.
CommitRequest("tag2", "value2");
EXPECT_EQ(2, GetNumCommitNudges());
ASSERT_TRUE(WillCommit());
DoSuccessfulCommit();
ASSERT_EQ(2U, GetNumCommitMessagesOnServer());
Expand Down
15 changes: 15 additions & 0 deletions sync/engine/nudge_handler.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Copyright 2014 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 "sync/engine/nudge_handler.h"

namespace syncer {

NudgeHandler::NudgeHandler() {
}

NudgeHandler::~NudgeHandler() {
}

} // namespace syncer
26 changes: 26 additions & 0 deletions sync/engine/nudge_handler.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// Copyright 2014 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.

#ifndef SYNC_ENGINE_NUDGE_HANDLER_H_
#define SYNC_ENGINE_NUDGE_HANDLER_H_

#include "base/compiler_specific.h"
#include "sync/base/sync_export.h"
#include "sync/internal_api/public/base/model_type.h"

namespace syncer {

class SYNC_EXPORT_PRIVATE NudgeHandler {
public:
NudgeHandler();
virtual ~NudgeHandler();

virtual void NudgeForInitialDownload(syncer::ModelType type) = 0;
virtual void NudgeForCommit(syncer::ModelType type) = 0;
virtual void NudgeForRefresh(syncer::ModelType type) = 0;
};

} // namespace syncer

#endif // SYNC_ENGINE_NUDGE_HANDLER_H_
5 changes: 4 additions & 1 deletion sync/engine/sync_scheduler_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "sync/test/callback_counter.h"
#include "sync/test/engine/fake_model_worker.h"
#include "sync/test/engine/mock_connection_manager.h"
#include "sync/test/engine/mock_nudge_handler.h"
#include "sync/test/engine/test_directory_setter_upper.h"
#include "sync/test/mock_invalidation.h"
#include "sync/util/extensions_activity.h"
Expand Down Expand Up @@ -131,7 +132,8 @@ class SyncSchedulerTest : public testing::Test {
&cancelation_signal_));
connection_->SetServerReachable();

model_type_registry_.reset(new ModelTypeRegistry(workers_, directory()));
model_type_registry_.reset(
new ModelTypeRegistry(workers_, directory(), &mock_nudge_handler_));

context_.reset(new SyncSessionContext(
connection_.get(), directory(),
Expand Down Expand Up @@ -240,6 +242,7 @@ class SyncSchedulerTest : public testing::Test {
scoped_ptr<ModelTypeRegistry> model_type_registry_;
scoped_ptr<SyncSessionContext> context_;
scoped_ptr<SyncSchedulerImpl> scheduler_;
MockNudgeHandler mock_nudge_handler_;
MockSyncer* syncer_;
MockDelayProvider* delay_;
std::vector<scoped_refptr<ModelSafeWorker> > workers_;
Expand Down
5 changes: 4 additions & 1 deletion sync/engine/syncer_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
#include "sync/syncable/syncable_write_transaction.h"
#include "sync/test/engine/fake_model_worker.h"
#include "sync/test/engine/mock_connection_manager.h"
#include "sync/test/engine/mock_nudge_handler.h"
#include "sync/test/engine/test_directory_setter_upper.h"
#include "sync/test/engine/test_id_factory.h"
#include "sync/test/engine/test_syncable_utils.h"
Expand Down Expand Up @@ -294,7 +295,8 @@ class SyncerTest : public testing::Test,
ModelSafeRoutingInfo routing_info;
GetModelSafeRoutingInfo(&routing_info);

model_type_registry_.reset(new ModelTypeRegistry(workers_, directory()));
model_type_registry_.reset(
new ModelTypeRegistry(workers_, directory(), &mock_nudge_handler_));
model_type_registry_->RegisterDirectoryTypeDebugInfoObserver(
&debug_info_cache_);

Expand Down Expand Up @@ -584,6 +586,7 @@ class SyncerTest : public testing::Test,

scoped_ptr<SyncSession> session_;
TypeDebugInfoCache debug_info_cache_;
MockNudgeHandler mock_nudge_handler_;
scoped_ptr<ModelTypeRegistry> model_type_registry_;
scoped_ptr<SyncSessionContext> context_;
bool saw_syncer_event_;
Expand Down
29 changes: 24 additions & 5 deletions sync/internal_api/sync_context_proxy_impl_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
#include "sync/internal_api/public/sync_context.h"
#include "sync/internal_api/sync_context_proxy_impl.h"
#include "sync/sessions/model_type_registry.h"
#include "sync/test/engine/mock_nudge_handler.h"
#include "sync/test/engine/test_directory_setter_upper.h"
#include "testing/gtest/include/gtest/gtest.h"

namespace syncer {
Expand All @@ -19,22 +21,39 @@ class SyncContextProxyImplTest : public ::testing::Test {
public:
SyncContextProxyImplTest()
: sync_task_runner_(base::ThreadTaskRunnerHandle::Get()),
type_task_runner_(base::ThreadTaskRunnerHandle::Get()),
registry_(new ModelTypeRegistry()),
context_proxy_(sync_task_runner_, registry_->AsWeakPtr()) {}
type_task_runner_(base::ThreadTaskRunnerHandle::Get()) {}

virtual void SetUp() {
dir_maker_.SetUp();
registry_.reset(new ModelTypeRegistry(
workers_, dir_maker_.directory(), &nudge_handler_));
context_proxy_.reset(
new SyncContextProxyImpl(sync_task_runner_, registry_->AsWeakPtr()));
}

virtual void TearDown() {
context_proxy_.reset();
registry_.reset();
dir_maker_.TearDown();
}

// The sync thread could be shut down at any time without warning. This
// function simulates such an event.
void DisableSync() { registry_.reset(); }

scoped_ptr<SyncContextProxy> GetProxy() { return context_proxy_.Clone(); }
scoped_ptr<SyncContextProxy> GetProxy() { return context_proxy_->Clone(); }

private:
base::MessageLoop loop_;
scoped_refptr<base::SequencedTaskRunner> sync_task_runner_;
scoped_refptr<base::SequencedTaskRunner> type_task_runner_;

std::vector<scoped_refptr<ModelSafeWorker> > workers_;
TestDirectorySetterUpper dir_maker_;
MockNudgeHandler nudge_handler_;
scoped_ptr<ModelTypeRegistry> registry_;
SyncContextProxyImpl context_proxy_;

scoped_ptr<SyncContextProxyImpl> context_proxy_;
};

// Try to connect a type to a SyncContext that has already shut down.
Expand Down
18 changes: 17 additions & 1 deletion sync/internal_api/sync_manager_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -396,7 +396,7 @@ void SyncManagerImpl::Init(
DVLOG(1) << "Setting invalidator client ID: " << invalidator_client_id;
allstatus_.SetInvalidatorClientId(invalidator_client_id);

model_type_registry_.reset(new ModelTypeRegistry(workers, directory()));
model_type_registry_.reset(new ModelTypeRegistry(workers, directory(), this));

// Bind the SyncContext WeakPtr to this thread. This helps us crash earlier
// if the pointer is misused in debug mode.
Expand Down Expand Up @@ -900,6 +900,22 @@ void SyncManagerImpl::RequestNudgeForDataTypes(
nudge_location);
}

void SyncManagerImpl::NudgeForInitialDownload(syncer::ModelType type) {
// TODO(rlarocque): Initial downloads should have a separate nudge type.
DCHECK(thread_checker_.CalledOnValidThread());
RefreshTypes(ModelTypeSet(type));
}

void SyncManagerImpl::NudgeForCommit(syncer::ModelType type) {
DCHECK(thread_checker_.CalledOnValidThread());
RequestNudgeForDataTypes(FROM_HERE, ModelTypeSet(type));
}

void SyncManagerImpl::NudgeForRefresh(syncer::ModelType type) {
DCHECK(thread_checker_.CalledOnValidThread());
RefreshTypes(ModelTypeSet(type));
}

void SyncManagerImpl::OnSyncCycleEvent(const SyncCycleEvent& event) {
DCHECK(thread_checker_.CalledOnValidThread());
// Only send an event if this is due to a cycle ending and this cycle
Expand Down
Loading

0 comments on commit 1eed4f1

Please sign in to comment.