[GCM] Fixing the client interval interaction with heartbeat ack

google_apis/gcm/engine/heartbeat_manager.cc

 // Copyright 2014 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/heartbeat_manager.h"
 
+#include <limits>

[Nicolas Zea, 2016/03/03 19:09:40]

nit: not needed anymore

[fgorski, 2016/03/03 20:31:32]

Done.

 #include <utility>
 
 #include "base/callback.h"
 #include "base/location.h"
 #include "base/metrics/histogram.h"
 #include "base/power_monitor/power_monitor.h"
 #include "base/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
 #include "build/build_config.h"
@@ skipping 47 matching lines @@
   DCHECK(!send_heartbeat_callback.is_null());
   DCHECK(!trigger_reconnect_callback.is_null());
   send_heartbeat_callback_ = send_heartbeat_callback;
   trigger_reconnect_callback_ = trigger_reconnect_callback;
 
   // Listen for system suspend and resume events.
   base::PowerMonitor* monitor = base::PowerMonitor::Get();
   if (monitor)
     monitor->AddObserver(this);
 
+  // Calculated the heartbeat interval just before we start the timer.
+  UpdateHeartbeatInterval();
+
   // Kicks off the timer.
   waiting_for_ack_ = false;
   RestartTimer();
 }
 
 void HeartbeatManager::Stop() {
   heartbeat_expected_time_ = base::Time();
   heartbeat_interval_ms_ = 0;
   heartbeat_timer_->Stop();
   waiting_for_ack_ = false;
@@ skipping 15 matching lines @@
 
 void HeartbeatManager::UpdateHeartbeatConfig(
     const mcs_proto::HeartbeatConfig& config) {
   if (!config.IsInitialized() ||
       !config.has_interval_ms() ||
       config.interval_ms() <= 0) {
     return;
   }
   DVLOG(1) << "Updating server heartbeat interval to " << config.interval_ms();
   server_interval_ms_ = config.interval_ms();
+
+  // Make sure heartbeat interval is recalculated when new server interval is
+  // available.
+  UpdateHeartbeatInterval();
 }
 
 base::TimeTicks HeartbeatManager::GetNextHeartbeatTime() const {
   if (heartbeat_timer_->IsRunning())
     return heartbeat_timer_->desired_run_time();
   else
     return base::TimeTicks();
 }
 
 void HeartbeatManager::UpdateHeartbeatTimer(scoped_ptr<base::Timer> timer) {
@@ skipping 40 matching lines @@
     ResetConnection(ConnectionFactory::HEARTBEAT_FAILURE);
     return;
   }
 
   waiting_for_ack_ = true;
   RestartTimer();
   send_heartbeat_callback_.Run();
 }
 
 void HeartbeatManager::RestartTimer() {
+  int interval_ms = heartbeat_interval_ms_;
   if (!waiting_for_ack_) {

[Nicolas Zea, 2016/03/03 19:09:40]

nit: for readability, would be better to do:
if (waiting for ack)
   ...
} else {
  ...
}

(negative conditions are less readable in general)

[fgorski, 2016/03/03 20:31:32]

Done.

-    // Recalculate the timer interval based network type.
-    // Server interval takes precedence over client interval, even if the latter
-    // is less.
-    if (server_interval_ms_ != 0) {
-      // If a server interval is set, it overrides any local one.
-      heartbeat_interval_ms_ = server_interval_ms_;
-    } else if (HasClientHeartbeatInterval()) {
-      // Client interval might have been adjusted up, which should only take
-      // effect during a reconnection.
-      if (client_interval_ms_ < heartbeat_interval_ms_ ||
-          heartbeat_interval_ms_ == 0) {
-        heartbeat_interval_ms_ = client_interval_ms_;
-      }
-    } else {
-      heartbeat_interval_ms_ = GetDefaultHeartbeatInterval();
-    }
-    DVLOG(1) << "Sending next heartbeat in "
-             << heartbeat_interval_ms_ << " ms.";
+    DVLOG(1) << "Sending next heartbeat in " << interval_ms << " ms.";
   } else {
-    heartbeat_interval_ms_ = kHeartbeatAckDefaultMs;
-    DVLOG(1) << "Resetting timer for ack with "
-             << heartbeat_interval_ms_ << " ms interval.";
+    interval_ms = kHeartbeatAckDefaultMs;
+    DVLOG(1) << "Resetting timer for ack within " << interval_ms << " ms.";
   }
 
   heartbeat_expected_time_ =
