Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11416093:
Add HostState enum to track host process status. (Closed)
|
Created:
8 years, 1 month ago by Sergey Ulanov Modified:
8 years ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, rmsousa+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd 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.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171677
Patch Set 1 : #Patch Set 2 : #
Total comments: 12
Patch Set 3 : #Patch Set 4 : #
Total comments: 4
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #Messages
Total messages: 12 (0 generated)
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_me2... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/11416093/diff/4001/remoting/host/remoting_me2... remoting/host/remoting_me2me_host.cc:179: HOST_BOOTSTRAPPING, nit: HOST_STARTING https://codereview.chromium.org/11416093/diff/4001/remoting/host/remoting_me2... 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_me2... 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/diff/4001/remoting/host/remoting_me2... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/11416093/diff/4001/remoting/host/remoting_me2... 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_me2... 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_me2... remoting/host/remoting_me2me_host.cc:198: // RESTARTING->STOPPING nit: RESTARTING->STARTED is missing
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/diff/4001/remoting/host/remoting_me2... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/11416093/diff/4001/remoting/host/remoting_me2... 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_me2... 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_me2... 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_me2... 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_me2... 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/diff/4001/remoting/host/remoting_me2... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/11416093/diff/4001/remoting/host/remoting_me2... 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.
On 2012/11/20 21:18:29, alexeypa wrote: > https://codereview.chromium.org/11416093/diff/4001/remoting/host/remoting_me2... > File remoting/host/remoting_me2me_host.cc (right): > > https://codereview.chromium.org/11416093/diff/4001/remoting/host/remoting_me2... > 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.
https://chromiumcodereview.appspot.com/11416093/diff/1003/remoting/host/remot... File remoting/host/remoting_me2me_host.cc (right): https://chromiumcodereview.appspot.com/11416093/diff/1003/remoting/host/remot... remoting/host/remoting_me2me_host.cc:260: nit: remove the empty line? https://chromiumcodereview.appspot.com/11416093/diff/1003/remoting/host/remot... remoting/host/remoting_me2me_host.cc:980: nit: remove the empty line. Or two.
https://chromiumcodereview.appspot.com/11416093/diff/1003/remoting/host/remot... File remoting/host/remoting_me2me_host.cc (right): https://chromiumcodereview.appspot.com/11416093/diff/1003/remoting/host/remot... 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/remot... remoting/host/remoting_me2me_host.cc:980: On 2012/11/21 23:53:48, alexeypa wrote: > nit: remove the empty line. Or two. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/11416093/15001 |
