Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
910403002:
Suspend (rather than shut down) the host upon policy errors. (Closed)
|
Created:
5 years, 10 months ago by Łukasz Anforowicz Modified:
5 years, 10 months ago CC:
chromium-reviews, chromoting-reviews_chromium.org, Wez Base URL:
https://chromium.googlesource.com/chromium/src.git@mistyped-policies Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSuspend (rather than shut down) the host upon policy errors.
This changelist primarily changes HostProcess::OnPolicyError, so that it
doesn't anymore shut down the host, but instead restarts the host and prevents
the host from starting again until a valid policy has been read (the
"preventing" functionality has already been present in the form of
StartHostIfReady method for some time now).
This changelist also simplifies HostSignalingManager's functionality related to
sending host-offline-reason. Previously HostProcess was destroyed while
HostSignalingManager was trying to send host-offline-reason. Things are
simpler if HostProcess waits until HostSignalingManager sends
host-offline-reason or times out. This synchronization is especially helpful
when restarting the host (to avoid having to deal with an old
HostSignalingManager from previous SendHostOfflineReason attempt, while we
construct a new HostSignalingManager when starting a host after a reset).
This changelist also introduces a separate host-offline-reason for policy
errors (and updates WebApp resources strings to reflect that). Note that since
we are not shutting down the host, we probably cannot use one of HostExitCodes
as the host-offline-reason.
Testing done:
1. On Linux manually introduce policy errors at startup, verify host doesn't
start and sends host-offline-reason, fix errors, verify host starts,
reintroduce errors, verify host suspends and sends host-offline-reason, fix
errors, verify host starts again.
2. Verify WebApp UI shows correct tooltip text for the new host-offline-reason.
BUG=455903
Committed: https://crrev.com/fecb18b781906729a0ae22fd82a6dc3fcc929355
Cr-Commit-Position: refs/heads/master@{#317708}
Patch Set 1 : #
Total comments: 24
Patch Set 2 : Addressed code review feedback from Wez. #
Total comments: 9
Patch Set 3 : Style tweaks in remoting_me2me_host.cc #
Total comments: 11
Patch Set 4 : Clean-up and readability improvements for state transitions. #
Total comments: 11
Patch Set 5 : s/HOST_INITIALIZING/HOST_STARTING/g #
Total comments: 14
Patch Set 6 : Addressed Sergey's code review feedback. #Patch Set 7 : Rebasing... #
Messages
Total messages: 25 (9 generated)
Patchset #1 (id:1) has been deleted
lukasza@chromium.org changed reviewers: + sergeyu@chromium.org
Sergey, could you please take a look? Wez, I've added you to CC in case you want to take a look, since you've code reviewed my changelist that introduced HostSignalingManager and the current changelist modifies this class (I think/hope simplifying it). Thanks, Lukasz
wez@chromium.org changed reviewers: + wez@chromium.org
https://codereview.chromium.org/910403002/diff/20001/remoting/host/host_signa... File remoting/host/host_signaling_manager.h (right): https://codereview.chromium.org/910403002/diff/20001/remoting/host/host_signa... remoting/host/host_signaling_manager.h:51: // destroyed nit: Missing punctuation at end of line. https://codereview.chromium.org/910403002/diff/20001/remoting/host/host_signa... remoting/host/host_signaling_manager.h:83: void OnHostOfflineReasonAck(bool success); You've removed this method, haven't you? https://codereview.chromium.org/910403002/diff/20001/remoting/host/host_signa... remoting/host/host_signaling_manager.h:93: base::ThreadChecker thread_checker_; Do you need an explicit thread-checker, or could you use base::NonThreadSafe, thereby expressing it as a property of the class' interface? https://codereview.chromium.org/910403002/diff/20001/remoting/host/remoting_m... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/910403002/diff/20001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:293: void ShutdownOnNetworkThread(const std::string* host_offline_reason); Do we need to express "no reason" via nullptr - could we use an empty string instead? https://codereview.chromium.org/910403002/diff/20001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:1025: void HostProcess::ReportPolicyErrorAndRestartHost() { Please put some blank lines in this function to break it up into logical blocks, e.g. the initial DCHECK()s express preconditions, then there's some setting of internal state, and finally the shutdown call. https://codereview.chromium.org/910403002/diff/20001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:1452: ShutdownOnNetworkThread(nullptr); Is "no reason" the right thing to report here? Why not report "restarting"? https://codereview.chromium.org/910403002/diff/20001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:1488: if (host_offline_reason == nullptr) { nit: Please add some comments to these two blocks to clarify the intended behaviour. https://codereview.chromium.org/910403002/diff/20001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:1505: base::Bind(&HostProcess::OnHostOfflineReasonAck, base::Unretained(this))); With things structured as they are, with a nullptr when there is no offline-reason to send, you don't really need the ShutdownOnNetworkThread/ContinueShutdownOnNetworkThread split - you can just have ShutdownOnNetworkThread send the shutdown reason if one is specified, and bind ShutdownOnNetworkThread(nullptr) to the completion callback. Even if you want to log whether the send succeeded or failed, OnHostOfflineReasonAck() can just call ShutdownOnNetworkThread(nullptr) to complete the shutdown. https://codereview.chromium.org/910403002/diff/20001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:1508: void HostProcess::ContinueShutdownOnNetworkThread() { If you prefer to keep this as-is then I'd suggest renaming this to ShutdownOnNetworkThread() and the other to ...WithReason(). https://codereview.chromium.org/910403002/diff/20001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:1532: DCHECK(context_->network_task_runner()->BelongsToCurrentThread()); As above, suggest blank line between the pre-condition DCHECK and the actual logic.
Thanks for the feedback Wez. I tried addressing it in patchset #2. Sergey - can you take a look as well? (since this builds on top of the StartHostIfReady you've introduced recently to push policy loading earlier / in parallel with config loading). Thanks, Lukasz https://codereview.chromium.org/910403002/diff/20001/remoting/host/host_signa... File remoting/host/host_signaling_manager.h (right): https://codereview.chromium.org/910403002/diff/20001/remoting/host/host_signa... remoting/host/host_signaling_manager.h:51: // destroyed On 2015/02/12 02:51:25, Wez wrote: > nit: Missing punctuation at end of line. Done. https://codereview.chromium.org/910403002/diff/20001/remoting/host/host_signa... remoting/host/host_signaling_manager.h:83: void OnHostOfflineReasonAck(bool success); On 2015/02/12 02:51:25, Wez wrote: > You've removed this method, haven't you? Yes, thanks for catching this. https://codereview.chromium.org/910403002/diff/20001/remoting/host/host_signa... remoting/host/host_signaling_manager.h:93: base::ThreadChecker thread_checker_; On 2015/02/12 02:51:25, Wez wrote: > Do you need an explicit thread-checker, or could you use base::NonThreadSafe, > thereby expressing it as a property of the class' interface? Either option would work. I picked the "member option", because thread_checker.h says that "having a ThreadChecker member and calling CalledOnValidThread is the preferable solution" (https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr...) https://codereview.chromium.org/910403002/diff/20001/remoting/host/remoting_m... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/910403002/diff/20001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:293: void ShutdownOnNetworkThread(const std::string* host_offline_reason); On 2015/02/12 02:51:25, Wez wrote: > Do we need to express "no reason" via nullptr - could we use an empty string > instead? We can use an empty string. Thanks for bringing this up. I did think about it, not sure why I picked the pointer option now... It seemed like a more distinct / more obvious option, but HeartbeatSender already treats an empty string as special, so it is probably better to be consistent. https://codereview.chromium.org/910403002/diff/20001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:1025: void HostProcess::ReportPolicyErrorAndRestartHost() { On 2015/02/12 02:51:25, Wez wrote: > Please put some blank lines in this function to break it up into logical blocks, > e.g. the initial DCHECK()s express preconditions, then there's some setting of > internal state, and finally the shutdown call. Done. https://codereview.chromium.org/910403002/diff/20001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:1452: ShutdownOnNetworkThread(nullptr); On 2015/02/12 02:51:25, Wez wrote: > Is "no reason" the right thing to report here? Why not report "restarting"? Good point. Done. I was also struggling with a decision whether to just have string literals with host-offline-reason at the point of use VS introduce string constants somewhere (maybe in host_exit_codes.h, maybe in a new header...). Having a second string in here, pushed me to at least add string constants in an anonymous namespace in the same file. https://codereview.chromium.org/910403002/diff/20001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:1488: if (host_offline_reason == nullptr) { On 2015/02/12 02:51:25, Wez wrote: > nit: Please add some comments to these two blocks to clarify the intended > behaviour. Done (at least I tried to add sensible comments). https://codereview.chromium.org/910403002/diff/20001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:1505: base::Bind(&HostProcess::OnHostOfflineReasonAck, base::Unretained(this))); On 2015/02/12 02:51:25, Wez wrote: > With things structured as they are, with a nullptr when there is no > offline-reason to send, you don't really need the > ShutdownOnNetworkThread/ContinueShutdownOnNetworkThread split - you can just > have ShutdownOnNetworkThread send the shutdown reason if one is specified, and > bind ShutdownOnNetworkThread(nullptr) to the completion callback. Even if you > want to log whether the send succeeded or failed, OnHostOfflineReasonAck() can > just call ShutdownOnNetworkThread(nullptr) to complete the shutdown. Done. I think things look better now. - I didn't go with WithReason/WithNoReason function pair, because the WithNoReason part (if treated as "public" to the rest of this class) has to duplicate resetting of host, host_event_logger, host_status_logger, host_change_notification_listener. - Part of me wants to treat "not possible without a config" as an assert/DCHECK, but this requires unnecessary knowledge at callsites of ShutdownOnNetworkThread (and ShutdownHost!) whether or not config should be read already. Things seem better with a log instead of an assert (I think). https://codereview.chromium.org/910403002/diff/20001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:1508: void HostProcess::ContinueShutdownOnNetworkThread() { On 2015/02/12 02:51:25, Wez wrote: > If you prefer to keep this as-is then I'd suggest renaming this to > ShutdownOnNetworkThread() and the other to ...WithReason(). N/A (I went with one function as you suggested above). https://codereview.chromium.org/910403002/diff/20001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:1532: DCHECK(context_->network_task_runner()->BelongsToCurrentThread()); On 2015/02/12 02:51:25, Wez wrote: > As above, suggest blank line between the pre-condition DCHECK and the actual > logic. Done.
One point I meant to make when I reviewed this; the HostSignallingManager now does little more than construct the HeartbeatSender and provide a way to share the SignalingStrategy with it. Do we actually need the HostSignallingManager at all - could we just provide a constructor function that composes the necessary bits to return a HeartbeatSender, and have the HeartbeatSender own the SignalingStrategy? Or should SignalingStrategy be a shared resource, since it's used by both the HeartbeatSender, the host, and individual sessions?
https://codereview.chromium.org/910403002/diff/20001/remoting/host/host_signa... File remoting/host/host_signaling_manager.h (right): https://codereview.chromium.org/910403002/diff/20001/remoting/host/host_signa... remoting/host/host_signaling_manager.h:93: base::ThreadChecker thread_checker_; On 2015/02/12 18:08:01, lukasza wrote: > On 2015/02/12 02:51:25, Wez wrote: > > Do you need an explicit thread-checker, or could you use base::NonThreadSafe, > > thereby expressing it as a property of the class' interface? > > Either option would work. I picked the "member option", because > thread_checker.h says that "having a ThreadChecker member and calling > CalledOnValidThread is the preferable solution" > (https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr...) Acknowledged! Didn't realise that was the new approach. :) https://codereview.chromium.org/910403002/diff/20001/remoting/host/remoting_m... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/910403002/diff/20001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:293: void ShutdownOnNetworkThread(const std::string* host_offline_reason); On 2015/02/12 18:08:01, lukasza wrote: > On 2015/02/12 02:51:25, Wez wrote: > > Do we need to express "no reason" via nullptr - could we use an empty string > > instead? > > We can use an empty string. Thanks for bringing this up. I did think about it, > not sure why I picked the pointer option now... It seemed like a more distinct > / more obvious option, but HeartbeatSender already treats an empty string as > special, so it is probably better to be consistent. Acknowledged. https://codereview.chromium.org/910403002/diff/20001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:1488: if (host_offline_reason == nullptr) { On 2015/02/12 18:08:01, lukasza wrote: > On 2015/02/12 02:51:25, Wez wrote: > > nit: Please add some comments to these two blocks to clarify the intended > > behaviour. > > Done (at least I tried to add sensible comments). These look good in terms of content, but instead of the ellipses, suggest "// Shut down everything except the HostSignalingManager." "// Before tearing down HostSignalingManager, send the |host_offline_reason|, if any." https://codereview.chromium.org/910403002/diff/40001/remoting/host/remoting_m... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/910403002/diff/40001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:1497: if (!host_offline_reason.empty() && serialized_config_.empty()) { This would read more clearly as: if (!host_offline_reason.empty()) { if (!serialized_config_.empty()) { ... send offline reason and return ... } else { .. error message ... } } ... shut down ALL da things ... https://codereview.chromium.org/910403002/diff/40001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:1542: ShutdownOnNetworkThread(std::string()); Now that I see this written out, it seems less clear than splitting ShutdownOnNetworkThread() and ShutdownOnNetworkThreadWithReason(). WDYT? https://codereview.chromium.org/910403002/diff/40001/remoting/resources/remot... File remoting/resources/remoting_strings.grd (right): https://codereview.chromium.org/910403002/diff/40001/remoting/resources/remot... remoting/resources/remoting_strings.grd:893: name="IDS_OFFLINE_REASON_RESTARTING"> Do these really need localizing? Where in the UI are they displayed?
Thanks Wez. I replied and made some tweaks "inline" (where I also tried addressing your other top-level comment - I've copy&pasted your comment into host_signaling_manager.h). -Lukasz https://codereview.chromium.org/910403002/diff/20001/remoting/host/remoting_m... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/910403002/diff/20001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:1488: if (host_offline_reason == nullptr) { On 2015/02/12 21:07:07, Wez wrote: > On 2015/02/12 18:08:01, lukasza wrote: > > On 2015/02/12 02:51:25, Wez wrote: > > > nit: Please add some comments to these two blocks to clarify the intended > > > behaviour. > > > > Done (at least I tried to add sensible comments). > > These look good in terms of content, but instead of the ellipses, suggest > > "// Shut down everything except the HostSignalingManager." > > "// Before tearing down HostSignalingManager, send the |host_offline_reason|, if > any." Done. https://codereview.chromium.org/910403002/diff/40001/remoting/host/host_signa... File remoting/host/host_signaling_manager.h (left): https://codereview.chromium.org/910403002/diff/40001/remoting/host/host_signa... remoting/host/host_signaling_manager.h:9: Wez said: > One point I meant to make when I reviewed this; the HostSignallingManager > now does little more than construct the HeartbeatSender and provide a way > to share the SignalingStrategy with it. > > Do we actually need the HostSignallingManager at all - could we just > provide a constructor function that composes the necessary bits to return > a HeartbeatSender, and have the HeartbeatSender own the SignalingStrategy? > > Or should SignalingStrategy be a shared resource, since it's used by both > the HeartbeatSender, the host, and individual sessions? I agree with your observation that HostSignalingManager doesn't do much on top of the objects it wraps, but I think it is still desirable to keep it: 1) I think it is cleaner if HeartbeatSender continues to depend just on a pre-created SignalingStrategy and doesn't have to know about and manage destruction of SignalingConnector. 2) OnAuthFailed fits HostSignalingManager and SignalingConnector, but doesn't fit IMO exposed out of HeartbeatSender. I guess you could say that the "constructor/factory function" could take a callback that would be passed under the covers to SignalingConnector. 3) host_signaling_manager.cc hides implementation details from other places in the code (presence of DnsBlackholeChecker, exposing only SignalStrategy while internally constructing and depending on XmppSignalStrategy). I guess these implementation details can be hidden inside the "constructor/factory function" compilation unit. Also - one alternative is to go all the way back to having separate heartbeat_sender_, signaling_connector_ and signaling_strategy_ fields in HostProcess. What do you think? I am probably biased, since I am the guy who introduced HostSignalingManager in the first place. https://codereview.chromium.org/910403002/diff/40001/remoting/host/remoting_m... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/910403002/diff/40001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:1497: if (!host_offline_reason.empty() && serialized_config_.empty()) { On 2015/02/12 21:07:07, Wez wrote: > This would read more clearly as: > > if (!host_offline_reason.empty()) { > if (!serialized_config_.empty()) { > ... send offline reason and return ... > } else { > .. error message ... > } > } > > ... shut down ALL da things ... Done. https://codereview.chromium.org/910403002/diff/40001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:1542: ShutdownOnNetworkThread(std::string()); On 2015/02/12 21:07:07, Wez wrote: > Now that I see this written out, it seems less clear than splitting > ShutdownOnNetworkThread() and ShutdownOnNetworkThreadWithReason(). WDYT? I think I like it better this way. If we have a pair of functions, then one of these functions (ContinueShutdown) is not really callable from anywhere other than Shutdown + OnAck. If we want to make ContinueShutdown callable separately (and name it ShutdownOnNetworkThread[WithoutReason]) then it has to duplicate the code that resets the 4 members (host, host_event_logger, ...) or we have to move this code to yet another method. OTOH, there are 2 things that bother me about the current approach: 1) a bit complicated logic (3 levels of nesting! :-) for figuring out when to call SendHostOfflineReason (i.e. have to check host_offline_reason.empty(), host_signaling_manager_ not null, serializer_config_.empty). 2) the whole ShutdownOnNetworkThread got a bit long But I think this is still ok. ShutdownOnNetworkThread is not that difficult to follow (and I think surely not more difficult than having to understand how 2 or 3 methods work together / which one of them can be invoked standalone vs only from the other one). https://codereview.chromium.org/910403002/diff/40001/remoting/resources/remot... File remoting/resources/remoting_strings.grd (right): https://codereview.chromium.org/910403002/diff/40001/remoting/resources/remot... remoting/resources/remoting_strings.grd:893: name="IDS_OFFLINE_REASON_RESTARTING"> On 2015/02/12 21:07:07, Wez wrote: > Do these really need localizing? Good question. An alternative would be to only localize "This is a host-offline-reason" part and append host-offline-reason verbatim, saying that this is a "code" (and any similarity to English is accidental :-). > Where in the UI are they displayed? Today, the WebApp UI shows this in a tooltop shown when hovering a mouse over a hostname of a host that sent a host-offline-reason. We need to add UI for Android and iOS. And I think we need to replace the tooltip with something more discoverable (Jamie and Kelvin disagree / think that the tooltip is good enough). Also - today the user gets a somewhat cryptic, non-actionable, although at least localized host-offline-reason. Ideally we would guide the user to instructions for how to analyze the host-side logs to get more info about the errors. Maybe we can add a link to the localized string? (but today we don't have public chromoting docs where "what do I do after getting host-offline-reason X" could go).
https://codereview.chromium.org/910403002/diff/60001/remoting/host/remoting_m... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/910403002/diff/60001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:152: const char kHostOfflineBecauseOfRestarting[] = "RESTARTING"; s/Because/Reason/ to match field name https://codereview.chromium.org/910403002/diff/60001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:1004: if (state_ == HOST_INITIALIZING || state_ == HOST_STOPPING_TO_RESTART) { What if policies are updated while we are still waiting response to the host-offline-reason when restarting the host due to the previous policy change? https://codereview.chromium.org/910403002/diff/60001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:1022: StartHostIfReady(); This also calls ReportPolicyErrorAndRestartHost(), but only if config is ready. Its also confusing that StartHostIfReady() is called when policies are broken. So maybe always call ReportPolicyErrorAndRestartHost() here, just check that the config has been loaded? https://codereview.chromium.org/910403002/diff/60001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:1036: state_ = HOST_STOPPING_TO_RESTART; Call RestartHost()? https://codereview.chromium.org/910403002/diff/60001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:1485: void HostProcess::ShutdownOnNetworkThread( This function handles two separate cases: host restart and host shutdown. It did make sense in the past when restart was multi-step process that required jumping to other threads. But it's not the case anymore. So I suggest splitting these two cases. Also HOST_STOPPING_TO_RESTART state is not useful anymore - host can be restarted synchronously. Instead you need a new state for waiting for offline reason ACK and another one for the vegetative state when policies are broken (or maybe reuse INITIALIZING for that). https://codereview.chromium.org/910403002/diff/60001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:1538: if (success) { HOST_LOG << "SendHostOfflineReason " << (success ? "succeeded" : "timed out");
Patchset #4 (id:80001) has been deleted
Thanks for the feedback Sergey. I tried to respond by renaming HostState names + being more restrictive about + actually enforcing allowed state transitions. Wez, I did end up with the functionality of the old ShutdownOnNetworkThread split across 2 functions (GoOffline + OnHostOfflineReasonAck). https://codereview.chromium.org/910403002/diff/40001/remoting/host/remoting_m... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/910403002/diff/40001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:1542: ShutdownOnNetworkThread(std::string()); On 2015/02/12 22:51:13, lukasza wrote: > On 2015/02/12 21:07:07, Wez wrote: > > Now that I see this written out, it seems less clear than splitting > > ShutdownOnNetworkThread() and ShutdownOnNetworkThreadWithReason(). WDYT? > > I think I like it better this way. > > If we have a pair of functions, then one of these functions (ContinueShutdown) > is not really callable from anywhere other than Shutdown + OnAck. If we want to > make ContinueShutdown callable separately (and name it > ShutdownOnNetworkThread[WithoutReason]) then it has to duplicate the code that > resets the 4 members (host, host_event_logger, ...) or we have to move this code > to yet another method. > > OTOH, there are 2 things that bother me about the current approach: > 1) a bit complicated logic (3 levels of nesting! :-) for figuring out when to > call SendHostOfflineReason (i.e. have to check host_offline_reason.empty(), > host_signaling_manager_ not null, serializer_config_.empty). > 2) the whole ShutdownOnNetworkThread got a bit long > > But I think this is still ok. ShutdownOnNetworkThread is not that difficult to > follow (and I think surely not more difficult than having to understand how 2 or > 3 methods work together / which one of them can be invoked standalone vs only > from the other one). Actually, when addressing Sergey's feedback (and trying to clean-up state transitions in general) I went ahead with 2 functions in patchset #4. I think things are in a better shape right now (better than baseline + better than the 1st two patchsets). https://codereview.chromium.org/910403002/diff/60001/remoting/host/remoting_m... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/910403002/diff/60001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:1004: if (state_ == HOST_INITIALIZING || state_ == HOST_STOPPING_TO_RESTART) { On 2015/02/13 02:59:29, Sergey Ulanov wrote: > What if policies are updated while we are still waiting response to the > host-offline-reason when restarting the host due to the previous policy change? This should be fine, because policy_state_ will be taken into account when restarting (i.e. once we get to OnHostOfflineReasonAck, it will call to StartHostIfReady which will start the host if policy are fine at this point). https://codereview.chromium.org/910403002/diff/60001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:1022: StartHostIfReady(); On 2015/02/13 02:59:28, Sergey Ulanov wrote: > This also calls ReportPolicyErrorAndRestartHost(), but only if config is ready. > Its also confusing that StartHostIfReady() is called when policies are broken. > So maybe always call ReportPolicyErrorAndRestartHost() here, just check that the > config has been loaded? Good point. Thanks. https://codereview.chromium.org/910403002/diff/60001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:1036: state_ = HOST_STOPPING_TO_RESTART; On 2015/02/13 02:59:29, Sergey Ulanov wrote: > Call RestartHost()? Good point. Note that to do that I removed source-state DCHECK in RestartHost (but this DCHECK is at any rate made obsolete by the DCHECKs I introduced in SetState). https://codereview.chromium.org/910403002/diff/60001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:1485: void HostProcess::ShutdownOnNetworkThread( On 2015/02/13 02:59:28, Sergey Ulanov wrote: > This function handles two separate cases: host restart and host shutdown. It did > make sense in the past when restart was multi-step process that required jumping > to other threads. But it's not the case anymore. So I suggest splitting these > two cases. Also HOST_STOPPING_TO_RESTART state is not useful anymore - host can > be restarted synchronously. Instead you need a new state for waiting for offline > reason ACK and another one for the vegetative state when policies are broken (or > maybe reuse INITIALIZING for that). Host doesn't restart synchronously - there is the waiting for OnHostOfflineReasonAck (and during this time HOST_STOPPING_TO_RESTART can possibly change into HOST_STOPPING, so I think it is still useful to keep these 2 states). OTOH, I probably should rename them to HOST_GOING_OFFLINE_TO_RESTART and HOST_GOING_OFFLINE_TO_STOP. And I should go from HOST_GOING_OFFLINE_TO_RESTART to HOST_INITIALIZING (if policy is not ready at this point). https://codereview.chromium.org/910403002/diff/60001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:1538: if (success) { On 2015/02/13 02:59:28, Sergey Ulanov wrote: > HOST_LOG << "SendHostOfflineReason " > << (success ? "succeeded" : "timed out"); Done. https://codereview.chromium.org/910403002/diff/100001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (left): https://codereview.chromium.org/910403002/diff/100001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:201: Summary of the changes I made to "allowed state transitions": STOPPED->STARTED is the only transition that I deleted altogether (reading the code I don't see how this transition could possibly happen). I renamed STOPPING_TO_RESTART to GOING_OFFLINE_TO_RESTART and renamed STOPPING to GOING_OFFLINE_TO_STOP. There is no direct transition from INITIALIZING to STOPPED. This always goes through GOING_OFFLINE_TO_STOP first. Also - after my changes there is no direct transition from STOPPING_TO_RESTART to STARTED. Right now it always (sometimes very briefly/synchronously) goes through intermediate INITIALIZING state. And there is one new transition because of OnPolicyError - going from INITIALIZING to GOING_OFFLINE_TO_RESTART.
This looks reasonable; I'll defer to Sergey to provide the final approval! https://codereview.chromium.org/910403002/diff/40001/remoting/resources/remot... File remoting/resources/remoting_strings.grd (right): https://codereview.chromium.org/910403002/diff/40001/remoting/resources/remot... remoting/resources/remoting_strings.grd:893: name="IDS_OFFLINE_REASON_RESTARTING"> On 2015/02/12 22:51:13, lukasza wrote: > On 2015/02/12 21:07:07, Wez wrote: > > Do these really need localizing? > > Good question. An alternative would be to only localize "This is a > host-offline-reason" part and append host-offline-reason verbatim, saying that > this is a "code" (and any similarity to English is accidental :-). > > > Where in the UI are they displayed? > > Today, the WebApp UI shows this in a tooltop shown when hovering a mouse over a > hostname of a host that sent a host-offline-reason. We need to add UI for > Android and iOS. And I think we need to replace the tooltip with something more > discoverable (Jamie and Kelvin disagree / think that the tooltip is good > enough). > > Also - today the user gets a somewhat cryptic, non-actionable, although at least > localized host-offline-reason. Ideally we would guide the user to instructions > for how to analyze the host-side logs to get more info about the errors. Maybe > we can add a link to the localized string? (but today we don't have public > chromoting docs where "what do I do after getting host-offline-reason X" could > go). No, if these end up in UX they should be localized; that's fine. https://codereview.chromium.org/910403002/diff/100001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/910403002/diff/100001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:195: HOST_INITIALIZING, Suggest renaming this to HOST_STARTING since it can now be hit multiple times. https://codereview.chromium.org/910403002/diff/100001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:202: // 1) policy change (or a config change in the future) requires a restart. How can we detect a config change in the future...? https://codereview.chromium.org/910403002/diff/100001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:204: HOST_GOING_OFFLINE_TO_RESTART, Why not simply HOST_RESTARTING or HOST_STOPPING_TO_RESTART? https://codereview.chromium.org/910403002/diff/100001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:207: HOST_GOING_OFFLINE_TO_STOP, Why not simply HOST_STOPPING as it was before? https://codereview.chromium.org/910403002/diff/100001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1542: void HostProcess::OnHostOfflineReasonAck(bool success) { nit: It's weird for this to be OnHostOfflineReasonAck without there being any function in here called anything relating to sending the host offline reason. :/
I renamed INITIALIZING to STARTING as suggested by Wez and kept the other changes (hopefully my comments below do a good job of explaining why). Sergey - can you take a look please? https://codereview.chromium.org/910403002/diff/100001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/910403002/diff/100001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:195: HOST_INITIALIZING, On 2015/02/14 00:49:15, Wez wrote: > Suggest renaming this to HOST_STARTING since it can now be hit multiple times. Done. https://codereview.chromium.org/910403002/diff/100001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:202: // 1) policy change (or a config change in the future) requires a restart. On 2015/02/14 00:49:15, Wez wrote: > How can we detect a config change in the future...? I meant that a config change can theoretically also require a restart (i.e. Sergey's TODO in onConfigUpdate says [emphasis mine]: "[...] Change ApplyConfig() to detect other changes in the config and *restart* host if necessary here.") and that such restart would be implemented in the future. I'll just remove the text in parens. https://codereview.chromium.org/910403002/diff/100001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:204: HOST_GOING_OFFLINE_TO_RESTART, On 2015/02/14 00:49:15, Wez wrote: > Why not simply HOST_RESTARTING or HOST_STOPPING_TO_RESTART? I like your suggestion for its brevity, but I think HOST_GOING_OFFLINE_TO_RESTART and HOST_GOING_OFFLINE_TO_STOP are slightly better so I will stick with them (at least for now): 1) I see "restarting" and "stopping" as something that begins *after* OnHostOfflineReasonAck. The state *before* OnHostOfflineReasonAck needs a separate name IMO (i.e. "going offline"). 2) The meaning of "stopping" and "shutting down" is quite overloaded and ambiguous (i.e. I dislike the old ShutdownOnNetworkingThread name which sometimes does a real shutdown via ShutdownOnUiThread and sometimes doesn't). Having a separate name for "going offline" helps avoid the ambiguous "shutdown" or "stopping". https://codereview.chromium.org/910403002/diff/100001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:207: HOST_GOING_OFFLINE_TO_STOP, On 2015/02/14 00:49:15, Wez wrote: > Why not simply HOST_STOPPING as it was before? Please see my other comment. https://codereview.chromium.org/910403002/diff/100001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1542: void HostProcess::OnHostOfflineReasonAck(bool success) { On 2015/02/14 00:49:15, Wez wrote: > nit: It's weird for this to be OnHostOfflineReasonAck without there being any > function in here called anything relating to sending the host offline reason. :/ This (presence of OnXxx method, without DoXxx method) is similar to other methods in HostProcess, like OnPolicyUpdate, OnAuthFailed, OnUnknownHostIdError, OnHeartbeatSuccessful...
LGTM once my comments are addressed. https://codereview.chromium.org/910403002/diff/120001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/910403002/diff/120001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:203: // 2) policy error puts the host offline until we have a valid policy again. Isn't it in HOST_STARTING state in this case? https://codereview.chromium.org/910403002/diff/120001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:212: // Allowed state transitions (enforced via DCHECKs in SetState method): nit: Put this comment above HostState enum? https://codereview.chromium.org/910403002/diff/120001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:227: void SetState(HostState target_state) { Move this below in the class (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...). Also move definition out of the class declaration. https://codereview.chromium.org/910403002/diff/120001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:235: (target_state == HOST_GOING_OFFLINE_TO_RESTART)); add something like '<< state_ << "->" << target_state' to make it easier to debug failures in this DCHECK. Same for the other DCHECKs below. https://codereview.chromium.org/910403002/diff/120001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1053: if (((state_ == HOST_STARTING) || (state_ == HOST_STARTED)) && Maybe move these checks into ReportPolicyErrorAndRestartHost()? If you keep them here then add DCHECKs for them in ReportPolicyErrorAndRestartHost(). https://codereview.chromium.org/910403002/diff/120001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1053: if (((state_ == HOST_STARTING) || (state_ == HOST_STARTED)) && nit: IMO these extra parentheses around == operations hurt readability and I think chromium code normally doesn't use them for conditions like this. Same for for ! operation below. (you still want the parentheses around || and &&). Style guide is ambiguous about it (Feel free to insert extra parentheses judiciously because they can be very helpful in increasing readability when used appropriately.), but the example it gives doesn't have parentheses https://codereview.chromium.org/910403002/diff/120001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1537: HOST_LOG << "SendHostOfflineReason( " << host_offline_reason << ") " Maybe something like "Can't send offline reason (<the reason>) without a valid host config"?
New patchsets have been uploaded after l-g-t-m from sergeyu@chromium.org
Thanks Sergey. https://codereview.chromium.org/910403002/diff/120001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/910403002/diff/120001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:203: // 2) policy error puts the host offline until we have a valid policy again. On 2015/02/18 00:28:31, Sergey Ulanov wrote: > Isn't it in HOST_STARTING state in this case? "Host is sending offline reason" implies state_ == HOST_GOING_OFFLINE_TO_RESTART. After the offline reason is sent, the state_ will change to state_ == HOST_STARTING. I will just remove the "Used when" part of the comment to avoid the confusion. https://codereview.chromium.org/910403002/diff/120001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:212: // Allowed state transitions (enforced via DCHECKs in SetState method): On 2015/02/18 00:28:31, Sergey Ulanov wrote: > nit: Put this comment above HostState enum? I moved this comment next to SetState method (so that the list of transitions is kept next to the code that enforces the allowed transitions). https://codereview.chromium.org/910403002/diff/120001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:227: void SetState(HostState target_state) { On 2015/02/18 00:28:31, Sergey Ulanov wrote: > Move this below in the class > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...). > Also move definition out of the class declaration. I see. Thanks for pointing this out. Since I dislike having the "Allowed state transitions" comment far away from SetState method that enforces the allowed transitions, I'll also move the comment to be next to the SetState method's definition. https://codereview.chromium.org/910403002/diff/120001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:235: (target_state == HOST_GOING_OFFLINE_TO_RESTART)); On 2015/02/18 00:28:31, Sergey Ulanov wrote: > add something like '<< state_ << "->" << target_state' to make it easier to > debug failures in this DCHECK. Same for the other DCHECKs below. Done. https://codereview.chromium.org/910403002/diff/120001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1053: if (((state_ == HOST_STARTING) || (state_ == HOST_STARTED)) && On 2015/02/18 00:28:31, Sergey Ulanov wrote: > Maybe move these checks into ReportPolicyErrorAndRestartHost()? If you keep them > here then add DCHECKs for them in ReportPolicyErrorAndRestartHost(). I've added a DCHECK(!serialized_config.empty()) to ReportPolicyErrorAndRestartHost. https://codereview.chromium.org/910403002/diff/120001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1053: if (((state_ == HOST_STARTING) || (state_ == HOST_STARTED)) && On 2015/02/18 00:28:31, Sergey Ulanov wrote: > nit: IMO these extra parentheses around == operations hurt readability and I > think chromium code normally doesn't use them for conditions like this. Same for > for ! operation below. (you still want the parentheses around || and &&). > Style guide is ambiguous about it (Feel free to insert extra parentheses > judiciously because they can be very helpful in increasing readability when used > appropriately.), but the example it gives doesn't have parentheses Done. https://codereview.chromium.org/910403002/diff/120001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1537: HOST_LOG << "SendHostOfflineReason( " << host_offline_reason << ") " On 2015/02/18 00:28:31, Sergey Ulanov wrote: > Maybe something like "Can't send offline reason (<the reason>) without a valid > host config"? Done.
The CQ bit was checked by lukasza@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/910403002/#ps160001 (title: "Rebasing...")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/910403002/160001
The CQ bit was unchecked by lukasza@chromium.org
The CQ bit was checked by lukasza@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/910403002/160001
Message was sent while issue was closed.
Committed patchset #7 (id:160001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/fecb18b781906729a0ae22fd82a6dc3fcc929355 Cr-Commit-Position: refs/heads/master@{#317708} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