-      base::Time::Now() +
-      base::TimeDelta::FromMilliseconds(heartbeat_interval_ms_);
+      base::Time::Now() + base::TimeDelta::FromMilliseconds(interval_ms);
   heartbeat_timer_->Start(FROM_HERE,
-                         base::TimeDelta::FromMilliseconds(
-                             heartbeat_interval_ms_),
-                         base::Bind(&HeartbeatManager::OnHeartbeatTriggered,
-                                    weak_ptr_factory_.GetWeakPtr()));
+                          base::TimeDelta::FromMilliseconds(interval_ms),
+                          base::Bind(&HeartbeatManager::OnHeartbeatTriggered,
+                                     weak_ptr_factory_.GetWeakPtr()));
 
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS)
   // Windows, Mac, Android, iOS, and Chrome OS all provide a way to be notified
   // when the system is suspending or resuming.  The only one that does not is
   // Linux so we need to poll to check for missed heartbeats.
   base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE,
       base::Bind(&HeartbeatManager::CheckForMissedHeartbeat,
                  weak_ptr_factory_.GetWeakPtr()),
       base::TimeDelta::FromMilliseconds(kHeartbeatMissedCheckMs));
@@ skipping 16 matching lines @@
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS)
   // Otherwise check again later.
   base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE,
       base::Bind(&HeartbeatManager::CheckForMissedHeartbeat,
                  weak_ptr_factory_.GetWeakPtr()),
       base::TimeDelta::FromMilliseconds(kHeartbeatMissedCheckMs));
 #endif  // defined(OS_LINUX) && !defined(OS_CHROMEOS)
 }
 
+void HeartbeatManager::UpdateHeartbeatInterval() {
+  // Server interval takes precedence over client interval, even if the latter
+  // is less.
+  if (server_interval_ms_ != 0) {
+    // If a server interval is set, it overrides any local one.
+    heartbeat_interval_ms_ = server_interval_ms_;
+  } else if (HasClientHeartbeatInterval() &&
+             (client_interval_ms_ < heartbeat_interval_ms_ ||
+              heartbeat_interval_ms_ == 0)) {
+    // Client interval might have been adjusted up, which should only take
+    // effect during a reconnection.
+    heartbeat_interval_ms_ = client_interval_ms_;
+  } else if (heartbeat_interval_ms_ == 0) {
+    // If interval is still 0, recalculate it based on network type.
+    heartbeat_interval_ms_ = GetDefaultHeartbeatInterval();
+  }

[fgorski, 2016/03/03 17:10:35]

zea@ perhaps we can have a DCHECK(heartbeat_interval_ms_ > 0) here?

[Nicolas Zea, 2016/03/03 19:09:40]

DCHECK_GT, but yes SGTM :)

+}
+
 int HeartbeatManager::GetDefaultHeartbeatInterval() {
   // For unknown connections, use the longer cellular heartbeat interval.
   int heartbeat_interval_ms = kCellHeartbeatDefaultMs;
   if (net::NetworkChangeNotifier::GetConnectionType() ==
           net::NetworkChangeNotifier::CONNECTION_WIFI ||
       net::NetworkChangeNotifier::GetConnectionType() ==
           net::NetworkChangeNotifier::CONNECTION_ETHERNET) {
     heartbeat_interval_ms = kWifiHeartbeatDefaultMs;
   }
   return heartbeat_interval_ms;
@@ skipping 38 matching lines @@
       interval <= max_heartbeat_interval;
 }
 
 void HeartbeatManager::ResetConnection(
     ConnectionFactory::ConnectionResetReason reason) {
   Stop();
   trigger_reconnect_callback_.Run(reason);
 }
 
 }  // namespace gcm