Reporting of policy errors via host-offline-reason: part 3

remoting/host/heartbeat_sender.cc

 // Copyright (c) 2012 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 "remoting/host/heartbeat_sender.h"
 
 #include <math.h>
 
 #include "base/bind.h"
 #include "base/message_loop/message_loop_proxy.h"
@@ skipping 36 matching lines @@
 const int64 kResendDelayOnHostNotFoundMs = 10 * 1000; // 10 seconds.
 const int kMaxResendOnHostNotFoundCount = 12;  // 2 minutes (12 x 10 seconds).
 
 }  // namespace
 
 HeartbeatSender::HeartbeatSender(
     const base::Closure& on_heartbeat_successful_callback,
     const base::Closure& on_unknown_host_id_error,
     const std::string& host_id,
     SignalStrategy* signal_strategy,
-    scoped_refptr<RsaKeyPair> key_pair,
+    const scoped_refptr<const RsaKeyPair>& key_pair,
     const std::string& directory_bot_jid)
     : on_heartbeat_successful_callback_(on_heartbeat_successful_callback),
       on_unknown_host_id_error_(on_unknown_host_id_error),
       host_id_(host_id),
       signal_strategy_(signal_strategy),
       key_pair_(key_pair),
       directory_bot_jid_(directory_bot_jid),
       interval_ms_(kDefaultHeartbeatIntervalMs),
       sequence_id_(0),
       sequence_id_was_set_(false),
@@ skipping 27 matching lines @@
     timer_.Stop();
     timer_resend_.Stop();
   }
 }
 
 bool HeartbeatSender::OnSignalStrategyIncomingStanza(
     const buzz::XmlElement* stanza) {
   return false;
 }
 
+void HeartbeatSender::CallHostOfflineReasonAckCallback(
+    AckOrTimeout ack_or_timeout) {
+  // We go through PostTask (rather than directly calling Run),
+  // to unwind frames with |this| pointer from the stack.
+  // While Run invocation can be the last statement in this
+  // particular method, we don't want to force maintainers
+  // of our callers to ensure that they call us last.

[Wez, 2014/12/18 00:46:20]

This is a common pattern, so doesn't need such a long explanation, e.g. "Run the
ACK callback under a clean stack via PostTask()."

[Łukasz Anforowicz, 2014/12/18 19:02:33]

I tried to shorten the comment following your suggestion.  OTOH, Lambros was
asking for an explanation why we need to go through the trouble of PostTask /
why we care about a clean callstack, so I ended up inserting an explanation
focusing on the "why".

[Wez, 2014/12/19 22:05:36]

Acknowledged.

+  base::Closure fully_bound_ack_callback =

[Lambros, 2014/12/17 23:44:34]

nit: Don't need the temporary. We do
PostTask(FROM_HERE, base::Bind(...
a lot in our code.

[Łukasz Anforowicz, 2014/12/18 00:03:41]

Acknowledged.

+      base::Bind(host_offline_reason_ack_callback_, ack_or_timeout);
+  base::MessageLoop::current()->PostTask(FROM_HERE, fully_bound_ack_callback);

[Wez, 2014/12/18 00:46:20]

nit: Use ThreadTaskRunnerHandle::Get() here.

[Łukasz Anforowicz, 2014/12/18 19:02:33]

Done.  Can you please give more details / help me understand why this is
preferrable?  I used message loop because it seemed more generic / more broadly
applicabla (i.e. wasn't introducing a tight coupling between this PostTask call
and SingleThreadTaskRunner [i.e. another SequencedTaskRunner would also work]).

[Wez, 2014/12/19 22:05:36]

In using the current MessageLoop's TaskRunner you're intrinsically tying the
operation to the SingleThreadTaskRunner for this thread, so it's equivalent.

The idea is really that ThreadTaskRunnerHandle is the new shiny way to get the
current thread's TaskRunner; the fact that that is a SingleThreadTaskRunner
whereas you only really need a SequencedTaskRunner doesn't really make an
difference.

+
+  host_offline_reason_ack_callback_.Reset();
+}
+
 void HeartbeatSender::SetHostOfflineReason(
     const std::string& host_offline_reason,
-    const base::Closure& ack_callback) {
+    const base::TimeDelta& timeout,
+    const base::Callback<void(AckOrTimeout)>& ack_callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(host_offline_reason_ack_callback_.is_null());
   host_offline_reason_ = host_offline_reason;
   host_offline_reason_ack_callback_ = ack_callback;
+  host_offline_reason_timeout_timer_.Start(
+      FROM_HERE, timeout,
+      base::Bind(&HeartbeatSender::CallHostOfflineReasonAckCallback,
+                 base::Unretained(this), AckOrTimeout::Timeout));

[Wez, 2014/12/18 00:46:20]

Re-using CallHostOfflineReasonAckCallback here really doesn't seem to save you
much/anything. Suggest simply having two methods e.g. OnHostOfflineReasonAck()
and OnHostOfflineReasonTimeout(), which IMO will be more readable anyway.

[Łukasz Anforowicz, 2014/12/18 19:02:33]

Good point.  Done.  This also helped keep the timer and callback resetting logic
in one place (only in the On...Timeout and On...Ack methods).  Thanks.

   if (signal_strategy_->GetState() == SignalStrategy::CONNECTED) {
     DoSendStanza();
   }
 }
 
 void HeartbeatSender::SendStanza() {
   // Make sure we don't send another heartbeat before the heartbeat interval
   // has expired.
   timer_resend_.Stop();
   DoSendStanza();
@@ skipping 102 matching lines @@
 
       // Notify listener of the first successful heartbeat.
       if (!heartbeat_succeeded_) {
         on_heartbeat_successful_callback_.Run();
       }
       heartbeat_succeeded_ = true;
 
       // Notify caller of SetHostOfflineReason that we got an ack.
       if (is_offline_heartbeat_response) {
         if (!host_offline_reason_ack_callback_.is_null()) {
-          base::MessageLoop::current()->PostTask(
-              FROM_HERE,
-              host_offline_reason_ack_callback_);
-          host_offline_reason_ack_callback_.Reset();
+          host_offline_reason_timeout_timer_.Stop();

[Wez, 2014/12/18 00:46:20]

This seems wrong - surely you want to stop the timer even if the callback is
already null?

How can the ACK callback already be null, though?

[Łukasz Anforowicz, 2014/12/18 19:02:33]

In this case the timer is already stopped.
Right - if the callback is null then the timer should have already been stopped.
 I've added explicit asserts in the new On...Timeout and On...Ack methods.

+          CallHostOfflineReasonAckCallback(AckOrTimeout::Ack);
         }
       }
     }
   }
 }
 
 void HeartbeatSender::SetInterval(int interval) {
   if (interval != interval_ms_) {
     interval_ms_ = interval;
 
@@ skipping 62 matching lines @@
 
   std::string message = signal_strategy_->GetLocalJid() + ' ' +
       base::IntToString(sequence_id_);
   std::string signature(key_pair_->SignMessage(message));
   signature_tag->AddText(signature);
 
   return signature_tag.Pass();
 }
 
 }  // namespace remoting