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

remoting/host/minimum_heartbeat_supporter.h

+// 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.
+
+#ifndef REMOTING_HOST_MINIMUM_HEARTBEAT_SUPPORTER_H_
+#define REMOTING_HOST_MINIMUM_HEARTBEAT_SUPPORTER_H_
+
+#include <string>
+
+#include "base/callback.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "remoting/base/auto_thread_task_runner.h"
+#include "remoting/base/rsa_key_pair.h"
+#include "remoting/host/ack_or_timeout_reporter.h"
+#include "remoting/signaling/xmpp_signal_strategy.h"
+
+namespace base {
+class TimeDelta;
+}
+
+namespace net {
+class NetworkChangeNotifier;
+}
+
+namespace remoting {
+
+class ChromotingHostContext;
+class DnsBlackholeChecker;
+class HeartbeatSender;
+class OAuthTokenGetter;
+class SignalStrategy;
+class SignalingConnector;
+
+// MinimumHeartbeatSupporter has 2 functions:
+// 1. Keep sending regulard heartbeats to the service.

[Lambros, 2014/12/06 01:25:05]

nit: regular

[Łukasz Anforowicz, 2014/12/09 18:47:07]

Done.

+// 2. Keep the host process alive while sending host-offline-reason heartbeat.

[Wez, 2014/12/09 00:51:58]

Would it be cleaner to leave HostProcess in charge of heartbeats and move the
Chromoting-host related parts of that into a sub-component that can be created
and torn-down as needed?

[Łukasz Anforowicz, 2014/12/09 18:47:07]

I like the fact that MinimumHeartbeatSupporter has a relatively narrow interface
- users call Create method and afterwards don't have to worry about creating and
maintaining lifetime of the 4 objects that cooperate to send heartbeats -
everything is taken care of by MinimumHeartbeatSupporter.

I don't understand what you mean by "Chromoting-host related parts".  Fields
encapsulated by MinimumHeartbeatSupporter are what is needed to send heartbeats
(including when sending a host-offline-reason).  We want to tear down everything
else when minimizing the security risk associated with malformed policy.

+class MinimumHeartbeatSupporter

[Wez, 2014/12/09 00:51:58]

This seems a not-very-helpful name - it's not clear at a first glance what this
does that HeartbeatSender does not.

[Łukasz Anforowicz, 2014/12/09 18:47:07]

What name would you suggest?

RE: the difference from HeartbeatSender

HeartbeatSender periodically sends heartbeats once something else connects a
signal strategy.  MinimumHeartbeatSender periodically sends heartbeats, without
needing other objects for keeping the signal strategy connected - it itself
takes care of connecting and authtenticating.

RE: This seems a not-very-helpful name

When picking MinimumHeartbeatSupporter I was trying to convey the fact that
keeping this object alive is a minimum required to keep sending heartbeats (i.e.
that this is a life-support system for heartbeats).

+    : public base::RefCounted<MinimumHeartbeatSupporter> {

[Wez, 2014/12/09 00:51:58]

Noooo! Does this really need to be ref-counted?

[Łukasz Anforowicz, 2014/12/09 18:47:07]

It is kept alive by 1) HostProcess (wants to keep sending heartbeats as long as
the host is running) and 2) itself (to give it a chance to send
host-offline-reason during error-related shutdown).  More than 1 owner = have to
have ref-counting?

I think we cannot pass scoped_ptr<MinimumHeartbeatSupporter> from HostProcess to
MinimimHeartbeatSupporter - same comment as in report_ack_or_timeout.cc (cannot
pass scoped_ptr<T>.Pass as an argument and at the same time use it as "this"
when calling an instance method).

+ public:
+  // TODO(lukasza): Refactor to limit the number of parameters below.
+  // Probably necessitates refactoring HostProcess to extract a new
+  // class to read and store config/policy/cmdline values.

[Wez, 2014/12/09 00:51:58]

We store config in a dictionary; we specifically removed the sub-class for that
because it looked just like a DictionaryValue. ;)

[Łukasz Anforowicz, 2014/12/09 18:47:07]

Acknowledged.  OTOH, I thought more about strongly-typed interface (i.e. having
a |host_id| property, rather than passing a "host_id" as a string to a
dictionary-like interface).

+  static scoped_refptr<MinimumHeartbeatSupporter> Create(
+      const base::Closure& on_heartbeat_successful_callback,
+      const base::Closure& on_unknown_host_id_error_callback,
+      const base::Closure& on_auth_failed_callback,
+      const ChromotingHostContext& host_context,
+      const XmppSignalStrategy::XmppServerConfig& xmpp_server_config,
+      const std::string& talkgadget_prefix,
+      const std::string& host_id,
+      const scoped_refptr<const 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.
+  // Return value is valid until |this| gets deleted.
+  SignalStrategy* signal_strategy() { return signal_strategy_.get(); }
+
+  // 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|).
+  void SendHostOfflineReason(const std::string& host_offline_reason,
+                             const base::TimeDelta& timeout);
+
+ private:
+  // Ref-counting helpers.
+  friend class base::RefCounted<MinimumHeartbeatSupporter>;
+  ~MinimumHeartbeatSupporter();
+
+  MinimumHeartbeatSupporter(
+      scoped_refptr<AutoThreadTaskRunner> network_task_runner,
+      scoped_ptr<net::NetworkChangeNotifier> network_change_notifier,
+      scoped_ptr<XmppSignalStrategy> signal_strategy,
+      scoped_ptr<SignalingConnector> signaling_connector,
+      scoped_ptr<HeartbeatSender> heartbeat_sender);
+
+  void OnAckOrTimeout(AckOrTimeout ack_or_timeout);
+
+  // Order of fields below is important for destructing them in the right order.
+  // - |heartbeat_sender_| and |signaling_connector_| have to be destructed
+  //   before |signal_strategy_| because their destructors need to call
+  //   signal_strategy_->RemoveListener(this)
+  // - |signaling_connector_| has to be destructed before
+  //   |network_change_notifier_| because its destructor needs to deregister
+  //   network change notifications
+  // - |network_task_runner_| is used by all the other fields and has to be
+  //   destructed last.
+  scoped_refptr<AutoThreadTaskRunner> network_task_runner_;
+  scoped_ptr<net::NetworkChangeNotifier> network_change_notifier_;
+  scoped_ptr<XmppSignalStrategy> signal_strategy_;
+  scoped_ptr<SignalingConnector> signaling_connector_;
+  scoped_ptr<HeartbeatSender> heartbeat_sender_;
+
+  DISALLOW_COPY_AND_ASSIGN(MinimumHeartbeatSupporter);
+};
+
+}  // namespace remoting
+
+#endif  // REMOTING_HOST_MINIMUM_HEARTBEAT_SUPPORTER_H_