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

remoting/host/minimum_heartbeat_supporter.h

Index: remoting/host/minimum_heartbeat_supporter.h
diff --git a/remoting/host/minimum_heartbeat_supporter.h b/remoting/host/minimum_heartbeat_supporter.h
new file mode 100644
index 0000000000000000000000000000000000000000..b9265838dc2b46e93a8bcdb53b6f231cb89be59d
--- /dev/null
+++ b/remoting/host/minimum_heartbeat_supporter.h
@@ -0,0 +1,130 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.

[Lambros, 2014/11/14 01:24:58]

2014, no '(c)'

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

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef REMOTING_HOST_MINIMUM_HEARTBEAT_SUPPORTER_H_
+#define REMOTING_HOST_MINIMUM_HEARTBEAT_SUPPORTER_H_
+
+#include <string>
+
+#include "base/cancelable_callback.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/ref_counted_delete_on_message_loop.h"
+#include "base/memory/scoped_ptr.h"
+#include "net/url_request/url_request_context_getter.h"
+#include "remoting/base/auto_thread.h"
+#include "remoting/base/rsa_key_pair.h"
+#include "remoting/host/heartbeat_sender.h"
+#include "remoting/signaling/xmpp_signal_strategy.h"
+
+namespace base {
+  class TimeDelta;
+}
+
+namespace net {
+  class NetworkChangeNotifier;
+}
+
+namespace remoting {
+
+class ChromotingHostContext;
+class SignalStrategy;
+class SignalingConnector;
+class OAuthTokenGetter;
+
+/* Keeping an instance of MinimumHeartbeatSupporter alive

[Lambros, 2014/11/14 01:24:57]

nit: C++ style '//'

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

Done.

+ * ensures that we keep trying to send heartbeats (which among other
+ * things means that it ensures that we don't exit the host process).
+ *
+ * Primary scenario where this class is useful is extending host process
+ * lifetime after |SendHostOfflineReason| is called.
+ *
+ * Secondary benefit of this class is hiding/encapsulating heartbeat
+ * and xmpp related things inside.
+ */
+class MinimumHeartbeatSupporter
+    : public base::RefCountedDeleteOnMessageLoop<MinimumHeartbeatSupporter>,
+      public HeartbeatSender::Listener {
+ public:
+  class Listener {
+   public:
+    virtual ~Listener() { }
+
+    // Invoked after the first successful heartbeat.
+    virtual void OnHeartbeatSuccessful() = 0;
+
+    // Invoked when the host ID is permanently not recognized by the server.
+    virtual void OnUnknownHostIdError() = 0;
+
+    // Invoked when authentication fails.
+    virtual void OnAuthFailed() = 0;
+  };
+
+  MinimumHeartbeatSupporter(
+      const ChromotingHostContext* host_context,
+      const XmppSignalStrategy::XmppServerConfig& xmpp_server_config,
+      const std::string& talkgadget_prefix,
+      const std::string& host_id,
+      scoped_refptr<RsaKeyPair> key_pair,
+      const std::string& directory_bot_jid,
+      const std::string& oauth_refresh_token,
+      bool use_service_account);
+
+  // gets signal strategy used for talking to the Chromoting bot

[Lambros, 2014/11/14 01:24:57]

nit: Capital letter + period.

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

Done.

+  // return value is valid until |this| gets deleted
+  SignalStrategy* GetSignalStrategy();
+
+  // Registers a listener to forward various events to
+  // Caller has to make sure that |listener| is valid for the lifetime of |this|
+  // or until the next call to |SetListener| [whichever is shorter]
+  void SetListener(Listener* listener);
+
+  // Kicks off sending a heartbeat containing a host-offline-reason attribute.
+  //
+  // Ensures that |this| (and transitively the whole process) is kept alive
+  // while waiting for an ack from the bot
+  // - this is subject to |timeout|
+  // - this is implemented by temporarily bumping-up ref-count to |this|
+  //
+  // Caller has to guarantee that |host_offline_reason| is valid
+  // for the lifetime of |this| (which in practice means that
+  // |host_offline_reason| has to point to a string literal.
+  void SendHostOfflineReason(
+      const char* host_offline_reason,
+      const base::TimeDelta& timeout);
+
+ private:
+   friend class base::RefCountedDeleteOnMessageLoop<MinimumHeartbeatSupporter>;
+   friend class base::DeleteHelper<MinimumHeartbeatSupporter>;
+   virtual ~MinimumHeartbeatSupporter() override;
+
+  // HeartbeatSender::Listener overrides.
+  virtual void OnHeartbeatSuccessful() override;
+  virtual void OnUnknownHostIdError() override;
+
+  // Callback used by SignalingConnector
+  void OnAuthFailed();
+
+  // Fields that support sending of heartbeats
+  scoped_refptr<AutoThreadTaskRunner> network_task_runner_;
+  scoped_ptr<net::NetworkChangeNotifier> network_change_notifier_;
+  scoped_ptr<XmppSignalStrategy> signal_strategy_;

[Lambros, 2014/11/14 01:24:57]

I'm not sure about this being the class that owns all this stuff? Things like
SignalStrategy should be maybe passed in to the ctor (dependency injection)?

On the other hand, if this is going to be an important class that owns the
world, it should probably have a more generic name like "StatusSender" or
something that implies: this class is *the* place for doing all status-related
stuff, and is responsible for storing and creating the SignalStrategy and
anything else needed for sending status messages?

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

RE: this class owning the fields

This class needs to control the lifetime of the things above (like
signal_strategy_, signaling_connector_, oauth_token_getter_, heartbeat_sender_)
in order to be able to send the host-offline-reason heartbeat, even if the rest
of the host is down.  When I say "control", I primarily mean "keep those objects
alive and be able to use them", but I also think it is good to encapsulate
creation of those objects here (otherwise HostProcess becomes a kind of a god
object).

Maybe we can discuss the naming in person.  I picked MinimumHeartbeatSupporter,
because having an object of this class alive, is a minimal thing that will
periodically keep sending heartbeats to the bot.

RE: dependency injection

Hmmm... good point.  To make things unit-testable, I extracted a
ReportAckOrTimeout function and put it into a separate
ack_or_timeout_reporter.h/cc.  After converting to Callbacks (instead of
Listener pattern) I was also able to follow your advice on having a dumb
constructor, with most of object creation logic in a separate, static method.

+  scoped_ptr<SignalingConnector> signaling_connector_;
+  scoped_ptr<OAuthTokenGetter> oauth_token_getter_;
+  scoped_ptr<HeartbeatSender> heartbeat_sender_;
+
+  Listener* listener_;
+
+  // Helper methods and fields for mechanics of SendHostOfflineReason
+  scoped_ptr<base::CancelableClosure> timeout_callback_;
+  scoped_ptr<base::CancelableClosure> ack_callback_;
+  void OnAck();
+  void OnTimeout();
+  void OnAckOrTimeout();
+
+  DISALLOW_COPY_AND_ASSIGN(MinimumHeartbeatSupporter);
+};
+
+}  // namespace remoting
+
+#endif  // REMOTING_HOST_MINIMUM_HEARTBEAT_SUPPORTER_H_