Add HostState enum to track host process status.

remoting/host/remoting_me2me_host.cc

 // Copyright (c) 2012 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.
 //
 // This file implements a standalone host process for Me2Me.
 
 #include <string>
 
 #include "base/at_exit.h"
 #include "base/bind.h"
@@ skipping 156 matching lines @@
   virtual void OnChannelError() OVERRIDE;
 
   // HeartbeatSender::Listener overrides.
   virtual void OnUnknownHostIdError() OVERRIDE;
 
   void StartHostProcess();
 
   int get_exit_code() const;
 
  private:
+  enum HostState {

[alexeypa (please no reviews), 2012/11/20 17:00:56]

bootstrapping and restarting states are confusing. I'd aim for the following
states:

stopped,
starting,
started,
stopping

... and the transitions:

stopped -> starting
starting-> started
starting-> stopping (something like a bad config file)
starting-> stopped (error handling)
started -> starting (restart)
started -> stopping
stopping -> stopped

[Sergey Ulanov, 2012/11/20 20:02:34]

We don't really need Starting state because host startup is synchronous - we can
go from STOPPED to STARTED state in one step. Shutdown is asynchronous and
that's why we need STOPPING and RESTARTING states.
The reason we need BOOTSTRAPPING state is because we can't start the host when
the host process is launched until we read policies. So BOOTSTRAPPING means host
is stopped but it should be started as soon as we receive policies (see
OnPolicyUpdate()). STOPPED means that it's stopped and should not be started
unless config changes. You suggested to transition from STARTED to STARTING when
restarting, but that doesn't really make sense - when restarting a host we need
to stop it asynchronously and then start it again synchronously, so the state
will be set to STARTING, while the old |host_| is being stopped.


On 2012/11/20 17:00:56, alexeypa wrote:

[alexeypa (please no reviews), 2012/11/20 21:18:30]

I see you point but the state diagram still looks confusing to me. I thought
about it for a while and the reasons seems to be that we mixing the host state
and events driving the state change. For instance RESTARTING is the same thing
as STOPPING except that it was caused by Restart event, not Stop.

On the other hand WAITING_POLICIES state totally makes sense. I agree that if
starting is synchronous when STARTING state is not necessary. Though having
STARTING state might result in more structured transitions. For instance we will
not have STOPPING -> STARTED transition, which otherwise looks weird.

How about these states, events and transitions:

States:

WAITING_POLICIES
STOPPED
STARTED
STOPPING

Transitions:

1. WAITING_POLICIES -> STOPPED (Policies received)
2. STOPPED -> STOPPED (Tried to start the host, it didn't work out)
3. STOPPED -> STARTED (Config is good, the host has started)
4. STARTED -> STOPPING (Restart request, stop request)
5. STOPPING -> STARTED (Restart request, Config is good, the host has started)

/STARTING state really makes it more structured/

Such a state diagram will mean that we will need to generate events upon
entering some state. For instance when entering STOPPED state we will check
whether the host needs to be restarted. I think it is a reasonable approach that
makes transitions more intuitive.

+    // Host process has just been started.
+    HOST_BOOTSTRAPPING,

[Wez, 2012/11/20 03:44:19]

nit: HOST_STARTING

[alexeypa (please no reviews), 2012/11/20 17:00:56]

+1

[Sergey Ulanov, 2012/11/20 20:02:34]

STARTING doesn't make sense because host can be started synchronously.
BOOTSTRAPPING is different from STARTING because we never return to that state.
It's used only once while we still waiting for the policies to be read. Maybe
rename it to WAITING_POLICIES?

+
+    // Host is started and running.
+    HOST_STARTED,
+
+    // Host is being restarted.
+    HOST_RESTARTING,

[Wez, 2012/11/20 03:44:19]

Could this re-use HOST_STARTING, and we allow STARTING->STOPPING?

[Sergey Ulanov, 2012/11/20 20:02:34]

See my comments above.

+
+    // Host is being stopped.
+    HOST_STOPPING,
+
+    // Host has been stopped.
+    HOST_STOPPED,
+
+    // Allowed state transitions:
+    //   BOOTSTRAPPING->STARTED
+    //   BOOTSTRAPPING->STOPPED
+    //   STARTED->RESTARTING
+    //   STARTED->STOPPING
+    //   RESTARTING->STOPPING

[alexeypa (please no reviews), 2012/11/20 17:00:56]

nit: RESTARTING->STARTED is missing

[Sergey Ulanov, 2012/11/20 20:02:34]

Done.

+    //   STOPPING->STOPPED
+    //   STOPPED->STARTED
+    //
+    // |host_| must be NULL in BOOTSTRAPPING and STOPPED states and not-NULL in
+    // all other states.
+  };
+
 #if defined(OS_POSIX)
   // Registers a SIGTERM handler on the network thread, to shutdown the host.
   void ListenForShutdownSignal();
 
   // Callback passed to RegisterSignalHandler() to handle SIGTERM events.
   void SigTermHandler(int signal_number);
 #endif
 
   // Asks the daemon to inject Secure Attention Sequence to the console.
   void SendSasToConsole();
@@ skipping 17 matching lines @@
   void OnAuthFailed();
 
   void RejectAuthenticatingClient();
 
   // Invoked when the user uses the Disconnect windows to terminate
   // the sessions, or when the local session is activated in curtain mode.
   void OnDisconnectRequested();
 
   void RestartHost();
 
-  void RestartOnHostShutdown();
-
-  void Shutdown(int exit_code);
+  // Stops the host and shuts down the process with the specified |exit_code|.
+  void ShutdownHost(int exit_code);
 
   void OnShutdownFinished();
 
-  void ResetHost();
-
   // Crashes the process in response to a daemon's request. The daemon passes
   // the location of the code that detected the fatal error resulted in this
   // request.
   void OnCrash(const std::string& function_name,
                const std::string& file_name,
                const int& line_number);
 
   scoped_ptr<ChromotingHostContext> context_;
   scoped_ptr<IPC::ChannelProxy> daemon_channel_;
   scoped_ptr<net::NetworkChangeNotifier> network_change_notifier_;
@@ skipping 15 matching lines @@
   std::string xmpp_auth_service_;
 
   scoped_ptr<policy_hack::PolicyWatcher> policy_watcher_;
   bool allow_nat_traversal_;
   std::string talkgadget_prefix_;
 
   scoped_ptr<CurtainMode> curtain_;
   scoped_ptr<CurtainingHostObserver> curtaining_host_observer_;
   bool curtain_required_;
 
-  bool restarting_;
-  bool shutting_down_;
+  HostState state_;
 
   scoped_ptr<DesktopEnvironmentFactory> desktop_environment_factory_;
   scoped_ptr<DesktopResizer> desktop_resizer_;
   scoped_ptr<ResizingHostObserver> resizing_host_observer_;
   scoped_ptr<XmppSignalStrategy> signal_strategy_;
   scoped_ptr<SignalingConnector> signaling_connector_;
   scoped_ptr<HeartbeatSender> heartbeat_sender_;
   scoped_ptr<LogToServer> log_to_server_;
   scoped_ptr<HostEventLogger> host_event_logger_;
 
   scoped_ptr<HostUserInterface> host_user_interface_;
 
   scoped_refptr<ChromotingHost> host_;
 
 #if defined(REMOTING_MULTI_PROCESS)
   DesktopSessionConnector* desktop_session_connector_;
 #endif  // defined(REMOTING_MULTI_PROCESS)
 
   int exit_code_;
 };
 
 HostProcess::HostProcess(scoped_ptr<ChromotingHostContext> context)
     : context_(context.Pass()),
       allow_nat_traversal_(true),
       curtain_required_(false),
