[GCM] Update backoff settings and reconnection logic

Updates the backoff policy, add support for manually triggering a
connection reset that restores the previous backoff, and don't reset
backoff until successful login.

BUG=284553

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

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

8 files changed, 136 insertions, 12 deletions

diff --git a/google_apis/gcm/engine/connection_factory.h b/google_apis/gcm/engine/connection_factory.h
index 3cff482..962a79a 100644
--- a/google_apis/gcm/engine/connection_factory.h
+++ b/google_apis/gcm/engine/connection_factory.h
@@ -57,6 +57,12 @@ class GCM_EXPORT ConnectionFactory {
   // a null time, indicating either no attempt to connect has been made or no
   // backoff is in progress.
   virtual base::TimeTicks NextRetryAttempt() const = 0;
+
+  // Manually reset the connection. This can occur if an application specific
+  // event forced a reset (e.g. server sends a close connection response).
+  // If the last connection was made within kConnectionResetWindowSecs, the old
+  // backoff is restored, else a new backoff kicks off.
+  virtual void SignalConnectionReset() = 0;
 };
 
 }  // namespace gcm
diff --git a/google_apis/gcm/engine/connection_factory_impl.cc b/google_apis/gcm/engine/connection_factory_impl.cc
index 388b9dc..d577a68 100644
--- a/google_apis/gcm/engine/connection_factory_impl.cc
+++ b/google_apis/gcm/engine/connection_factory_impl.cc
@@ -22,6 +22,11 @@ namespace {
 // The amount of time a Socket read should wait before timing out.
 const int kReadTimeoutMs = 30000;  // 30 seconds.
 
+// If a connection is reset after succeeding within this window of time,
+// the previous backoff entry is restored (and the connection success is treated
+// as if it was transient).
+const int kConnectionResetWindowSecs = 10;  // 10 seconds.
+
 // Backoff policy.
 const net::BackoffEntry::Policy kConnectionBackoffPolicy = {
   // Number of initial errors (in sequence) to ignore before applying
@@ -36,10 +41,10 @@ const net::BackoffEntry::Policy kConnectionBackoffPolicy = {
 
   // Fuzzing percentage. ex: 10% will spread requests randomly
   // between 90%-100% of the calculated time.
-  0.2, // 20%.
+  0.5,  // 50%.
 
   // Maximum amount of time we are willing to delay our request in ms.
-  1000 * 3600 * 4, // 4 hours.
+  1000 * 60 * 5, // 5 minutes.
 
   // Time to keep an entry from being discarded even when it
   // has no significant state, -1 to never discard.
@@ -58,6 +63,7 @@ ConnectionFactoryImpl::ConnectionFactoryImpl(
   : mcs_endpoint_(mcs_endpoint),
     network_session_(network_session),
     net_log_(net_log),
+    connecting_(false),
     weak_ptr_factory_(this) {
 }
 
@@ -70,6 +76,7 @@ void ConnectionFactoryImpl::Initialize(
     const ConnectionHandler::ProtoSentCallback& write_callback) {
   DCHECK(!connection_handler_);
 
+  previous_backoff_ = CreateBackoffEntry(&kConnectionBackoffPolicy);
   backoff_entry_ = CreateBackoffEntry(&kConnectionBackoffPolicy);
   request_builder_ = request_builder;
 
@@ -92,6 +99,7 @@ void ConnectionFactoryImpl::Connect() {
   DCHECK(connection_handler_);
   DCHECK(!IsEndpointReachable());
 
+  connecting_ = true;
   if (backoff_entry_->ShouldRejectRequest()) {
     DVLOG(1) << "Delaying MCS endpoint connection for "
              << backoff_entry_->GetTimeUntilRelease().InMilliseconds()
@@ -112,6 +120,21 @@ bool ConnectionFactoryImpl::IsEndpointReachable() const {
   return connection_handler_ && connection_handler_->CanSendMessage();
 }
 
+void ConnectionFactoryImpl::SignalConnectionReset() {
+  if (connecting_)
+    return;  // Already attempting to reconnect.
+
+  if (!backoff_reset_time_.is_null() &&
+      base::TimeTicks::Now() - backoff_reset_time_ <=
+          base::TimeDelta::FromSeconds(kConnectionResetWindowSecs)) {
+    backoff_entry_.swap(previous_backoff_);
+    backoff_entry_->InformOfRequest(false);
+  }
+  backoff_reset_time_ = base::TimeTicks();
+  previous_backoff_->Reset();
+  Connect();
+}
+
 base::TimeTicks ConnectionFactoryImpl::NextRetryAttempt() const {
   if (!backoff_entry_)
     return base::TimeTicks();
@@ -186,15 +209,19 @@ void ConnectionFactoryImpl::OnConnectDone(int result) {
     return;
   }
 
-  DVLOG(1) << "MCS endpoint connection success.";
-
-  // Reset the backoff.
-  backoff_entry_->Reset();
-
+  DVLOG(1) << "MCS endpoint socket connection success, starting handshake.";
   InitHandler();
 }
 
 void ConnectionFactoryImpl::ConnectionHandlerCallback(int result) {
+  if (result == net::OK) {
+    // Handshake succeeded, reset the backoff.
+    connecting_ = false;
+    backoff_reset_time_ = base::TimeTicks::Now();
+    previous_backoff_.swap(backoff_entry_);
+    backoff_entry_->Reset();
+    return;
+  }
   // TODO(zea): Consider how to handle errors that may require some sort of
   // user intervention (login page, etc.).
   LOG(ERROR) << "Connection reset with error " << result;
diff --git a/google_apis/gcm/engine/connection_factory_impl.h b/google_apis/gcm/engine/connection_factory_impl.h
index d807270..b4b7ae3 100644
--- a/google_apis/gcm/engine/connection_factory_impl.h
+++ b/google_apis/gcm/engine/connection_factory_impl.h
@@ -43,6 +43,7 @@ class GCM_EXPORT ConnectionFactoryImpl :
   virtual void Connect() OVERRIDE;
   virtual bool IsEndpointReachable() const OVERRIDE;
   virtual base::TimeTicks NextRetryAttempt() const OVERRIDE;
+  virtual void SignalConnectionReset() OVERRIDE;
 
   // NetworkChangeNotifier observer implementations.
   virtual void OnConnectionTypeChanged(
@@ -68,10 +69,10 @@ class GCM_EXPORT ConnectionFactoryImpl :
   // Callback for Socket connection completion.
   void OnConnectDone(int result);
 
- private:
   // ConnectionHandler callback for connection issues.
   void ConnectionHandlerCallback(int result);
 
+ private:
   // The MCS endpoint to make connections to.
   const GURL mcs_endpoint_;
 
@@ -84,6 +85,14 @@ class GCM_EXPORT ConnectionFactoryImpl :
   net::ClientSocketHandle socket_handle_;
   // Connection attempt backoff policy.
   scoped_ptr<net::BackoffEntry> backoff_entry_;
+  // Backoff policy from previous backoff attempt.
+  scoped_ptr<net::BackoffEntry> previous_backoff_;
+  base::TimeTicks backoff_reset_time_;
+
+  // 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.
+  bool connecting_;
 
   // The current connection handler, if one exists.
   scoped_ptr<ConnectionHandlerImpl> connection_handler_;
diff --git a/google_apis/gcm/engine/connection_factory_impl_unittest.cc b/google_apis/gcm/engine/connection_factory_impl_unittest.cc
index 1e0ccef..310dfcb 100644
--- a/google_apis/gcm/engine/connection_factory_impl_unittest.cc
+++ b/google_apis/gcm/engine/connection_factory_impl_unittest.cc
@@ -130,6 +130,7 @@ void TestConnectionFactoryImpl::ConnectImpl() {
 
 void TestConnectionFactoryImpl::InitHandler() {
   EXPECT_NE(connect_result_, net::ERR_UNEXPECTED);
+  ConnectionHandlerCallback(net::OK);
 }
 
 scoped_ptr<net::BackoffEntry> TestConnectionFactoryImpl::CreateBackoffEntry(
@@ -299,5 +300,80 @@ TEST_F(ConnectionFactoryImplTest, FailThenConnectionTypeEvent) {
   EXPECT_TRUE(factory()->NextRetryAttempt().is_null());
 }
 
+// Fail after successful connection via signal reset.
+TEST_F(ConnectionFactoryImplTest, FailViaSignalReset) {
+  factory()->Initialize(
+      ConnectionFactory::BuildLoginRequestCallback(),
+      ConnectionHandler::ProtoReceivedCallback(),
+      ConnectionHandler::ProtoSentCallback());
+  factory()->SetConnectResult(net::OK);
+  factory()->Connect();
+  WaitForConnections();
+  EXPECT_TRUE(factory()->NextRetryAttempt().is_null());
+
+  factory()->SignalConnectionReset();
+  EXPECT_FALSE(factory()->NextRetryAttempt().is_null());
+}
+
+TEST_F(ConnectionFactoryImplTest, IgnoreResetWhileConnecting) {
+  factory()->Initialize(
+      ConnectionFactory::BuildLoginRequestCallback(),
+      ConnectionHandler::ProtoReceivedCallback(),
+      ConnectionHandler::ProtoSentCallback());
+  factory()->SetConnectResult(net::OK);
+  factory()->Connect();
+  WaitForConnections();
+  EXPECT_TRUE(factory()->NextRetryAttempt().is_null());
+
+  factory()->SignalConnectionReset();
+  base::TimeTicks retry_time = factory()->NextRetryAttempt();
+  EXPECT_FALSE(retry_time.is_null());
+
+  const int kNumAttempts = 5;
+  for (int i = 0; i < kNumAttempts; ++i)
+    factory()->SignalConnectionReset();
+  EXPECT_EQ(retry_time, factory()->NextRetryAttempt());
+}
+
+// Go into backoff due to connection failure. On successful connection, receive
+// a signal reset. The original backoff should be restored and extended, rather
+// than a new backoff starting from scratch.
+TEST_F(ConnectionFactoryImplTest, SignalResetRestoresBackoff) {
+  factory()->Initialize(
+      ConnectionFactory::BuildLoginRequestCallback(),
+      ConnectionHandler::ProtoReceivedCallback(),
+      ConnectionHandler::ProtoSentCallback());
+  factory()->SetConnectResult(net::ERR_CONNECTION_FAILED);
+  base::TimeTicks connect_time = base::TimeTicks::Now();
+  factory()->Connect();
+  WaitForConnections();
+  base::TimeTicks retry_time = factory()->NextRetryAttempt();
+  EXPECT_FALSE(retry_time.is_null());
+
+  factory()->SetConnectResult(net::OK);
+  connect_time = base::TimeTicks::Now();
+  WaitForConnections();
+  EXPECT_TRUE(factory()->NextRetryAttempt().is_null());
+
+  factory()->SignalConnectionReset();
+  EXPECT_NE(retry_time, factory()->NextRetryAttempt());
+  retry_time = factory()->NextRetryAttempt();
+  EXPECT_FALSE(retry_time.is_null());
+  EXPECT_GE((retry_time - connect_time).InMilliseconds(),
+            CalculateBackoff(2));
+
+  factory()->SetConnectResult(net::OK);
+  connect_time = base::TimeTicks::Now();
+  WaitForConnections();
+  EXPECT_TRUE(factory()->NextRetryAttempt().is_null());
+
+  factory()->SignalConnectionReset();
+  EXPECT_NE(retry_time, factory()->NextRetryAttempt());
+  retry_time = factory()->NextRetryAttempt();
+  EXPECT_FALSE(retry_time.is_null());
+  EXPECT_GE((retry_time - connect_time).InMilliseconds(),
+            CalculateBackoff(3));
+}
+
 }  // namespace
 }  // namespace gcm
diff --git a/google_apis/gcm/engine/connection_handler_impl.cc b/google_apis/gcm/engine/connection_handler_impl.cc
index aff0dfd..9772000 100644
--- a/google_apis/gcm/engine/connection_handler_impl.cc
+++ b/google_apis/gcm/engine/connection_handler_impl.cc
@@ -379,6 +379,7 @@ void ConnectionHandlerImpl::OnGotMessageBytes() {
     } else {
       handshake_complete_ = true;
       DVLOG(1) << "GCM Handshake complete.";
+      connection_callback_.Run(net::OK);
     }
   }
   read_callback_.Run(protobuf.Pass());
diff --git a/google_apis/gcm/engine/fake_connection_factory.cc b/google_apis/gcm/engine/fake_connection_factory.cc
index 54b3423..93e66cb 100644
--- a/google_apis/gcm/engine/fake_connection_factory.cc
+++ b/google_apis/gcm/engine/fake_connection_factory.cc
@@ -43,4 +43,8 @@ base::TimeTicks FakeConnectionFactory::NextRetryAttempt() const {
   return base::TimeTicks();
 }
 
+void FakeConnectionFactory::SignalConnectionReset() {
+  Connect();
+}
+
 }  // namespace gcm
diff --git a/google_apis/gcm/engine/fake_connection_factory.h b/google_apis/gcm/engine/fake_connection_factory.h
index 60b10e1..bd29310 100644
--- a/google_apis/gcm/engine/fake_connection_factory.h
+++ b/google_apis/gcm/engine/fake_connection_factory.h
@@ -28,6 +28,7 @@ class FakeConnectionFactory : public ConnectionFactory {
   virtual void Connect() OVERRIDE;
   virtual bool IsEndpointReachable() const OVERRIDE;
   virtual base::TimeTicks NextRetryAttempt() const OVERRIDE;
+  virtual void SignalConnectionReset() OVERRIDE;
 
  private:
   scoped_ptr<FakeConnectionHandler> connection_handler_;
diff --git a/google_apis/gcm/engine/mcs_client.cc b/google_apis/gcm/engine/mcs_client.cc
index f0af051..e8291f0 100644
--- a/google_apis/gcm/engine/mcs_client.cc
+++ b/google_apis/gcm/engine/mcs_client.cc
@@ -451,6 +451,7 @@ void MCSClient::HandlePacketFromWire(
 
   switch (tag) {
     case kLoginResponseTag: {
+      DCHECK_EQ(CONNECTING, state_);
       mcs_proto::LoginResponse* login_response =
           reinterpret_cast<mcs_proto::LoginResponse*>(protobuf.get());
       DVLOG(1) << "Received login response:";
@@ -503,10 +504,9 @@ void MCSClient::HandlePacketFromWire(
       // timeout (with backoff).
       return;
     case kCloseTag:
-      LOG(ERROR) << "Received close command, closing connection.";
-      state_ = UNINITIALIZED;
-      initialization_callback_.Run(false, 0, 0);
-      // TODO(zea): should this happen in non-error cases? Reconnect?
+      LOG(ERROR) << "Received close command, resetting connection.";
+      state_ = LOADED;
+      connection_factory_->SignalConnectionReset();
       return;
     case kIqStanzaTag: {
       DCHECK_GE(stream_id_in_, 1U);