stopped -> starting
starting-> started
starting-> stopping (something like a bad config file)
starting-> stopped (error handling)
started -> starting (restart)
started -> stopping
stopping -> stopped
> 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/diff/4001/remoting/host/remo... 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
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?
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.
> 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)
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?
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.
> > 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.
