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

remoting/host/minimum_heartbeat_supporter.cc

+// 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.
+
+#include "remoting/host/minimum_heartbeat_supporter.h"
+
+#include "base/bind.h"
+#include "base/time/time.h"
+#include "net/base/network_change_notifier.h"
+#include "net/socket/client_socket_factory.h"
+#include "remoting/base/auto_thread_task_runner.h"
+#include "remoting/base/logging.h"
+#include "remoting/base/url_request_context_getter.h"
+#include "remoting/host/ack_or_timeout_reporter.h"
+#include "remoting/host/chromoting_host_context.h"
+#include "remoting/host/dns_blackhole_checker.h"
+#include "remoting/host/heartbeat_sender.h"
+#include "remoting/host/signaling_connector.h"
+#include "remoting/signaling/xmpp_signal_strategy.h"
+
+namespace remoting {
+
+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)
+    : network_task_runner_(network_task_runner),
+      network_change_notifier_(network_change_notifier.Pass()),
+      signal_strategy_(signal_strategy.Pass()),
+      signaling_connector_(signaling_connector.Pass()),
+      heartbeat_sender_(heartbeat_sender.Pass()) {
+}
+
+scoped_refptr<MinimumHeartbeatSupporter> 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<RsaKeyPair> key_pair,
+    const std::string& directory_bot_jid,
+    const std::string& oauth_refresh_token,
+    bool use_service_account) {
+  DCHECK(host_context.network_task_runner()->BelongsToCurrentThread());
+
+  // Create a NetworkChangeNotifier for use by the signaling connector.
+  scoped_ptr<net::NetworkChangeNotifier> network_change_notifier(
+      net::NetworkChangeNotifier::Create());
+
+  scoped_ptr<XmppSignalStrategy> signal_strategy(
+      new XmppSignalStrategy(
+          net::ClientSocketFactory::GetDefaultFactory(),
+          host_context.url_request_context_getter(),
+          xmpp_server_config));
+
+  scoped_ptr<DnsBlackholeChecker> dns_blackhole_checker(
+      new DnsBlackholeChecker(
+          host_context.url_request_context_getter(),
+          talkgadget_prefix));
+
+  scoped_ptr<SignalingConnector> signaling_connector(
+      new SignalingConnector(
+        signal_strategy.get(),
+        dns_blackhole_checker.Pass(),
+        on_auth_failed_callback));
+
+  if (!oauth_refresh_token.empty()) {
+    scoped_ptr<OAuthTokenGetter::OAuthCredentials> oauth_credentials(
+        new OAuthTokenGetter::OAuthCredentials(
+            xmpp_server_config.username,
+            oauth_refresh_token,
+            use_service_account));
+
+    scoped_ptr<OAuthTokenGetter> oauth_token_getter(
+        new OAuthTokenGetter(
+            oauth_credentials.Pass(),
+            host_context.url_request_context_getter(),
+            false));
+
+    signaling_connector->EnableOAuth(oauth_token_getter.Pass());
+  }
+
+  scoped_ptr<HeartbeatSender> heartbeat_sender(
+      new HeartbeatSender(
+          on_heartbeat_successful_callback,
+          on_unknown_host_id_error_callback,
+          host_id,
+          signal_strategy.get(),
+          key_pair,
+          directory_bot_jid));
+
+  return new MinimumHeartbeatSupporter(
+          host_context.network_task_runner(),
+          network_change_notifier.Pass(),
+          signal_strategy.Pass(),
+          signaling_connector.Pass(),
+          heartbeat_sender.Pass());
+}
+
+MinimumHeartbeatSupporter::~MinimumHeartbeatSupporter() {
+  DCHECK(network_task_runner_->BelongsToCurrentThread());
+
+  // Order of destroying fields below is important.

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

Rather than manually calling reset(), why not just order them correctly in the
header file? C++ guarantees to destroy these objects in reverse order. And
please add a comment explaining what the dependencies actually are?

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

Good point on the explicit comment - thanks for pointing this out.  Documenting
this in a comment is especially good now, when all of the 4 collaborating
objects are embedded inside MinimumHeartbeatSupporter (meaning that hopefully
the comment is unlikely to get out of sync with the implementation).

Hmmm... I guess I can remove the explicit resets...  My initial thinking was
that having an explicit destructor with explicit order of resets was more
straightforward and more easily readable by future maintainers.  But I already
saw some other classes with important order of field declarations, so I guess it
is ok to do it this way.

I think I still want an explicit destructor - both the assert and HOST_LOG seem
desirable.

+  heartbeat_sender_.reset();
+  signaling_connector_.reset();
+  signal_strategy_.reset();
+  network_change_notifier_.reset();
+
+  HOST_LOG << "MinimumHeartbeatSupporter is ready to die "

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

If you do need to log this, you could maybe simplify it to
"MinimumHeartbeatSupporter destroyed."

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

I think I do want to log this to aid in investigating "why the host didn't exit"
question.  Presence of this log will indicate that this is not
MinimumHeartbeatSupporter's fault.

I will simplify the log message.

+           << "and allow the host process to exit";
+}
+
+SignalStrategy* MinimumHeartbeatSupporter::GetSignalStrategy() {
+  return signal_strategy_.get();
+}
+
+void MinimumHeartbeatSupporter::SendHostOfflineReason(
+    const std::string& host_offline_reason,
+    const base::TimeDelta& timeout) {
+  DCHECK(network_task_runner_->BelongsToCurrentThread());
+
+  HOST_LOG << "SendHostOfflineReason: trying to send "
+           << host_offline_reason
+           << " to the bot";
+
+  ReportAckOrTimeout(
+      // Passing partially bound |SetHostOfflineReason| as |function_that_acks|
+      base::Bind(
+          &HeartbeatSender::SetHostOfflineReason,
+          base::Unretained(heartbeat_sender_.get()),
+          host_offline_reason),
+      timeout,
+      network_task_runner_,
+      // The callback below will keep |this| alive until either ack or timeout.
+      base::Bind(&MinimumHeartbeatSupporter::OnAckOrTimeout, this));
+}
+
+void MinimumHeartbeatSupporter::OnAckOrTimeout(AckOrTimeout ack_or_timeout)
+{
+  DCHECK(network_task_runner_->BelongsToCurrentThread());
+  switch (ack_or_timeout) {
+    case Ack:
+      HOST_LOG << "SendHostOfflineReason: got ack";
+      break;
+    case Timeout:
+      HOST_LOG << "SendHostOfflineReason: timed out";
+      break;
+    default:
+      NOTREACHED();
+      break;
+  }
+}
+
+}  // namespace remoting
+