From 711317113397ea92c6a5978f6fb409b43bd96851 Mon Sep 17 00:00:00 2001 From: "rlarocque@chromium.org" Date: Wed, 11 Jan 2012 21:27:58 +0000 Subject: [PATCH] Detect sync server communication errors This is part 2 of 3 in a series of patches to remove the unnecessary SYNC_CYCLE_CONTINUATION sync cycle that follows every commit. This patch adds infrastructure to report errors involving communication with the sync server back to the SyncSession and SyncScheduler. It modifies SyncerProtoUtil::PostClientToServerMessage to have it return an error code describing why it failed. This function's callers (all of which are SyncerCommands) use the infrastructure added in the first patch of this series to return the error value to SyncShare(). From there, the return values are placed into the session's StatusController object. The method SyncSession::Succeeded() method provides an easy way to determine if the anything went wrong with the current sync session, and its response is based on the stored return values. This change has no effect on the client's behaviour. Although it gives the client access to lots more information, the client does not use it for any decision making. BUG=94670 TEST= Review URL: http://codereview.chromium.org/9158004 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@117285 0039d316-1c4b-4281-b951-d872f2087c98 --- .../browser/sync/engine/clear_data_command.cc | 6 +-- .../sync/engine/download_updates_command.cc | 8 +-- chrome/browser/sync/engine/syncer.cc | 12 +++-- .../browser/sync/engine/syncer_proto_util.cc | 54 +++++++++++++++---- .../browser/sync/engine/syncer_proto_util.h | 5 +- chrome/browser/sync/sessions/session_state.cc | 7 ++- chrome/browser/sync/sessions/session_state.h | 7 ++- .../sync/sessions/status_controller.cc | 16 +++++- .../browser/sync/sessions/status_controller.h | 5 +- .../sessions/status_controller_unittest.cc | 12 ++++- chrome/browser/sync/sessions/sync_session.cc | 22 +++++++- chrome/browser/sync/sessions/sync_session.h | 18 ++++++- 12 files changed, 140 insertions(+), 32 deletions(-) diff --git a/chrome/browser/sync/engine/clear_data_command.cc b/chrome/browser/sync/engine/clear_data_command.cc index 248fc8326d660a..f5cd39dc647004 100644 --- a/chrome/browser/sync/engine/clear_data_command.cc +++ b/chrome/browser/sync/engine/clear_data_command.cc @@ -47,7 +47,7 @@ SyncerError ClearDataCommand::ExecuteImpl(SyncSession* session) { DVLOG(1) << "Clearing server data"; - bool ok = SyncerProtoUtil::PostClientToServerMessage( + SyncerError result = SyncerProtoUtil::PostClientToServerMessage( client_to_server_message, &client_to_server_response, session); @@ -61,7 +61,7 @@ SyncerError ClearDataCommand::ExecuteImpl(SyncSession* session) { // See also: crbug.com/71616. // // Clear pending indicates that the server has received our clear message - if (!ok || !client_to_server_response.has_error_code() || + if (result != SYNCER_OK || !client_to_server_response.has_error_code() || client_to_server_response.error_code() != sync_pb::SyncEnums::SUCCESS) { // On failure, subsequent requests to the server will cause it to attempt // to resume the clear. The client will handle disabling of sync in @@ -71,7 +71,7 @@ SyncerError ClearDataCommand::ExecuteImpl(SyncSession* session) { LOG(ERROR) << "Error posting ClearData."; - return SYNCER_OK; + return result; } SyncEngineEvent event(SyncEngineEvent::CLEAR_SERVER_DATA_SUCCEEDED); diff --git a/chrome/browser/sync/engine/download_updates_command.cc b/chrome/browser/sync/engine/download_updates_command.cc index 8cee1b55dbfa4c..ee90cbe04f96eb 100644 --- a/chrome/browser/sync/engine/download_updates_command.cc +++ b/chrome/browser/sync/engine/download_updates_command.cc @@ -91,7 +91,7 @@ SyncerError DownloadUpdatesCommand::ExecuteImpl(SyncSession* session) { AppendClientDebugInfoIfNeeded(session, debug_info); - bool ok = SyncerProtoUtil::PostClientToServerMessage( + SyncerError result = SyncerProtoUtil::PostClientToServerMessage( client_to_server_message, &update_response, session); @@ -101,11 +101,11 @@ SyncerError DownloadUpdatesCommand::ExecuteImpl(SyncSession* session) { StatusController* status = session->mutable_status_controller(); status->set_updates_request_types(enabled_types); - if (!ok) { + if (result != SYNCER_OK) { status->increment_num_consecutive_errors(); status->mutable_updates_response()->Clear(); LOG(ERROR) << "PostClientToServerMessage() failed during GetUpdates"; - return SYNCER_OK; // TODO(rlarocque): Return an error here. + return result; } status->mutable_updates_response()->CopyFrom(update_response); @@ -115,7 +115,7 @@ SyncerError DownloadUpdatesCommand::ExecuteImpl(SyncSession* session) { << " updates and indicated " << update_response.get_updates().changes_remaining() << " updates left on server."; - return SYNCER_OK; + return result; } void DownloadUpdatesCommand::AppendClientDebugInfoIfNeeded( diff --git a/chrome/browser/sync/engine/syncer.cc b/chrome/browser/sync/engine/syncer.cc index f6490c58312331..b906062e0e93c3 100644 --- a/chrome/browser/sync/engine/syncer.cc +++ b/chrome/browser/sync/engine/syncer.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// 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. @@ -144,7 +144,8 @@ void Syncer::SyncShare(sessions::SyncSession* session, } case DOWNLOAD_UPDATES: { DownloadUpdatesCommand download_updates; - download_updates.Execute(session); + session->mutable_status_controller()->set_last_download_updates_result( + download_updates.Execute(session)); next_step = PROCESS_CLIENT_COMMAND; break; } @@ -222,14 +223,17 @@ void Syncer::SyncShare(sessions::SyncSession* session, } case POST_COMMIT_MESSAGE: { PostCommitMessageCommand post_commit_command; - post_commit_command.Execute(session); + session->mutable_status_controller()->set_last_post_commit_result( + post_commit_command.Execute(session)); next_step = PROCESS_COMMIT_RESPONSE; break; } case PROCESS_COMMIT_RESPONSE: { session->mutable_status_controller()->reset_num_conflicting_commits(); ProcessCommitResponseCommand process_response_command; - process_response_command.Execute(session); + session->mutable_status_controller()-> + set_last_process_commit_response_result( + process_response_command.Execute(session)); next_step = BUILD_AND_PROCESS_CONFLICT_SETS; break; } diff --git a/chrome/browser/sync/engine/syncer_proto_util.cc b/chrome/browser/sync/engine/syncer_proto_util.cc index f5169261f8c12c..7810c6000a8059 100644 --- a/chrome/browser/sync/engine/syncer_proto_util.cc +++ b/chrome/browser/sync/engine/syncer_proto_util.cc @@ -80,6 +80,28 @@ void LogResponseProfilingData(const ClientToServerResponse& response) { } } +SyncerError ServerConnectionErrorAsSyncerError( + const HttpResponse::ServerConnectionCode server_status) { + switch (server_status) { + case HttpResponse::CONNECTION_UNAVAILABLE: + return NETWORK_CONNECTION_UNAVAILABLE; + case HttpResponse::IO_ERROR: + return NETWORK_IO_ERROR; + case HttpResponse::SYNC_SERVER_ERROR: + // FIXME what does this mean? + return SYNC_SERVER_ERROR; + case HttpResponse::SYNC_AUTH_ERROR: + return SYNC_AUTH_ERROR; + case HttpResponse::RETRY: + return SERVER_RETURN_TRANSIENT_ERROR; + case HttpResponse::SERVER_CONNECTION_OK: + case HttpResponse::NONE: + default: + NOTREACHED(); + return UNSET; + } +} + } // namespace // static @@ -303,7 +325,7 @@ browser_sync::SyncProtocolError ConvertLegacyErrorCodeToNewError( } // namespace // static -bool SyncerProtoUtil::PostClientToServerMessage( +SyncerError SyncerProtoUtil::PostClientToServerMessage( const ClientToServerMessage& msg, ClientToServerResponse* response, SyncSession* session) { @@ -317,11 +339,20 @@ bool SyncerProtoUtil::PostClientToServerMessage( ScopedDirLookup dir(session->context()->directory_manager(), session->context()->account_name()); if (!dir.good()) - return false; + return DIRECTORY_LOOKUP_FAILED; if (!PostAndProcessHeaders(session->context()->connection_manager(), session, - msg, response)) - return false; + msg, response)) { + // There was an error establishing communication with the server. + // We can not proceed beyond this point. + const browser_sync::HttpResponse::ServerConnectionCode server_status = + session->context()->connection_manager()->server_status(); + + DCHECK_NE(server_status, browser_sync::HttpResponse::NONE); + DCHECK_NE(server_status, browser_sync::HttpResponse::SERVER_CONNECTION_OK); + + return ServerConnectionErrorAsSyncerError(server_status); + } browser_sync::SyncProtocolError sync_protocol_error; @@ -352,28 +383,29 @@ bool SyncerProtoUtil::PostClientToServerMessage( case browser_sync::UNKNOWN_ERROR: LOG(WARNING) << "Sync protocol out-of-date. The server is using a more " << "recent version."; - return false; + return SERVER_RETURN_UNKNOWN_ERROR; case browser_sync::SYNC_SUCCESS: LogResponseProfilingData(*response); - return true; + return SYNCER_OK; case browser_sync::THROTTLED: LOG(WARNING) << "Client silenced by server."; HandleThrottleError(sync_protocol_error, base::TimeTicks::Now() + GetThrottleDelay(*response), session->context(), session->delegate()); - return false; + return SERVER_RETURN_THROTTLED; case browser_sync::TRANSIENT_ERROR: - return false; + return SERVER_RETURN_TRANSIENT_ERROR; case browser_sync::MIGRATION_DONE: HandleMigrationDoneResponse(response, session); - return false; + return SERVER_RETURN_MIGRATION_DONE; case browser_sync::CLEAR_PENDING: + return SERVER_RETURN_CLEAR_PENDING; case browser_sync::NOT_MY_BIRTHDAY: - return false; + return SERVER_RETURN_NOT_MY_BIRTHDAY; default: NOTREACHED(); - return false; + return UNSET; } } diff --git a/chrome/browser/sync/engine/syncer_proto_util.h b/chrome/browser/sync/engine/syncer_proto_util.h index 0b209a7d752ea7..ea22caed14eee3 100644 --- a/chrome/browser/sync/engine/syncer_proto_util.h +++ b/chrome/browser/sync/engine/syncer_proto_util.h @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// 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. @@ -10,6 +10,7 @@ #include "base/gtest_prod_util.h" #include "base/time.h" +#include "chrome/browser/sync/internal_api/includes/syncer_error.h" #include "chrome/browser/sync/sessions/sync_session.h" #include "chrome/browser/sync/syncable/blob.h" #include "chrome/browser/sync/syncable/model_type.h" @@ -42,7 +43,7 @@ class SyncerProtoUtil { // Returns true on success. Also handles store birthday verification: // session->status()->syncer_stuck_ is set true if the birthday is // incorrect. A false value will always be returned if birthday is bad. - static bool PostClientToServerMessage( + static SyncerError PostClientToServerMessage( const ClientToServerMessage& msg, sync_pb::ClientToServerResponse* response, sessions::SyncSession* session); diff --git a/chrome/browser/sync/sessions/session_state.cc b/chrome/browser/sync/sessions/session_state.cc index 19f880c0a6369d..fb7e5ea19a100e 100644 --- a/chrome/browser/sync/sessions/session_state.cc +++ b/chrome/browser/sync/sessions/session_state.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// 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. @@ -95,7 +95,10 @@ DictionaryValue* DownloadProgressMarkersToValue( ErrorCounters::ErrorCounters() : num_conflicting_commits(0), consecutive_transient_error_commits(0), - consecutive_errors(0) { + consecutive_errors(0), + last_download_updates_result(UNSET), + last_post_commit_result(UNSET), + last_process_commit_response_result(UNSET) { } DictionaryValue* ErrorCounters::ToValue() const { diff --git a/chrome/browser/sync/sessions/session_state.h b/chrome/browser/sync/sessions/session_state.h index 448ecc1cd28cee..78cccff668ccaa 100644 --- a/chrome/browser/sync/sessions/session_state.h +++ b/chrome/browser/sync/sessions/session_state.h @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// 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. @@ -105,6 +105,11 @@ struct ErrorCounters { // Any protocol errors that we received during this sync session. SyncProtocolError sync_protocol_error; + + // Records the most recent results of PostCommit and GetUpdates commands. + SyncerError last_download_updates_result; + SyncerError last_post_commit_result; + SyncerError last_process_commit_response_result; }; // Caller takes ownership of the returned dictionary. diff --git a/chrome/browser/sync/sessions/status_controller.cc b/chrome/browser/sync/sessions/status_controller.cc index bf9416d51d52d2..e359c5895d0c7c 100644 --- a/chrome/browser/sync/sessions/status_controller.cc +++ b/chrome/browser/sync/sessions/status_controller.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// 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. @@ -211,6 +211,20 @@ void StatusController::set_sync_protocol_error( shared_.error.mutate()->sync_protocol_error = error; } +void StatusController::set_last_download_updates_result( + const SyncerError result) { + shared_.error.mutate()->last_download_updates_result = result; +} + +void StatusController::set_last_post_commit_result(const SyncerError result) { + shared_.error.mutate()->last_post_commit_result = result; +} + +void StatusController::set_last_process_commit_response_result( + const SyncerError result) { + shared_.error.mutate()->last_process_commit_response_result = result; +} + void StatusController::set_commit_set(const OrderedCommitSet& commit_set) { DCHECK(!group_restriction_in_effect_); shared_.commit_set = commit_set; diff --git a/chrome/browser/sync/sessions/status_controller.h b/chrome/browser/sync/sessions/status_controller.h index 2509894f449dca..041ff8bcb458ea 100644 --- a/chrome/browser/sync/sessions/status_controller.h +++ b/chrome/browser/sync/sessions/status_controller.h @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// 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. @@ -228,6 +228,9 @@ class StatusController { void increment_num_local_overwrites(); void increment_num_server_overwrites(); void set_sync_protocol_error(const SyncProtocolError& error); + void set_last_download_updates_result(const SyncerError result); + void set_last_post_commit_result(const SyncerError result); + void set_last_process_commit_response_result(const SyncerError result); void set_commit_set(const OrderedCommitSet& commit_set); void update_conflict_sets_built(bool built); diff --git a/chrome/browser/sync/sessions/status_controller_unittest.cc b/chrome/browser/sync/sessions/status_controller_unittest.cc index 90e986004ba080..d39ee78889c0d7 100644 --- a/chrome/browser/sync/sessions/status_controller_unittest.cc +++ b/chrome/browser/sync/sessions/status_controller_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// 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. @@ -130,6 +130,16 @@ TEST_F(StatusControllerTest, ReadYourWrites) { status.set_syncer_stuck(true); EXPECT_TRUE(status.syncer_status().syncer_stuck); + status.set_last_download_updates_result(SYNCER_OK); + EXPECT_EQ(SYNCER_OK, status.error().last_download_updates_result); + + status.set_last_post_commit_result(SYNC_AUTH_ERROR); + EXPECT_EQ(SYNC_AUTH_ERROR, status.error().last_post_commit_result); + + status.set_last_process_commit_response_result(SYNC_SERVER_ERROR); + EXPECT_EQ(SYNC_SERVER_ERROR, + status.error().last_process_commit_response_result); + for (int i = 0; i < 14; i++) status.increment_num_successful_commits(); EXPECT_EQ(14, status.syncer_status().num_successful_commits); diff --git a/chrome/browser/sync/sessions/sync_session.cc b/chrome/browser/sync/sessions/sync_session.cc index 064fef6744e9d7..9b483d6d6d3159 100644 --- a/chrome/browser/sync/sessions/sync_session.cc +++ b/chrome/browser/sync/sessions/sync_session.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// 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. @@ -233,6 +233,26 @@ std::set return enabled_groups_with_verified_updates; } +namespace { +// Return true if the command in question was attempted and did not complete +// successfully. +// +bool IsError(SyncerError error) { + return error != UNSET && error != SYNCER_OK; +} +} // namespace + +bool SyncSession::Succeeded() const { + const bool download_updates_error = + IsError(status_controller_->error().last_download_updates_result); + const bool post_commit_error = + IsError(status_controller_->error().last_post_commit_result); + const bool process_commit_response_error = + IsError(status_controller_->error().last_process_commit_response_result); + return !download_updates_error + && !post_commit_error + && !process_commit_response_error; +} } // namespace sessions } // namespace browser_sync diff --git a/chrome/browser/sync/sessions/sync_session.h b/chrome/browser/sync/sessions/sync_session.h index abed10eac7fee2..bf7989c54e8a13 100644 --- a/chrome/browser/sync/sessions/sync_session.h +++ b/chrome/browser/sync/sessions/sync_session.h @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// 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. @@ -112,6 +112,22 @@ class SyncSession { // engine again. bool HasMoreToSync() const; + // Returns true if there we did not detect any errors in this session. + // + // There are many errors that could prevent a sync cycle from succeeding. + // These include invalid local state, inability to contact the server, + // inability to authenticate with the server, and server errors. What they + // have in common is that the we either need to take some action and then + // retry the sync cycle or, in the case of transient errors, retry after some + // backoff timer has expired. Most importantly, the SyncScheduler should not + // assume that the original action that triggered the sync cycle (ie. a nudge + // or a notification) has been properly serviced. + // + // This function also returns false if SyncShare has not been called on this + // session yet, or if ResetTransientState() has been called on this session + // since the last call to SyncShare. + bool Succeeded() const; + // Collects all state pertaining to how and why |s| originated and unions it // with corresponding state in |this|, leaving |s| unchanged. Allows |this| // to take on the responsibilities |s| had (e.g. certain data types) in the