[GCM] Add support for canary connection attempts

Canary connection attempts occur on network changes, and are capable of
bypassing backoff (without resetting it). As such they happen immediately
if no connection attempt is already in progress, but if they fail backoff
will be increased accordingly.

BUG=355126
R=asvitkine@chromium.org, jianli@chromium.org

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

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

google_apis/gcm/engine/connection_factory.h

@@ -30,6 +30,7 @@ class GCM_EXPORT ConnectionFactory {
     CLOSE_COMMAND,      // Received a close command.
     HEARTBEAT_FAILURE,  // Heartbeat was not acknowledged in time.
     SOCKET_FAILURE,     // net::Socket error.
+    NETWORK_CHANGE,     // NetworkChangeNotifier notified of a network change.
     // Count of total number of connection reset reasons. All new reset reasons
     // should be added above this line.
     CONNECTION_RESET_COUNT,

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_)
+    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;
   }

google_apis/gcm/engine/connection_factory_impl.h

@@ -75,6 +75,14 @@ class GCM_EXPORT ConnectionFactoryImpl :
   virtual scoped_ptr<net::BackoffEntry> CreateBackoffEntry(
       const net::BackoffEntry::Policy* const policy);
 
+  // Helper method for creating the connection handler.
+  // Virtual for testing.
+  virtual scoped_ptr<ConnectionHandler> CreateConnectionHandler(
+      base::TimeDelta read_timeout,
+      const ConnectionHandler::ProtoReceivedCallback& read_callback,
+      const ConnectionHandler::ProtoSentCallback& write_callback,
+      const ConnectionHandler::ConnectionChangedCallback& connection_callback);
+
   // Returns the current time in Ticks.
   // Virtual for testing.
   virtual base::TimeTicks NowTicks();
@@ -86,6 +94,10 @@ class GCM_EXPORT ConnectionFactoryImpl :
   void ConnectionHandlerCallback(int result);
 
  private:
+  // Helper method for checking backoff and triggering a connection as
+  // necessary.
+  void ConnectWithBackoff();
+
   // Proxy resolution and connection functions.
   void OnProxyResolveDone(int status);
   void OnProxyConnectDone(int status);
@@ -121,11 +133,14 @@ class GCM_EXPORT ConnectionFactoryImpl :
   // completion.
   scoped_ptr<net::BackoffEntry> previous_backoff_;
 
-  // Whether a connection attempt is currently in progress or we're in backoff
-  // waiting until the next connection attempt. |!connecting_| denotes
-  // steady state with an active connection.
+  // Whether a connection attempt is currently actively in progress.
   bool connecting_;
 
+  // Whether the client is waiting for backoff to finish before attempting to
+  // connect. Canary jobs are able to preempt connections pending backoff
+  // expiration.
+  bool waiting_for_backoff_;
+
   // Whether login successfully completed after the connection was established.
   // If a connection reset happens while attempting to log in, the current
   // backoff entry is reused (after incrementing with a new failure).
@@ -136,7 +151,7 @@ class GCM_EXPORT ConnectionFactoryImpl :
   base::TimeTicks last_login_time_;
 
   // The current connection handler, if one exists.
-  scoped_ptr<ConnectionHandlerImpl> connection_handler_;
+  scoped_ptr<ConnectionHandler> connection_handler_;
 
   // Builder for generating new login requests.
   BuildLoginRequestCallback request_builder_;

google_apis/gcm/engine/connection_handler_impl.cc

@@ -402,6 +402,9 @@ void ConnectionHandlerImpl::CloseConnection() {
   if (socket_)
     socket_->Disconnect();
   socket_ = NULL;
+  handshake_complete_ = false;
+  message_tag_ = 0;
+  message_size_ = 0;
   input_stream_.reset();
   output_stream_.reset();
   weak_ptr_factory_.InvalidateWeakPtrs();

tools/metrics/histograms/histograms.xml

@@ -33577,6 +33577,7 @@ other types of suffix sets.
   <int value="1" label="Close command"/>
   <int value="2" label="Heartbeat failure"/>
   <int value="3" label="Socket failure"/>
+  <int value="4" label="Network change"/>
 </enum>
 
 <enum name="GCMEndpoints" type="int">