Add HostState enum to track host process status.

remoting/host/remoting_me2me_host.cc

Index: remoting/host/remoting_me2me_host.cc
diff --git a/remoting/host/remoting_me2me_host.cc b/remoting/host/remoting_me2me_host.cc
index 883a1018e768ece4a99fedf17f77a881c11dcbd1..3796f67a08ab2e86715c10e0df2d36796998710d 100644
--- a/remoting/host/remoting_me2me_host.cc
+++ b/remoting/host/remoting_me2me_host.cc
@@ -174,6 +174,35 @@ class HostProcess
   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();
@@ -211,14 +240,11 @@ class HostProcess
 
   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.
@@ -254,8 +280,7 @@ class HostProcess
   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_;
@@ -281,8 +306,7 @@ 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),
@@ -363,13 +387,13 @@ void HostProcess::OnConfigUpdated(
   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;
   }
 
@@ -391,7 +415,7 @@ void HostProcess::OnConfigWatcherError() {
 
   context_->network_task_runner()->PostTask(
       FROM_HERE,
-      base::Bind(&HostProcess::Shutdown, base::Unretained(this),
+      base::Bind(&HostProcess::ShutdownHost, base::Unretained(this),
                  kInvalidHostConfigurationExitCode));
 }
 
@@ -418,20 +442,20 @@ 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;
   }
 
@@ -477,7 +501,7 @@ void HostProcess::OnChannelError() {
   // 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));
 }
 
@@ -597,7 +621,7 @@ void HostProcess::ShutdownHostProcess() {
 // 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.
@@ -681,9 +705,10 @@ void HostProcess::OnPolicyUpdate(scoped_ptr<base::DictionaryValue> policies) {
           &bool_value)) {
       restart_required |= OnCurtainPolicyUpdate(bool_value);
   }
-  if (!host_) {
+
+  if (state_ == HOST_BOOTSTRAPPING) {
     StartHost();
-  } else if (restart_required) {
+  } else if (state_ == HOST_STARTED) {
     RestartHost();
   }
 }
@@ -696,7 +721,7 @@ bool HostProcess::OnHostDomainPolicyUpdate(const std::string& host_domain) {
 
   if (!host_domain.empty() &&
       !EndsWith(xmpp_login_, std::string("@") + host_domain, false)) {
-    Shutdown(kInvalidHostDomainExitCode);
+    ShutdownHost(kInvalidHostDomainExitCode);
   }
   return false;
 }
@@ -710,7 +735,7 @@ bool HostProcess::OnUsernamePolicyUpdate(bool host_username_match_required) {
     if (!CanGetUsername() ||
         !StartsWithASCII(xmpp_login_, GetUsername() + std::string("@"),
                          false)) {
-      Shutdown(kUsernameMismatchExitCode);
+      ShutdownHost(kUsernameMismatchExitCode);
     }
   } else {
     LOG(INFO) << "Policy does not require host username match.";
@@ -750,7 +775,7 @@ bool HostProcess::OnCurtainPolicyUpdate(bool curtain_required) {
     // 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;
     }
   }
@@ -791,9 +816,9 @@ 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(),
@@ -877,7 +902,7 @@ void HostProcess::StartHost() {
 }
 
 void HostProcess::OnAuthFailed() {
-  Shutdown(kInvalidOauthCredentialsExitCode);
+  ShutdownHost(kInvalidOauthCredentialsExitCode);
 }
 
 void HostProcess::RejectAuthenticatingClient() {
@@ -901,73 +926,47 @@ void HostProcess::OnDisconnectRequested() {
 
 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;
-
-  restarting_ = false;
-  host_ = NULL;
-  ResetHost();
-
-  StartHost();
-}
+  exit_code_ = exit_code;
 
-void HostProcess::Shutdown(int exit_code) {
-  DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
+  switch (state_) {
+    case HOST_BOOTSTRAPPING:
+      state_ = HOST_STOPPED;
+      OnShutdownFinished();
+      break;
 
-  if (shutting_down_)
-    return;
+    case HOST_STARTED:
+      if (status_service_)
+        status_service_->SetHostIsDown();
+      host_->Shutdown(base::Bind(
+        &HostProcess::OnShutdownFinished, base::Unretained(this)));
+      state_ = HOST_STOPPING;
+      break;
 
-  if (status_service_)
-    status_service_->SetHostIsDown();
+    case HOST_RESTARTING:
+      state_ = HOST_STOPPING;
+      break;
 
-  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();
@@ -975,6 +974,31 @@ void HostProcess::ResetHost() {
   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,