[GCM] Add support for canary connection attempts

google_apis/gcm/engine/connection_factory_impl.cc

 // Copyright (c) 2013 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 "google_apis/gcm/engine/connection_factory_impl.h"
 
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/sparse_histogram.h"
 #include "google_apis/gcm/engine/connection_handler_impl.h"
@@ skipping 36 matching lines @@
     net::NetLog* net_log)
   : mcs_endpoints_(mcs_endpoints),
     next_endpoint_(0),
     last_successful_endpoint_(0),
     backoff_policy_(backoff_policy),
     network_session_(network_session),
     bound_net_log_(
         net::BoundNetLog::Make(net_log, net::NetLog::SOURCE_SOCKET)),
     pac_request_(NULL),
     connecting_(false),
+    waiting_for_backoff_(false),
     logging_in_(false),
     weak_ptr_factory_(this) {
   DCHECK_GE(mcs_endpoints_.size(), 1U);
 }
 
 ConnectionFactoryImpl::~ConnectionFactoryImpl() {
   if (pac_request_) {
     network_session_->proxy_service()->CancelPacRequest(pac_request_);
     pac_request_ = NULL;
   }
 }
 
 void ConnectionFactoryImpl::Initialize(
     const BuildLoginRequestCallback& request_builder,
     const ConnectionHandler::ProtoReceivedCallback& read_callback,
     const ConnectionHandler::ProtoSentCallback& write_callback) {
   DCHECK(!connection_handler_);
 
   previous_backoff_ = CreateBackoffEntry(&backoff_policy_);
   backoff_entry_ = CreateBackoffEntry(&backoff_policy_);
   request_builder_ = request_builder;
 
   net::NetworkChangeNotifier::AddIPAddressObserver(this);
   net::NetworkChangeNotifier::AddConnectionTypeObserver(this);
-  connection_handler_.reset(
-      new ConnectionHandlerImpl(
-          base::TimeDelta::FromMilliseconds(kReadTimeoutMs),
-          read_callback,
-          write_callback,
-          base::Bind(&ConnectionFactoryImpl::ConnectionHandlerCallback,
-                     weak_ptr_factory_.GetWeakPtr())));
+  connection_handler_ = CreateConnectionHandler(
+      base::TimeDelta::FromMilliseconds(kReadTimeoutMs),
+      read_callback,
+      write_callback,
+      base::Bind(&ConnectionFactoryImpl::ConnectionHandlerCallback,
+                 weak_ptr_factory_.GetWeakPtr())).Pass();
 }
 
 ConnectionHandler* ConnectionFactoryImpl::GetConnectionHandler() const {
   return connection_handler_.get();
 }
 
 void ConnectionFactoryImpl::Connect() {
   DCHECK(connection_handler_);
 
-  connecting_ = true;
+  if (connecting_ || waiting_for_backoff_)

[jianli, 2014/03/28 04:26:13]

Now it becomes a bit harder to follow all the logic with different flags plus
checking IsEndpointReachable. Can we manage all these through some kind of enum
states, like: NOT_CONNECTED, CONNECTING, CONNECTED, BACKING_OFF.

[Nicolas Zea, 2014/03/28 17:53:42]

The problem is that, because of canaries, these two states are independent of
each other. The factory can be connecting while a backoff expiration is still
pending. And similarly the canary can succeed, making IsEndpointReachable true,
but the backoff is still pending (and its request will be silently dropped when
it finishes). So trying to combine them into a single variable seems more
error-prone to me. 

+    return;  // Connection attempt already in progress or pending.
+
+  if (IsEndpointReachable())
+    return;  // Already connected.
+
+  ConnectWithBackoff();
+}
+
+void ConnectionFactoryImpl::ConnectWithBackoff() {
+  // If a canary managed to connect while a backoff expiration was pending,
+  // just cleanup the internal state.
+  if (connecting_ || IsEndpointReachable()) {
+    waiting_for_backoff_ = false;
+    return;
+  }
+
   if (backoff_entry_->ShouldRejectRequest()) {
     DVLOG(1) << "Delaying MCS endpoint connection for "
              << backoff_entry_->GetTimeUntilRelease().InMilliseconds()
              << " milliseconds.";
+    waiting_for_backoff_ = true;
     base::MessageLoop::current()->PostDelayedTask(
         FROM_HERE,
-        base::Bind(&ConnectionFactoryImpl::Connect,
+        base::Bind(&ConnectionFactoryImpl::ConnectWithBackoff,
                    weak_ptr_factory_.GetWeakPtr()),
         backoff_entry_->GetTimeUntilRelease());
     return;
   }
 
   DVLOG(1) << "Attempting connection to MCS endpoint.";
+  waiting_for_backoff_ = false;
   ConnectImpl();
 }
 
 bool ConnectionFactoryImpl::IsEndpointReachable() const {
-  return connection_handler_ &&
-      connection_handler_->CanSendMessage() &&
-      !connecting_;
+  return connection_handler_ && connection_handler_->CanSendMessage();
 }
 
 void ConnectionFactoryImpl::SignalConnectionReset(
     ConnectionResetReason reason) {
   // A failure can trigger multiple resets, so no need to do anything if a
   // connection is already in progress.
-  if (connecting_)
+  if (connecting_) {
+    DVLOG(1) << "Connection in progress, ignoring reset.";
     return;
+  }
 
   UMA_HISTOGRAM_ENUMERATION("GCM.ConnectionResetReason",
                             reason,
                             CONNECTION_RESET_COUNT);
   if (!last_login_time_.is_null()) {
     UMA_HISTOGRAM_CUSTOM_TIMES("GCM.ConnectionUpTime",
                                NowTicks() - last_login_time_,
                                base::TimeDelta::FromSeconds(1),
                                base::TimeDelta::FromHours(24),
                                50);
     // |last_login_time_| will be reset below, before attempting the new
     // connection.
   }
 
   CloseSocket();
+  DCHECK(!IsEndpointReachable());
+
+  // Network changes get special treatment as they can trigger a one-off canary
+  // request that bypasses backoff (but does nothing if a connection is in
+  // progress). Other connection reset events can be ignored as a connection
+  // is already awaiting backoff expiration.
+  if (waiting_for_backoff_ && reason != NETWORK_CHANGE) {
+    DVLOG(1) << "Backoff expiration pending, ignoring reset.";
+    return;
+  }
 
   if (logging_in_) {
     // Failures prior to login completion just reuse the existing backoff entry.
     logging_in_ = false;
     backoff_entry_->InformOfRequest(false);
   } else if (reason == LOGIN_FAILURE ||
              ShouldRestorePreviousBackoff(last_login_time_, NowTicks())) {
     // Failures due to login, or within the reset window of a login, restore
     // the backoff entry that was saved off at login completion time.
     backoff_entry_.swap(previous_backoff_);
     backoff_entry_->InformOfRequest(false);
+  } else if (reason == NETWORK_CHANGE) {
+    ConnectImpl();  // Canary attempts bypass backoff without resetting it.
+    return;
   } else {
     // We shouldn't be in backoff in thise case.
     DCHECK_EQ(0, backoff_entry_->failure_count());
   }
+  DCHECK(!connecting_);
+  DCHECK(!waiting_for_backoff_);
 
   // At this point the last login time has been consumed or deemed irrelevant,
   // reset it.
   last_login_time_ = base::TimeTicks();
 
   Connect();
 }
 
 base::TimeTicks ConnectionFactoryImpl::NextRetryAttempt() const {
   if (!backoff_entry_)
     return base::TimeTicks();
   return backoff_entry_->GetReleaseTime();
 }
 
 void ConnectionFactoryImpl::OnConnectionTypeChanged(
     net::NetworkChangeNotifier::ConnectionType type) {
   if (type == net::NetworkChangeNotifier::CONNECTION_NONE)
     return;
 
-  // TODO(zea): implement different backoff/retry policies based on connection
-  // type.
-  DVLOG(1) << "Connection type changed to " << type << ", resetting backoff.";
-  backoff_entry_->Reset();
-  // Connect(..) should be retrying with backoff already if a connection is
-  // necessary, so no need to call again.
+  DVLOG(1) << "Connection type changed to " << type << ", reconnecting.";
+
+  // The connection may have been silently dropped, attempt to reconnect.
+  SignalConnectionReset(NETWORK_CHANGE);
 }
 
 void ConnectionFactoryImpl::OnIPAddressChanged() {
-  DVLOG(1) << "IP Address changed, resetting backoff.";
-  backoff_entry_->Reset();
-  // Connect(..) should be retrying with backoff already if a connection is
-  // necessary, so no need to call again.
+  DVLOG(1) << "IP Address changed, reconnecting.";
+
+  // The connection may have been silently dropped, attempt to reconnect.
+  SignalConnectionReset(NETWORK_CHANGE);
 }
 
 GURL ConnectionFactoryImpl::GetCurrentEndpoint() const {
   // Note that IsEndpointReachable() returns false anytime connecting_ is true,
   // so while connecting this always uses |next_endpoint_|.
   if (IsEndpointReachable())
     return mcs_endpoints_[last_successful_endpoint_];
   return mcs_endpoints_[next_endpoint_];
 }
 
 void ConnectionFactoryImpl::ConnectImpl() {
-  DCHECK(connecting_);
+  DCHECK(!IsEndpointReachable());
   DCHECK(!socket_handle_.socket());
 
+  connecting_ = true;
   int status = network_session_->proxy_service()->ResolveProxy(
       GetCurrentEndpoint(),
       &proxy_info_,
       base::Bind(&ConnectionFactoryImpl::OnProxyResolveDone,
                  weak_ptr_factory_.GetWeakPtr()),
       &pac_request_,
       bound_net_log_);
   if (status != net::ERR_IO_PENDING)
     OnProxyResolveDone(status);
 }
 
 void ConnectionFactoryImpl::InitHandler() {
   // May be null in tests.
   mcs_proto::LoginRequest login_request;
   if (!request_builder_.is_null()) {
     request_builder_.Run(&login_request);
     DCHECK(login_request.IsInitialized());
   }
 
   connection_handler_->Init(login_request, socket_handle_.socket());
 }
 
 scoped_ptr<net::BackoffEntry> ConnectionFactoryImpl::CreateBackoffEntry(
     const net::BackoffEntry::Policy* const policy) {
   return scoped_ptr<net::BackoffEntry>(new net::BackoffEntry(policy));
 }
 
+scoped_ptr<ConnectionHandler> ConnectionFactoryImpl::CreateConnectionHandler(
+    base::TimeDelta read_timeout,
+    const ConnectionHandler::ProtoReceivedCallback& read_callback,
+    const ConnectionHandler::ProtoSentCallback& write_callback,
+    const ConnectionHandler::ConnectionChangedCallback& connection_callback) {
+  return make_scoped_ptr<ConnectionHandler>(
+      new ConnectionHandlerImpl(read_timeout,
+                                read_callback,
+                                write_callback,
+                                connection_callback));
+}
+
 base::TimeTicks ConnectionFactoryImpl::NowTicks() {
   return base::TimeTicks::Now();
 }
 
 void ConnectionFactoryImpl::OnConnectDone(int result) {
   if (result != net::OK) {
     // If the connection fails, try another proxy.
     result = ReconsiderProxyAfterError(result);
     // ReconsiderProxyAfterError either returns an error (in which case it is
     // not reconsidering a proxy) or returns ERR_IO_PENDING if it is considering
     // another proxy.
     DCHECK_NE(result, net::OK);
     if (result == net::ERR_IO_PENDING)
       return;  // Proxy reconsideration pending. Return.
     LOG(ERROR) << "Failed to connect to MCS endpoint with error " << result;
     UMA_HISTOGRAM_BOOLEAN("GCM.ConnectionSuccessRate", false);
     CloseSocket();
     backoff_entry_->InformOfRequest(false);
     UMA_HISTOGRAM_SPARSE_SLOWLY("GCM.ConnectionFailureErrorCode", result);
 
     // If there are other endpoints available, use the next endpoint on the
     // subsequent retry.
     next_endpoint_++;
     if (next_endpoint_ >= mcs_endpoints_.size())
       next_endpoint_ = 0;
+    connecting_ = false;
     Connect();
     return;
   }
 
   UMA_HISTOGRAM_BOOLEAN("GCM.ConnectionSuccessRate", true);
   UMA_HISTOGRAM_COUNTS("GCM.ConnectionEndpoint", next_endpoint_);
   UMA_HISTOGRAM_BOOLEAN("GCM.ConnectedViaProxy",
                         !(proxy_info_.is_empty() || proxy_info_.is_direct()));
   ReportSuccessfulProxyConnection();
 
@@ skipping 172 matching lines @@
   // the destroyed socket.
   if (connection_handler_)
     connection_handler_->Reset();
 
   if (socket_handle_.socket() && socket_handle_.socket()->IsConnected())
     socket_handle_.socket()->Disconnect();
   socket_handle_.Reset();
 }
 
 }  // namespace gcm