-      restarting_(false),
-      shutting_down_(false),
+      state_(HOST_BOOTSTRAPPING),
       desktop_resizer_(DesktopResizer::Create()),
 #if defined(REMOTING_MULTI_PROCESS)
       desktop_session_connector_(NULL),
 #endif  // defined(REMOTING_MULTI_PROCESS)
       exit_code_(kSuccessExitCode) {
   network_change_notifier_.reset(net::NetworkChangeNotifier::Create());
   curtain_ = CurtainMode::Create(
       base::Bind(&HostProcess::OnDisconnectRequested,
                  base::Unretained(this)),
       base::Bind(&HostProcess::RejectAuthenticatingClient,
@@ skipping 60 matching lines @@
   // Filter out duplicates.
   if (serialized_config_ == serialized_config)
     return;
 
   LOG(INFO) << "Processing new host configuration.";
 
   serialized_config_ = serialized_config;
   scoped_ptr<JsonHostConfig> config(new JsonHostConfig(FilePath()));
   if (!config->SetSerializedData(serialized_config)) {
     LOG(ERROR) << "Invalid configuration.";
-    Shutdown(kInvalidHostConfigurationExitCode);
+    ShutdownHost(kInvalidHostConfigurationExitCode);
     return;
   }
 
   if (!ApplyConfig(config.Pass())) {
     LOG(ERROR) << "Failed to apply the configuration.";
-    Shutdown(kInvalidHostConfigurationExitCode);
+    ShutdownHost(kInvalidHostConfigurationExitCode);
     return;
   }
 
   // Start watching the policy (and eventually start the host) if this is
   // the first configuration update. Otherwise, create new authenticator
   // factory in case PIN has changed.
   if (!policy_watcher_) {
     policy_watcher_.reset(
         policy_hack::PolicyWatcher::Create(context_->file_task_runner()));
     policy_watcher_->StartWatching(
         base::Bind(&HostProcess::OnPolicyUpdate, base::Unretained(this)));
   } else {
     CreateAuthenticatorFactory();
   }
 }
 
 void HostProcess::OnConfigWatcherError() {
   DCHECK(context_->ui_task_runner()->BelongsToCurrentThread());
 
   context_->network_task_runner()->PostTask(
       FROM_HERE,
-      base::Bind(&HostProcess::Shutdown, base::Unretained(this),
+      base::Bind(&HostProcess::ShutdownHost, base::Unretained(this),
                  kInvalidHostConfigurationExitCode));
 }
 
 void HostProcess::StartWatchingConfigChanges() {
 #if !defined(REMOTING_MULTI_PROCESS)
     // Start watching the host configuration file.
     config_watcher_.reset(new ConfigFileWatcher(context_->ui_task_runner(),
                                                 context_->file_task_runner(),
                                                 this));
     config_watcher_->Watch(host_config_path_);
 #endif  // !defined(REMOTING_MULTI_PROCESS)
 }
 
 #if defined(OS_POSIX)
 void HostProcess::ListenForShutdownSignal() {
   DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
 
   remoting::RegisterSignalHandler(
       SIGTERM,
       base::Bind(&HostProcess::SigTermHandler, base::Unretained(this)));
 }
 
 void HostProcess::SigTermHandler(int signal_number) {
   DCHECK(signal_number == SIGTERM);
   DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
   LOG(INFO) << "Caught SIGTERM: Shutting down...";
-  Shutdown(kSuccessExitCode);
+  ShutdownHost(kSuccessExitCode);
 }
 #endif  // OS_POSIX
 
 void HostProcess::CreateAuthenticatorFactory() {
   DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
 
-  if (!host_ || shutting_down_)
+  if (state_ != HOST_STARTED)
     return;
 
   std::string local_certificate = key_pair_.GenerateCertificate();
   if (local_certificate.empty()) {
     LOG(ERROR) << "Failed to generate host certificate.";
-    Shutdown(kInitializationFailed);
+    ShutdownHost(kInitializationFailed);
     return;
   }
 
   scoped_ptr<protocol::AuthenticatorFactory> factory(
       new protocol::Me2MeHostAuthenticatorFactory(
           local_certificate, *key_pair_.private_key(), host_secret_hash_));
 #if defined(OS_LINUX)
   // On Linux, perform a PAM authorization step after authentication.
   factory.reset(new PamAuthorizationFactory(factory.Pass()));
 #endif
@@ skipping 25 matching lines @@
   return false;
 #endif  // !defined(REMOTING_MULTI_PROCESS)
 }
 
 void HostProcess::OnChannelError() {
   DCHECK(context_->ui_task_runner()->BelongsToCurrentThread());
 
   // Shutdown the host if the daemon disconnected the channel.
   context_->network_task_runner()->PostTask(
       FROM_HERE,
-      base::Bind(&HostProcess::Shutdown, base::Unretained(this),
+      base::Bind(&HostProcess::ShutdownHost, base::Unretained(this),
                  kSuccessExitCode));
 }
 
 void HostProcess::StartHostProcess() {
   DCHECK(context_->ui_task_runner()->BelongsToCurrentThread());
 
   if (!InitWithCommandLine(CommandLine::ForCurrentProcess())) {
     OnConfigWatcherError();
     return;
   }
@@ skipping 99 matching lines @@
   daemon_channel_.reset();
   desktop_environment_factory_.reset();
   host_user_interface_.reset();
 
   context_.reset();
 }
 
 // Overridden from HeartbeatSender::Listener
 void HostProcess::OnUnknownHostIdError() {
   LOG(ERROR) << "Host ID not found.";
-  Shutdown(kInvalidHostIdExitCode);
+  ShutdownHost(kInvalidHostIdExitCode);
 }
 
 // Applies the host config, returning true if successful.
 bool HostProcess::ApplyConfig(scoped_ptr<JsonHostConfig> config) {
   DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
 
   if (!config->GetString(kHostIdConfigPath, &host_id_)) {
     LOG(ERROR) << "host_id is not defined in the config.";
     return false;
   }
@@ skipping 63 matching lines @@
   if (policies->GetString(
           policy_hack::PolicyWatcher::kHostTalkGadgetPrefixPolicyName,
           &string_value)) {
     restart_required |= OnHostTalkGadgetPrefixPolicyUpdate(string_value);
   }
   if (policies->GetBoolean(
           policy_hack::PolicyWatcher::kHostRequireCurtainPolicyName,
           &bool_value)) {
       restart_required |= OnCurtainPolicyUpdate(bool_value);
   }
-  if (!host_) {
+
+  if (state_ == HOST_BOOTSTRAPPING) {
     StartHost();
-  } else if (restart_required) {
+  } else if (state_ == HOST_STARTED) {
     RestartHost();
   }
 }
 
 bool HostProcess::OnHostDomainPolicyUpdate(const std::string& host_domain) {
   // Returns true if the host has to be restarted after this policy update.
   DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
 
   LOG(INFO) << "Policy sets host domain: " << host_domain;
 
   if (!host_domain.empty() &&
       !EndsWith(xmpp_login_, std::string("@") + host_domain, false)) {
-    Shutdown(kInvalidHostDomainExitCode);
+    ShutdownHost(kInvalidHostDomainExitCode);
   }
   return false;
 }
 
 bool HostProcess::OnUsernamePolicyUpdate(bool host_username_match_required) {
   // Returns false: never restart the host after this policy update.
   DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
 
   if (host_username_match_required) {
     LOG(INFO) << "Policy requires host username match.";
     if (!CanGetUsername() ||
         !StartsWithASCII(xmpp_login_, GetUsername() + std::string("@"),
                          false)) {
-      Shutdown(kUsernameMismatchExitCode);
+      ShutdownHost(kUsernameMismatchExitCode);
     }
   } else {
     LOG(INFO) << "Policy does not require host username match.";
   }
 
   return false;
 }
 
 bool HostProcess::OnNatPolicyUpdate(bool nat_traversal_enabled) {
   // Returns true if the host has to be restarted after this policy update.
@@ skipping 19 matching lines @@
     // When curtain mode is in effect on Mac, the host process runs in the
     // user's switched-out session, but launchd will also run an instance at
     // the console login screen.  Even if no user is currently logged-on, we
     // can't support remote-access to the login screen because the current host
     // process model disconnects the client during login, which would leave
     // the logged in session un-curtained on the console until they reconnect.
     //
     // TODO(jamiewalch): Fix this once we have implemented the multi-process
     // daemon architecture (crbug.com/134894)
     if (getuid() == 0) {
-      Shutdown(kLoginScreenNotSupportedExitCode);
+      ShutdownHost(kLoginScreenNotSupportedExitCode);
       return false;
     }
   }
 #endif
 
   if (curtain_required_ != curtain_required) {
     if (curtain_required)
       LOG(ERROR) << "Policy requires curtain-mode.";
     else
       LOG(ERROR) << "Policy does not require curtain-mode.";
@@ skipping 20 matching lines @@
 
 void HostProcess::StartHostStatusService() {
   DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
   status_service_.reset(new HostStatusService());
 }
 
 void HostProcess::StartHost() {
   DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
   DCHECK(!host_);
   DCHECK(!signal_strategy_.get());
-
-  if (shutting_down_)
-    return;
+  DCHECK(state_ == HOST_BOOTSTRAPPING || state_ == HOST_RESTARTING ||
+         state_ == HOST_STOPPED) << state_;
+  state_ = HOST_STARTED;
 
   signal_strategy_.reset(
       new XmppSignalStrategy(context_->url_request_context_getter(),
                              xmpp_login_, xmpp_auth_token_,
                              xmpp_auth_service_));
 
   scoped_ptr<DnsBlackholeChecker> dns_blackhole_checker(
       new DnsBlackholeChecker(context_->url_request_context_getter(),
                               talkgadget_prefix_));
 
@@ skipping 63 matching lines @@
 
   if (status_service_)
     status_service_->SetHostIsUp(host_id_);
 
   host_->Start(xmpp_login_);
 
   CreateAuthenticatorFactory();
 }
 
 void HostProcess::OnAuthFailed() {
-  Shutdown(kInvalidOauthCredentialsExitCode);
+  ShutdownHost(kInvalidOauthCredentialsExitCode);
 }
 
 void HostProcess::RejectAuthenticatingClient() {
   DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
   DCHECK(host_);
   host_->RejectAuthenticatingClient();
 }
 
 // Invoked when the user uses the Disconnect windows to terminate
 // the sessions, or when the local session is activated in curtain mode.
 void HostProcess::OnDisconnectRequested() {
   if (!context_->network_task_runner()->BelongsToCurrentThread()) {
     context_->network_task_runner()->PostTask(FROM_HERE, base::Bind(
         &HostProcess::OnDisconnectRequested, base::Unretained(this)));
     return;
   }
   if (host_) {
     host_->DisconnectAllClients();
   }
 }
 
 void HostProcess::RestartHost() {
   DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
+  DCHECK_EQ(state_, HOST_STARTED)
 
-  if (restarting_ || shutting_down_)
-    return;
-
-  restarting_ = true;
+  state_ = HOST_RESTARTING;
   host_->Shutdown(base::Bind(
-      &HostProcess::RestartOnHostShutdown, base::Unretained(this)));
+      &HostProcess::OnShutdownFinished, base::Unretained(this)));
 }
 
-void HostProcess::RestartOnHostShutdown() {
+void HostProcess::ShutdownHost(int exit_code) {

[Wez, 2012/11/20 03:44:19]

nit: ShutdownHost -> Shutdown - this doesn't just shutdown the host, it shuts
down the hold process.

[Sergey Ulanov, 2012/11/20 20:02:34]

See my previous comments about not shutting down the whole process in the
future.

   DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
 
-  if (shutting_down_)
-    return;
+  exit_code_ = exit_code;
 
-  restarting_ = false;
-  host_ = NULL;
-  ResetHost();
+  switch (state_) {
+    case HOST_BOOTSTRAPPING:
+      state_ = HOST_STOPPED;
+      OnShutdownFinished();
+      break;
 
-  StartHost();
-}
+    case HOST_STARTED:
+      if (status_service_)
+        status_service_->SetHostIsDown();
+      host_->Shutdown(base::Bind(
+        &HostProcess::OnShutdownFinished, base::Unretained(this)));
+      state_ = HOST_STOPPING;
+      break;
 
-void HostProcess::Shutdown(int exit_code) {
-  DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
+    case HOST_RESTARTING:
+      state_ = HOST_STOPPING;
+      break;
 
-  if (shutting_down_)
-    return;
-
-  if (status_service_)
-    status_service_->SetHostIsDown();
-
-  shutting_down_ = true;
-  exit_code_ = exit_code;
-  if (host_) {
-    host_->Shutdown(base::Bind(
-        &HostProcess::OnShutdownFinished, base::Unretained(this)));
-  } else {
-    OnShutdownFinished();
+    case HOST_STOPPING:
+    case HOST_STOPPED:
+      // Host is already stopped or being stopped. No action is required.
+      break;
   }
 }
 
 void HostProcess::OnShutdownFinished() {
   DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
 
-  // Destroy networking objects while we are on the network thread.
   host_ = NULL;
-  ResetHost();
-
-  if (policy_watcher_.get()) {
-    base::WaitableEvent done_event(true, false);
-    policy_watcher_->StopWatching(&done_event);
-    done_event.Wait();
-    policy_watcher_.reset();
-  }
-
-  status_service_.reset();
-
-  // Complete the rest of shutdown on the main thread.
-  context_->ui_task_runner()->PostTask(
-      FROM_HERE,
-      base::Bind(&HostProcess::ShutdownHostProcess,
-                 base::Unretained(this)));
-}
-
-void HostProcess::ResetHost() {
-  DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
-
   curtaining_host_observer_.reset();
   host_event_logger_.reset();
   log_to_server_.reset();
   heartbeat_sender_.reset();
   signaling_connector_.reset();
   signal_strategy_.reset();
   resizing_host_observer_.reset();
+
+  if (state_ == HOST_RESTARTING) {
+    StartHost();
+  } else if (state_ == HOST_STOPPING) {
+    state_ = HOST_STOPPED;
+
+    if (policy_watcher_.get()) {
+      base::WaitableEvent done_event(true, false);
+      policy_watcher_->StopWatching(&done_event);
+      done_event.Wait();
+      policy_watcher_.reset();
+    }
+
+    status_service_.reset();
+
+    // Complete the rest of shutdown on the main thread.
+    context_->ui_task_runner()->PostTask(
+        FROM_HERE,
+        base::Bind(&HostProcess::ShutdownHostProcess,
+                   base::Unretained(this)));
+  } else {
+    // This method is used as a callback for ChromotingHost::Shutdown() which is
+    // called only in RESTARTING and STOPPING states.
+    NOTREACHED();
+  }
 }
 
 void HostProcess::OnCrash(const std::string& function_name,
                           const std::string& file_name,
                           const int& line_number) {
   CHECK(false);
 }
 
 }  // namespace remoting
 
@@ skipping 80 matching lines @@
             user32.GetFunctionPointer("SetProcessDPIAware"));
     set_process_dpi_aware();
   }
 
   // CommandLine::Init() ignores the passed |argc| and |argv| on Windows getting
   // the command line from GetCommandLineW(), so we can safely pass NULL here.
   return main(0, NULL);
 }
 
 #endif  // defined(OS_WIN)