Add HostState enum to track host process status. (issue 11416093)

ser...@chromium.org

unread,

Nov 19, 2012, 10:23:55 PM11/19/12

to alex...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, rmsous...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org

Reviewers: alexeypa,

Description:
Add HostState enum to track host process status.

Previously state of the host was tracked via shutting_down_ and
restarting_ flags which made state transitions hard to understand
in some case.

Please review this at https://codereview.chromium.org/11416093/

SVN Base: svn://svn.chromium.org/chrome/trunk/src

Affected files:
M remoting/host/remoting_me2me_host.cc

w...@chromium.org

unread,

Nov 19, 2012, 10:44:19 PM11/19/12

to ser...@chromium.org, alex...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, rmsous...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org

This doesn't feel to me like it makes the states any easier to
understand... ;)

https://codereview.chromium.org/11416093/diff/4001/remoting/host/remoting_me2me_host.cc
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/11416093/diff/4001/remoting/host/remoting_me2me_host.cc#newcode179
remoting/host/remoting_me2me_host.cc:179: HOST_BOOTSTRAPPING,
nit: HOST_STARTING

https://codereview.chromium.org/11416093/diff/4001/remoting/host/remoting_me2me_host.cc#newcode185
remoting/host/remoting_me2me_host.cc:185: HOST_RESTARTING,
Could this re-use HOST_STARTING, and we allow STARTING->STOPPING?

