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;
 }
 
+// The StaticCallHostOfflineReasonAckCallback is just like
+// Callback<void(AckOrTimeout)::Run method except that the first (i.e. |this|)
+// argument is taken by value rather than by pointer.  This is needed to make it
+// work with base::Bind inside CallHostOfflineReasonAckCallback below.

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

This description is not very helpful; I think all you really want to say is that
this method takes a supplied |ack_callback| and Run()s it with the bound
|success| value?

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

N/A - this function goes away in patchset #18 (it turns out that the way to bind
a callback I just need to pass the callback as a first argument, rather than
trying to pass Run method + callback pointer as I've been trying to do
previously).

+static void StaticCallHostOfflineReasonAckCallback(

[Lambros, 2014/12/16 02:14:05]

nit: Maybe just call it RunAckCallback().
nit: Move this to anonymous namespace, without any "static".

[Łukasz Anforowicz, 2014/12/17 18:02:04]

Done (except I renamed to RunHostOfflineReasonAckCallback).

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

CallHostOfflineReasonAckCallback

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

N/A as of patchset #18

+    base::Callback<void(HeartbeatSender::AckOrTimeout)> ack_callback,
+    HeartbeatSender::AckOrTimeout ack_or_timeout) {
+  ack_callback.Run(ack_or_timeout);
+}
+
+void HeartbeatSender::CallHostOfflineReasonAckCallback(

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

PostHostOfflineReasonAckCallback

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

Good point - thanks.

+    AckOrTimeout ack_or_timeout) {
+  // We go through PostTask (rather than directly calling Run),

[Lambros, 2014/12/16 02:14:05]

I don't understand why you have to make this asynchronous. Doing it this way
means you're breaking the atomicity of (calling the callback + stopping the
timer), surely?

[Łukasz Anforowicz, 2014/12/17 18:02:04]

RE: why

Requiring the callers (and callers of the callers) to always do the call as the
last statement seems icky.

RE: breaking the atomicity

What I really care about is that when a bot ack, then 
the host_offline_reason_ack_callback will be run exactly once
- I don't care about the slight delay introduced by PostTask
- The "exactly once" is enforced by calling Reset on the callback field (this is
independent from OneShotTimer::Stop call which is only needed for hygiene /
releasing other memory when ok to do so).
- I do care that it is ok for the callback to destroy HeartbeatSender (this is
why PostTask to call the callback with no more frames with heartbeat sender on
the callstack)

+  // 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.
+  base::Closure fully_bound_ack_callback =
+      base::Bind(StaticCallHostOfflineReasonAckCallback,

[Lambros, 2014/12/16 02:14:04]

Are you sure you really need this? It seems very sad that base::Bind() can't
take a Callback directly as its first parameter, and expects either a bare
function or a class method.

[Łukasz Anforowicz, 2014/12/17 18:02:04]

I tried passing the callback object by value - this didn't compile.  
I tried to look through the comments of base/callback.h and base/bind_helpers.h
but didn't see how to achieve this.

I'll send a post to chromium-dev to ask.

+                 host_offline_reason_ack_callback_, ack_or_timeout);
+  base::MessageLoop::current()->PostTask(FROM_HERE, fully_bound_ack_callback);
+
+  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));
   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();
+          CallHostOfflineReasonAckCallback(AckOrTimeout::Ack);
+          host_offline_reason_timeout_timer_.Stop();

[Lambros, 2014/12/16 02:14:04]

Maybe stop the timer first?

[Łukasz Anforowicz, 2014/12/17 18:02:04]

Done.

         }
       }
     }
   }
 }
 
 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