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

remoting/host/host_signaling_manager.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_HOST_SIGNALING_MANAGER_H_
+#define REMOTING_HOST_HOST_SIGNALING_MANAGER_H_
+
+#include <string>
+
+#include "base/callback.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/heartbeat_sender.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;
+
+// HostSignalingManager has 2 functions:
+// 1. Keep sending regular heartbeats to the service.

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

You mean to the Chromoting Directory, I think?

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

To Talk, Chromoting Directory and Chromoting Bot.

[Wez, 2014/12/19 22:05:37]

We only send the heartbeat to the directory - it just happens to pass via Talk
and the Bot.

My point is that "service" is very vague and could, e.g. under Windows, be
confused with the wrong concept.

[Łukasz Anforowicz, 2015/01/07 01:24:04]

Got it - done (s/service/Chromoting Directory/).

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

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

nit: The second function is to make a best-effort to deliver the shutdown reason
to the cloud - the need for keeping the process alive is just an implementation
detail.

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

I think I'll leave it as-is.  The ref-counting trick to keep the host process
alive is something that found very useful but not obvious initially - I think it
is useful to call this aspect of the implementation explicitly.  Additionally -
the main reason why I am grouping the signaling/heartbeat-objects together is so
I can keep them alive together while the process's life is extended and
everything else is dead or dying.  If I didn't need to keep those objects
together then I wouldn't have HostSignalingManager.

[Wez, 2014/12/19 22:05:37]

Acknowledged.

+class HostSignalingManager {
+ 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/18 00:46:21]

The parameters need documenting.

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

I documented some of the parameters.  

I didn't document the following:
- host_context - self-explanatory / comment would be reduntant?
- xmpp_server_config - self-explanatory / comment would be reduntant??
- host_id - self-explanatory / comment would be reduntant??
- oauth_refresh_token - self-explanatory / comment would be reduntant??