https://codereview.chromium.org/11416093/diff/4001/remoting/host/remoting_me2me_host.cc#newcode936
remoting/host/remoting_me2me_host.cc:936: void
HostProcess::ShutdownHost(int exit_code) {
nit: ShutdownHost -> Shutdown - this doesn't just shutdown the host, it
shuts down the hold process.

https://codereview.chromium.org/11416093/

alex...@chromium.org

unread,

Nov 20, 2012, 12:00:56 PM11/20/12

to ser...@chromium.org, w...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, rmsous...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org

https://codereview.chromium.org/11416093/diff/4001/remoting/host/remoting_me2me_host.cc
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/11416093/diff/4001/remoting/host/remoting_me2me_host.cc#newcode177
remoting/host/remoting_me2me_host.cc:177: enum HostState {
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

https://codereview.chromium.org/11416093/diff/4001/remoting/host/remoting_me2me_host.cc#newcode179
remoting/host/remoting_me2me_host.cc:179: HOST_BOOTSTRAPPING,

On 2012/11/20 03:44:19, Wez wrote:
> nit: HOST_STARTING

+1

https://codereview.chromium.org/11416093/diff/4001/remoting/host/remoting_me2me_host.cc#newcode198
remoting/host/remoting_me2me_host.cc:198: // RESTARTING->STOPPING
nit: RESTARTING->STARTED is missing

https://codereview.chromium.org/11416093/

ser...@chromium.org

unread,

Nov 20, 2012, 2:19:50 PM11/20/12

to alex...@chromium.org, w...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, rmsous...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org

On 2012/11/20 03:44:18, Wez wrote:
> This doesn't feel to me like it makes the states any easier to
> understand...
;)

I'll follow up on this CL with a change that allows to keep the host process
started even when host is disabled. This is required to keep listening on
WebSocket while the host is disabled. That change will add STOPPED->STARTED
state transition which is hard to track with the current flags.
But even without that change having enum for the state is helpful, e.g.
meaning
of restarting_ is not clear when shutting_down_ is set, so it's not clear
why
RestartOnHostShutdown() doesn't always restart the host even though
restarting_
is set.

https://codereview.chromium.org/11416093/

ser...@chromium.org

unread,

Nov 20, 2012, 3:02:34 PM11/20/12

to alex...@chromium.org, w...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, rmsous...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org

https://codereview.chromium.org/11416093/diff/4001/remoting/host/remoting_me2me_host.cc
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/11416093/diff/4001/remoting/host/remoting_me2me_host.cc#newcode177
remoting/host/remoting_me2me_host.cc:177: enum HostState {

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:
> 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

https://codereview.chromium.org/11416093/diff/4001/remoting/host/remoting_me2me_host.cc#newcode179
remoting/host/remoting_me2me_host.cc:179: HOST_BOOTSTRAPPING,
On 2012/11/20 03:44:19, Wez wrote:
> nit: HOST_STARTING

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?

https://codereview.chromium.org/11416093/diff/4001/remoting/host/remoting_me2me_host.cc#newcode185
remoting/host/remoting_me2me_host.cc:185: HOST_RESTARTING,

On 2012/11/20 03:44:19, Wez wrote:

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

See my comments above.

https://codereview.chromium.org/11416093/diff/4001/remoting/host/remoting_me2me_host.cc#newcode198
remoting/host/remoting_me2me_host.cc:198: // RESTARTING->STOPPING

On 2012/11/20 17:00:56, alexeypa wrote:
> nit: RESTARTING->STARTED is missing

Done.

https://codereview.chromium.org/11416093/diff/4001/remoting/host/remoting_me2me_host.cc#newcode936
remoting/host/remoting_me2me_host.cc:936: void
HostProcess::ShutdownHost(int exit_code) {

On 2012/11/20 03:44:19, Wez wrote:

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

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

https://codereview.chromium.org/11416093/

alex...@chromium.org

unread,

Nov 20, 2012, 4:18:30 PM11/20/12

to ser...@chromium.org, w...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, rmsous...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org

https://codereview.chromium.org/11416093/diff/4001/remoting/host/remoting_me2me_host.cc
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/11416093/diff/4001/remoting/host/remoting_me2me_host.cc#newcode177
remoting/host/remoting_me2me_host.cc:177: enum HostState {

On 2012/11/20 20:02:34, sergeyu wrote:
> We don't really need ...

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.

https://codereview.chromium.org/11416093/

ser...@chromium.org

unread,

Nov 20, 2012, 5:39:30 PM11/20/12

to alex...@chromium.org, w...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, rmsous...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org

That doesn't look right - we want host to be started after we receive
policies
if we have a valid config.

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

We still need to know somehow if the host needs to be started again once
it's
stopped. Note that we don't always want to start the host even when config
appears to be valid. E.g. if we have a config and tried to start the host,
but
then failed to get access token (e.g. because refresh token is invalid)
then we
want it to stay in STOPPED state. We could have a separate flag for this
(e.g.
|restarting_|), but it would only make it more confusing. It's better if we
store it as part of the state.
To make distinction between RESTARTING and STOPPING states cleaner, maybe
rename
RESTARTING to STOPPING_TO_RESTART. WDYT?

https://codereview.chromium.org/11416093/

ser...@chromium.org

unread,

Nov 21, 2012, 6:44:43 PM11/21/12

to alex...@chromium.org, w...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, rmsous...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org

Renamed BOOTSTRAPPING to INITIALIZING and RESTARTING to STOPPING_TO_RESTART.
Also cleaned up start-up sequence - now there is StartOnNetworkThread() that
creates objects that work on the network thread. Added TODOs for further
cleanups. PTAL.

https://codereview.chromium.org/11416093/

alex...@chromium.org

unread,

Nov 21, 2012, 6:50:51 PM11/21/12

to ser...@chromium.org, w...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, rmsous...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org

> > 1. WAITING_POLICIES -> STOPPED (Policies received)
> That doesn't look right - we want host to be started after we receive
> policies
> if we have a valid config.

You are making the assumption that an event causing a transition can cause
only
one transition. The state diagram I'm talking about assumes that a single
event
can cause multiple transitions. For instance arriving policies cause
WAITING_POLICIES -> STOPPED, but when we switch to STOPPED we check whether
the
host needs to started and if so, another event is generated causing STOPPED
->
STARTED transition.

The idea is to minimize number of possible transitions between states (5 vs
8 in
this case) so that there are less variations of code to run. This is
especially
useful when the number of states in the state diagram grows.
This approach though comes at cost of additional state variables (in
addition to
the state number itself) and slightly more complex event generation code.

> We still need to know somehow if the host needs to be started again once
> it's
> stopped. Note that we don't always want to start the host even when config
> appears to be valid. E.g. if we have a config and tried to start the
> host, but
> then failed to get access token (e.g. because refresh token is invalid)
> then
we
> want it to stay in STOPPED state.

Yes, agree. Some additional state needs to be kept bit I think less
variance in
the transitions handling code is worth it.

> To make distinction between RESTARTING and STOPPING states cleaner, maybe
rename
> RESTARTING to STOPPING_TO_RESTART. WDYT?

STOPPING_TO_RESTART sounds better. Though my main concern is the number of
transitions because they directly reflect complexity of the code. The
difference
between two approaches is not that great, so it isn't worth discussing it
till
the end of times. lgtm.

https://chromiumcodereview.appspot.com/11416093/

alex...@chromium.org

unread,

Nov 21, 2012, 6:53:48 PM11/21/12

to ser...@chromium.org, w...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, rmsous...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org

https://chromiumcodereview.appspot.com/11416093/diff/1003/remoting/host/remoting_me2me_host.cc
File remoting/host/remoting_me2me_host.cc (right):

https://chromiumcodereview.appspot.com/11416093/diff/1003/remoting/host/remoting_me2me_host.cc#newcode260
remoting/host/remoting_me2me_host.cc:260:
nit: remove the empty line?

https://chromiumcodereview.appspot.com/11416093/diff/1003/remoting/host/remoting_me2me_host.cc#newcode980
remoting/host/remoting_me2me_host.cc:980:
nit: remove the empty line. Or two.

https://chromiumcodereview.appspot.com/11416093/

ser...@chromium.org

unread,

Nov 26, 2012, 8:04:38 PM11/26/12

to alex...@chromium.org, w...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, rmsous...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org

https://chromiumcodereview.appspot.com/11416093/diff/1003/remoting/host/remoting_me2me_host.cc
File remoting/host/remoting_me2me_host.cc (right):

https://chromiumcodereview.appspot.com/11416093/diff/1003/remoting/host/remoting_me2me_host.cc#newcode260
remoting/host/remoting_me2me_host.cc:260:

On 2012/11/21 23:53:48, alexeypa wrote:
> nit: remove the empty line?

Done.

https://chromiumcodereview.appspot.com/11416093/diff/1003/remoting/host/remoting_me2me_host.cc#newcode980
remoting/host/remoting_me2me_host.cc:980: