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 "base/time/time.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;
+
+// Keeping an instance of MinimumHeartbeatSupporter alive
+// 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::RefCounted<MinimumHeartbeatSupporter> {
+ public:
+  static scoped_refptr<MinimumHeartbeatSupporter> Create(

[Lambros, 2014/12/02 03:35:23]

This feels like too many parameters to one method, which goes against Chromium
style. Could we simplify this interface?
For example, |host_id| and |key_pair| are used only to construct a
HeartbeatSender. Could the caller create the HeartbeatSender and pass that in?

[Łukasz Anforowicz, 2014/12/02 20:08:01]

Can you comment on the things below please?:

1. I can introduce a MinimumHeartbeatSupporter::Callbacks struct that holds all
the 3 callbacks.  This will help reduce the number of parameters.  OTOH, I feel
that it just adds noise elsewhere (extra struct declaration + definition and
filling out by the caller).

2. The caller could in theory create some of the objects here (i.e.
HeartbeatSender), but they are closely coupled (i.e. to construct
HeartbeatSender one needs to have a SignalStrategy) and I like how the current
changelist keeps everything inside the static Create method.

3. Maybe it is possible to combine configuration values into a bigger, coherent
bundle, but I am not sure how to do it:
- talkgadget_prefix comes from policy
(HostProcess::OnHostTalkGadgetPrefixPolicyUpdate)
- host_id, key_pair, use_service_account comes from config
(HostProcess::ApplyConfig)
- oauth_refresh_token is read from a path specified in config
- directory_bot_jid comes from commandline (HostProcess::InitWithCommandLine)

I am also guessing that Chromium style frowns upon too many fields and one can
treat the current changelist as simply exposing presence of too many fields and
responsibilities inside HostProcess.  It feels that there is one or more class
hiding inside HostProcess that are waiting to be refactored out
(HostConfigAndPolicyAndCommandLineValues?) but I am not quite sure how to
cleanly do this refactoring + I would rather do this refactoring as a separate
changelist.

I think my proposal is to just leave a TODO here.  What do you think?

[Lambros, 2014/12/03 03:20:25]

I think a TODO: is fine for now - I accept that refactoring is beyond the scope
of this CL.

+      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<RsaKeyPair> key_pair,

[Lambros, 2014/12/02 03:35:23]

It is strange to have 'const' with a pass-by-value parameter. Either lose the
'const', or pass by const-reference.

[Łukasz Anforowicz, 2014/12/02 20:08:01]

Good catch - thanks.  I blame it on:
1. My stupidity - trying to consistently, aesthetically have consts on all
arguments, without thinking about details of what I am doing
2. Just copying the declaration from constructor of HeartbeatSender - it has
identical parameter declaration with const applied to scoped_refptr (introduced
by
https://chromium.googlesource.com/chromium/src/+/8f1504bf1badb27cff0f1a71886f...).

I see that HeartbeatSender only calls RsaKeyPair::SignMessage which is a const
method - therefore I'll do two things:
A. Convert the param into a const reference (i.e. s/const scoped_refptr<T>/const
scoped_refptr<T>&/ ) both in HeartbeatSender and in MinimumHeartbeatSupporter.
B. Enforce constness of RsaKeyPair by changing
s/scoped_refptr<T>/scoped_refptr<const T>/). (without this HeartbeatSender can
call non-const functions of RsaKeyPair).

[Lambros, 2014/12/03 03:20:24]

Yes, I think scoped_refptr should be passed around by const-reference (but
stored by value).

+      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* GetSignalStrategy();

[Lambros, 2014/12/02 03:35:23]

nit: This is a simple getter, so name it Unix-style: signal_strategy() and
inline the definition here.

[Łukasz Anforowicz, 2014/12/02 20:08:01]

Done.

+
+  // 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);
+
+  scoped_refptr<AutoThreadTaskRunner> network_task_runner_;
+  scoped_ptr<net::NetworkChangeNotifier> network_change_notifier_;

[Lambros, 2014/12/02 03:35:22]

|network_change_notifier_| is never used in this class. Can we remove it?

[Łukasz Anforowicz, 2014/12/02 20:08:01]

No - there is an implicit / magical / global dependency :-(

From
https://code.google.com/p/chromium/codesearch#chromium/src/net/base/network_c...:
  // Creates the process-wide, platform-specific NetworkChangeNotifier.  The
  // caller owns the returned pointer.  You may call this on any thread.  You
  // may also avoid creating this entirely (in which case nothing will be
  // monitored), but if you do create it, you must do so before any other
  // threads try to access the API below, and it must outlive all other threads
  // which might try to use it.
  static NetworkChangeNotifier* Create();

And our SignalingConnector uses this magical/global/implicit dependency here: 
https://code.google.com/p/chromium/codesearch#chromium/src/remoting/host/sign...
SignalingConnector::SignalingConnector(
    XmppSignalStrategy* signal_strategy,
    scoped_ptr<DnsBlackholeChecker> dns_blackhole_checker,
    const base::Closure& auth_failed_callback)
    : signal_strategy_(signal_strategy),
      auth_failed_callback_(auth_failed_callback),
      dns_blackhole_checker_(dns_blackhole_checker.Pass()),
      reconnect_attempts_(0) {
  DCHECK(!auth_failed_callback_.is_null());
  DCHECK(dns_blackhole_checker_.get());
  net::NetworkChangeNotifier::AddConnectionTypeObserver(this);
  net::NetworkChangeNotifier::AddIPAddressObserver(this);
  signal_strategy_->AddListener(this);
  ScheduleTryReconnect();
}

SignalingConnector::~SignalingConnector() {
  signal_strategy_->RemoveListener(this);
  net::NetworkChangeNotifier::RemoveConnectionTypeObserver(this);
  net::NetworkChangeNotifier::RemoveIPAddressObserver(this);
}

[Lambros, 2014/12/03 03:20:25]

Fair enough :) I guess we had this problem before, and you're just moving the
code around.

+  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_