[GCM] Add support for canary connection attempts

google_apis/gcm/engine/connection_factory_impl.cc

Index: google_apis/gcm/engine/connection_factory_impl.cc
diff --git a/google_apis/gcm/engine/connection_factory_impl.cc b/google_apis/gcm/engine/connection_factory_impl.cc
index deb810264171bab7fe326114fcc3dec19249fc90..9dff1f69c8db392b62a6d2cf23ea8759d3c2fa73 100644
--- a/google_apis/gcm/engine/connection_factory_impl.cc
+++ b/google_apis/gcm/engine/connection_factory_impl.cc
@@ -54,6 +54,7 @@ ConnectionFactoryImpl::ConnectionFactoryImpl(
         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);
@@ -78,13 +79,12 @@ void ConnectionFactoryImpl::Initialize(
 
   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 {
@@ -94,35 +94,53 @@ ConnectionHandler* ConnectionFactoryImpl::GetConnectionHandler() const {
 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,
@@ -138,6 +156,16 @@ void ConnectionFactoryImpl::SignalConnectionReset(
   }
 
   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.
@@ -149,10 +177,15 @@ void ConnectionFactoryImpl::SignalConnectionReset(
     // 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.
@@ -172,19 +205,17 @@ void ConnectionFactoryImpl::OnConnectionTypeChanged(
   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 {
@@ -196,9 +227,10 @@ GURL ConnectionFactoryImpl::GetCurrentEndpoint() const {
 }
 
 void ConnectionFactoryImpl::ConnectImpl() {
-  DCHECK(connecting_);
+  DCHECK(!IsEndpointReachable());
   DCHECK(!socket_handle_.socket());
 
+  connecting_ = true;
   int status = network_session_->proxy_service()->ResolveProxy(
       GetCurrentEndpoint(),
       &proxy_info_,
@@ -226,6 +258,18 @@ scoped_ptr<net::BackoffEntry> ConnectionFactoryImpl::CreateBackoffEntry(
   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();
 }
@@ -251,6 +295,7 @@ void ConnectionFactoryImpl::OnConnectDone(int result) {
     next_endpoint_++;
     if (next_endpoint_ >= mcs_endpoints_.size())
       next_endpoint_ = 0;
+    connecting_ = false;
     Connect();
     return;
   }