- use_service_account - I have no idea what this means / this is not documented
in remoting_me2me_host.cc and in oauth_token_getter.h/cc :-(  I added a shallow
comment that nevertheless I hope helps here: // Controls which API key gets
used.

I just renamed (without adding a comment - I hope the name change is sufficient
to make these parameters easier to understand)
- key_pair to host_key_pair (also renaming in HeartbeatSender but not elsewhere)
- talkgadget_prefix to talkgadget_prefix_policy (without renaming other
occurences - i.e. in HostProcess).  I am not sure what this does exactly.  This
seems to be part of a hostname in a http url that DnsBlackholeChecker tries to
talk to.  I am not sure why this needs to be controlled by a policy or what this
is useful for :-(

[Wez, 2014/12/19 22:05:37]

Please document the paremeters in the Create() method's block comment, not
in-line.

[Łukasz Anforowicz, 2015/01/07 01:24:04]

Done.

+  static scoped_ptr<HostSignalingManager> Create(
+      const base::Closure& on_heartbeat_successful_callback,
+      const base::Closure& on_unknown_host_id_error_callback,
+      const base::Closure& on_auth_failed_callback,

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

As previously commented, these should be combined into a
HostSignalingManager::EventHandler interface.

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

Nooooo.  As I pointed out earlier, this results in icky forwarding code in
HostSignalingManager + necessity for an explicit listener/eventhandler
deregistration.  With callbacks, HostSignalingManager::Create can just treat the
callbacks as a blackbox/opaque things and pass them to HeartbeatSender and
SignalingConnector.  With the event handler interface we have to hardwire the
listener logic into HostSignalingManager.  You can compare
minimum_heartbeat_supporter.h/.cc and remoting_me2me_host.cc between patchset #1
and patchset #2 - I think things are much cleaner in patchset #2.

[Wez, 2014/12/19 22:05:37]

I disagree; the callback-based implementation has a little less boilerplate but
is not cleaner.

With the callbacks you have more parameters to Create(), making the call more
cumbersome, and the semantics of those callback with respect to the lifetime of
the HostSignalingManager are non-obvious.

With an EventHandler interface the HostSignalingManager can
Bind(&EventHandler::Foo, weak_event_handler_) using a WeakPtrFactory initialized
with the EventHandler supplied to Create(). You can then define that the
EventHandler will stop receiving notifications when either the
HostSignalingManager is deleted directly, or SendHostOfflineReasonAndDelete() is
called.

[Łukasz Anforowicz, 2015/01/07 01:24:04]

I think the lifetime semantics are cleaner with callbacks:
- consumers of the callbacks (HostSignalingManager, HeartbeatSender,
SignalingConnector) assume that the callbacks are valid/alive forever
- creator of the callbacks (HostProcess) ensure the callbacks are safe wrt the
lifetime of the creator (i.e. the creator is responsible to force the callbacks
into no-ops before the creator gets deleted);  

I also like that the lifetime controlling by creator is done in a way that
doesn't have to know who consumes the callbacks (i.e. the creator is not coupled
with consumers wrt managing the lifetime of the callbacks).
This sounds like a lot of magic that users of HostSignalingManager have to know,
before they can confidently/safely use the listener interface.


Maybe we can chat about this (this did help convince me that
ack_or_timeout_reporter is a bad idea - maybe this will also be most efficient
in this case).

[Wez, 2015/01/09 02:55:45]

Not really; it becomes a perfectly straightforward contract that the interface
you pass in must remain valid until the HostSignalingManager is destroyed, or
told to delete itself. 

That's much more explicit that passing in a set of callbacks that may or may not
still be called after you've deleted this object.

[Łukasz Anforowicz, 2015/01/09 19:00:13]

Ok.

+      const ChromotingHostContext& host_context,

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

Ick... do you really need this? The ChromotingHostContext is really only
intended as a convenience for creating the necessary threads, not to be passed
around into other classes.

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

I think it is ok / not icky...
1. HostSignalingManager doesn't take ownership or mutate ChromotingHostContext
(i.e. gets it via const ref).
2. This helps minimize the number of parameters.
3. It is ok for HostProcess to get the threads via ChromotingHostContext -
should be ok here as well.

[Wez, 2014/12/19 22:05:37]

HostProcess is a special-case in that it is an implementation detail of
remoting_me2me_host - this class just needs a thread to run on and a
URLRequestGetter, so passing in the entire ChromotingHostContext creates an
artificial dependency.

[Łukasz Anforowicz, 2015/01/07 01:24:04]

Done.

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

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

what's this?

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

It's a JID to use when talking to Directory Bot.  I thought that this is covered
just by the name of the parameter.  Anyway - I tried to improve this by adding a
comment with an example:
      // Directory Bot's JID - i.e. "remoting@bot.talk.google.com".

[Wez, 2014/12/19 22:05:37]

Right; the question is not _what_ is the parameter, which the name covers, but
_why_ is it here? Why is it not just a constant in the implementation?

[Łukasz Anforowicz, 2015/01/07 01:24:04]

Got it.  As Feynman says (https://www.youtube.com/watch?v=36GT2zI8lVA) "why"
questions are inherently difficult to answer :-)

Answer#1: It's here because it is overridable on the command line (i.e. see
https://code.google.com/p/chromium/codesearch#chromium/src/remoting/base/serv...).
 
Answer#2: It's here to enable overriding it when testing against locally running
talk/bot (i.e. see "host" section here:
https://wiki.corp.google.com/twiki/bin/view/Main/RunningChromotingServersLocally).

I don't think any of that needs to go into comments though, right?  I'll leave
them as-is for now.

+      const std::string& oauth_refresh_token,
+      bool use_service_account);

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

What does "use_service_account" mean?

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

I don't know - I am just moving an already working code from HostProcess :-( 
remoting_me2_me_host.cc also didn't have any comment on the field.  This gets
passed to constructor of OAuthTokenGetter::OAuthCredentials struct (and doesn't
have a comment/explanation there either.  This is the code where it ends up used
(in oauth_token_getter.cc):

  // Service accounts use different API keys, as they use the client app flow.
  google_apis::OAuth2Client oauth2_client =
      oauth_credentials_->is_service_account ?
      google_apis::CLIENT_REMOTING_HOST : google_apis::CLIENT_REMOTING;

[Wez, 2014/12/19 22:05:37]

Yeah, that parameter is super badly documented; your comment is helpful here,
IMO - suggest elaborating e.g. "True if the OAuth refresh token is for a service
account, False for a user account, to allow the correct client-ID to be used."

[Łukasz Anforowicz, 2015/01/07 01:24:04]

I've added the comment you suggested to the constructor of OAuthCredentials
in oauth_token_getter.h

+
+  ~HostSignalingManager();
+
+  // Gets signal strategy used for talking to the Chromoting bot.

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

"Get the SignalStrategy to use to talk to..."

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

Done.

+  // Return value is valid until |this| gets deleted.

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

nit: Suggest: "The SignalStrategy remains owned by the HostSignalingManager."

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

Done.

+  SignalStrategy* signal_strategy() { return signal_strategy_.get(); }
+
+  // Kicks off sending a heartbeat containing a host-offline-reason attribute.
+  //
+  // Will delete |this| once either the bot acks receiving the
+  // |host_offline_reason|, or the |timeout| is reached.  Deleting
+  // |this| will release |network_task_runner_| and allow the host
+  // process to exit.

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

You don't need the sentence re the task-runner.

The AutoThreadTaskRunner behaviour is implicitly relied upon by various bits of
Chromoting, and we don't describe it everywhere; this class' responsibility is
to keep itself alive until the offline reason is ack'd or times out, and it's
the caller's responsibility to maintain the TaskRunners and process until it's
done.

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

...
This is not true.  The caller (HostProcess) shuts down and drops its references
to all the task runners.  The
HostSignalingManager::SendHostOfflineReasonAndDelete is the only thing at this
point that keeps the process alive.

The extra comment doesn't hurt, right?  If the comment is not true, then the
whole HostSignalingManager / SendHostOfflineReason is broken so it is unlikely
for the comment to get out of sync with the code, right?

[Wez, 2014/12/19 22:05:37]

Acknowledged.

+  void SendHostOfflineReasonAndDelete(const std::string& host_offline_reason,
+                                      const base::TimeDelta& timeout);
+
+ private:
+  HostSignalingManager(
+      scoped_refptr<AutoThreadTaskRunner> network_task_runner,
+      scoped_ptr<net::NetworkChangeNotifier> network_change_notifier,
+      scoped_ptr<SignalStrategy> signal_strategy,
+      scoped_ptr<SignalingConnector> signaling_connector,
+      scoped_ptr<HeartbeatSender> heartbeat_sender);
+
+  void OnAckOrTimeout(HeartbeatSender::AckOrTimeout ack_or_timeout);
+
+  // Order of fields below is important for destructing them in the right order.

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

That cannot be true of |network_task_runner_|, so move it out of the commented
block.

It would probably be clearer to do the teardown explicitly in the destructor,
with a comment before each .reset() to explain the ordering.

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

Arrrrgh... I had the teardown done explicitly in the destructor, but moved it
here in response to code review feedback from Lambros :-)  I'll leave it in as
is for now.

+  // - |heartbeat_sender_| and |signaling_connector_| have to be destructed

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

s/destructed/destroyed/

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

Done.

+  //   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.

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

This comment doesn't make sense, since all you're doing is dropping a reference,
not destroying it.

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

Dropping a ref-count to network_task_runner will drop a ref-count to
ui_task_runner (held by the cleanup/stop/join callback of network_task_runner)
and can therefore shutdown the process prematurely (although I understand that
this will not somehow magically destroy the thread the destructor is running on;
 the process shutdown might though, right?  or still wrong?).

[Wez, 2014/12/19 22:05:37]

That case should be safe - the AutoThreadTaskRunner will PostTask() the
|stop_task| it was initialised with to the underlying thread's TaskRunner - in
the case of AutoThread, the stop_task then posts to the owning thread to join()
it, before dropping the reference to it.

Because threads have SequentialTaskRunners, you're guaranteed that the tasks
leading to process shutdown won't get processed until after you've finished
working here, and returned control to your thread's MessageLoop to process those
tasks.

[Łukasz Anforowicz, 2015/01/07 01:24:04]

Okay, I think I get it - dropping the reference will post join to the other
thread, but the task_runner wrapped by AutoThreadTaskRunner can still be used
just fine, even after AutoThreadTaskRunner is dead and a join is pending on the
other thread.

I'll remove the comment.

[Wez, 2015/01/09 02:55:45]

Yes; sounds like we could have explained that more clearly in the AutoThread()
implementation.

[Łukasz Anforowicz, 2015/01/09 19:00:13]

Acknowledged.

+  scoped_refptr<AutoThreadTaskRunner> network_task_runner_;

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

You need to clarify what this task-runner is used for, i.e. the threading
properties of the class.

Looking at the implementation, you basically use it just to DCHECK that this
class is always created, used & destroyed from the same thread, and to
initialise the underlying signaling object - why not just use
ThreadTaskRunnerHandle::Get() to grab the calling thread's |task_runner| in your
constructor, so the caller needn't pass it in explicitly? Or use
base::NonThreadSafe as a base-class and do checks based on that, and just use
ThreadTaskRunnerHandle::Get() to get the TaskRunner to pass to the underlying
signaling object.

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

This field keeps the process alive.  I've added a comment.

+  scoped_ptr<net::NetworkChangeNotifier> network_change_notifier_;
+  scoped_ptr<SignalStrategy> signal_strategy_;
+  scoped_ptr<SignalingConnector> signaling_connector_;
+  scoped_ptr<HeartbeatSender> heartbeat_sender_;
+
+  // HostSignalingManager object owns itself via |self_| field while
+  // SendHostOfflineReasonAndDelete is waiting for an ack or timeout.
+  HostSignalingManager* self_;

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

This is a bare pointer so it doesn't have any effect on the lifetime of
anything, surely?

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

Yes, I don't know what I was thinking... :-(

[Wez, 2014/12/19 22:05:37]

Yeah, we all have code-review moments like that. :D

[Łukasz Anforowicz, 2015/01/07 01:24:04]

Acknowledged.

+
+  DISALLOW_COPY_AND_ASSIGN(HostSignalingManager);
+};
+
+}  // namespace remoting
+
+#endif  // REMOTING_HOST_HOST_SIGNALING_MANAGER_H_