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

remoting/host/heartbeat_sender.cc

Index: remoting/host/heartbeat_sender.cc
diff --git a/remoting/host/heartbeat_sender.cc b/remoting/host/heartbeat_sender.cc
index c64f2f49cf1c6b536afa13481ebde83f6dea8882..248057cca11e11405b33aedc471b136696ed1ece 100644
--- a/remoting/host/heartbeat_sender.cc
+++ b/remoting/host/heartbeat_sender.cc
@@ -54,7 +54,7 @@ HeartbeatSender::HeartbeatSender(
     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),
@@ -102,13 +102,32 @@ bool HeartbeatSender::OnSignalStrategyIncomingStanza(
   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();
   }
@@ -231,10 +250,8 @@ void HeartbeatSender::ProcessResponse(
       // 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);
         }
       }
     }