Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2582003002:
Refactor ArcBridgeServiceImpl part 1. (Closed)
|
Created:
4 years ago by hidehiko Modified:
4 years ago Reviewers:
Luis Héctor Chávez CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, yusukes+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor ArcBridgeServiceImpl part 1.
This CL refactors ArcBridgeServiceImpl focusing on the
retry mechanism.
- With this CL, it starts to use OneShotTimer, which supports
cancel operation.
- Rename "reconnect" to "restart" for wording consistency.
- Introduce restart_delay_ for testing, instead of special
bool flag. Also, for restarting of testing, get rid of
synchronous PrerequisitesChanged() call, instead PostTask always.
- Extract StartArcSession() for sharing PrerequisitesChanged()
and restarting.
- Added more DCHECKs.
BUG=657687
BUG=b/31079732
TEST=Ran bots.
Committed: https://crrev.com/e18e9bcb729ce33f96a7b90c659cb5016bf97dbc
Cr-Commit-Position: refs/heads/master@{#439850}
Patch Set 1 #
Total comments: 15
Patch Set 2 : rebase #Patch Set 3 : Address comments. #Patch Set 4 : Address comments. #Patch Set 5 : Fix the latest bug. #
Messages
Total messages: 32 (21 generated)
The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hidehiko@chromium.org changed reviewers: + lhchavez@chromium.org
For better code review-ability, I split this CL from crrev.com/2574013003. Hope it is also better for you. PTAL.
Yes, this is much easier to review :D https://codereview.chromium.org/2582003002/diff/1/components/arc/arc_bridge_s... File components/arc/arc_bridge_service_impl.cc (right): https://codereview.chromium.org/2582003002/diff/1/components/arc/arc_bridge_s... components/arc/arc_bridge_service_impl.cc:25: namespace { nit: maybe add a newline before for consistency? https://codereview.chromium.org/2582003002/diff/1/components/arc/arc_bridge_s... components/arc/arc_bridge_service_impl.cc:89: void ArcBridgeServiceImpl::PrerequisitesChanged() { Now that you've removed the third caller of PrerequisitesChanged(), how about inlining the correct half of the method in the two places where it's called? https://codereview.chromium.org/2582003002/diff/1/components/arc/arc_bridge_s... components/arc/arc_bridge_service_impl.cc:97: restart_timer_.Stop(); How about you move this to L96-97 to RequestStop(), where it makes more sense? https://codereview.chromium.org/2582003002/diff/1/components/arc/arc_bridge_s... components/arc/arc_bridge_service_impl.cc:100: DCHECK(!restart_timer_.IsRunning()); Both branches check for this. Move to after L93? https://codereview.chromium.org/2582003002/diff/1/components/arc/arc_bridge_s... components/arc/arc_bridge_service_impl.cc:163: // If READY, ARC instance is unexpectedly crashed so we need to restart it nit: "ARC instance unexpectedly crashed". https://codereview.chromium.org/2582003002/diff/1/components/arc/arc_bridge_s... components/arc/arc_bridge_service_impl.cc:168: if (session_started_ && In the same spirit as we did reset |reconnect_|, shouldn't you set |session_started_ = false| here? Otherwise it's possible to call RequestStart() if the state wasn't READY or STOPPING and have that be ignored. At the very least it should be set to false in an else block. Otherwise, please mention that the semantics of |session_started_| have changed to mean something like "the user has requested for the ARC instance to be up".
https://codereview.chromium.org/2582003002/diff/1/components/arc/arc_bridge_s... File components/arc/arc_bridge_service_impl.cc (right): https://codereview.chromium.org/2582003002/diff/1/components/arc/arc_bridge_s... components/arc/arc_bridge_service_impl.cc:89: void ArcBridgeServiceImpl::PrerequisitesChanged() { On 2016/12/16 23:02:13, Luis Héctor Chávez wrote: > Now that you've removed the third caller of PrerequisitesChanged(), how about > inlining the correct half of the method in the two places where it's called? Nevermind about this, I just saw your second patch where you actually do this.
The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run
Thank you for review. PTAL. https://codereview.chromium.org/2582003002/diff/1/components/arc/arc_bridge_s... File components/arc/arc_bridge_service_impl.cc (right): https://codereview.chromium.org/2582003002/diff/1/components/arc/arc_bridge_s... components/arc/arc_bridge_service_impl.cc:25: namespace { On 2016/12/16 23:02:13, Luis Héctor Chávez wrote: > nit: maybe add a newline before for consistency? Done. https://codereview.chromium.org/2582003002/diff/1/components/arc/arc_bridge_s... components/arc/arc_bridge_service_impl.cc:89: void ArcBridgeServiceImpl::PrerequisitesChanged() { On 2016/12/16 23:06:35, Luis Héctor Chávez wrote: > On 2016/12/16 23:02:13, Luis Héctor Chávez wrote: > > Now that you've removed the third caller of PrerequisitesChanged(), how about > > inlining the correct half of the method in the two places where it's called? > > Nevermind about this, I just saw your second patch where you actually do this. Yes. Let me work on it in the following CL. https://codereview.chromium.org/2582003002/diff/1/components/arc/arc_bridge_s... components/arc/arc_bridge_service_impl.cc:97: restart_timer_.Stop(); On 2016/12/16 23:02:13, Luis Héctor Chávez wrote: > How about you move this to L96-97 to RequestStop(), where it makes more sense? If this is not a big problem, can I keep this in this CL. In this CL, I'd like to keep the main implementation of starting/stopping in this method, which looks the current implementation direction to me. Instead, the code around here is largely refactored in the following CL, and I'd like to address your comments in the CL. https://codereview.chromium.org/2582003002/diff/1/components/arc/arc_bridge_s... components/arc/arc_bridge_service_impl.cc:100: DCHECK(!restart_timer_.IsRunning()); On 2016/12/16 23:02:13, Luis Héctor Chávez wrote: > Both branches check for this. Move to after L93? Similar to above, can I address this in the following CL? https://codereview.chromium.org/2582003002/diff/1/components/arc/arc_bridge_s... components/arc/arc_bridge_service_impl.cc:163: // If READY, ARC instance is unexpectedly crashed so we need to restart it On 2016/12/16 23:02:13, Luis Héctor Chávez wrote: > nit: "ARC instance unexpectedly crashed". Done. https://codereview.chromium.org/2582003002/diff/1/components/arc/arc_bridge_s... components/arc/arc_bridge_service_impl.cc:168: if (session_started_ && On 2016/12/16 23:02:13, Luis Héctor Chávez wrote: > In the same spirit as we did reset |reconnect_|, shouldn't you set > |session_started_ = false| here? Otherwise it's possible to call RequestStart() > if the state wasn't READY or STOPPING and have that be ignored. At the very > least it should be set to false in an else block. > > Otherwise, please mention that the semantics of |session_started_| have changed > to mean something like "the user has requested for the ARC instance to be up". IIUC, unlike |reconnect_|, |session_started_| should not be set to false here. |reconnect_| is a flag representing if the current ARC instance gets READY state already. On the other hand |session_started_| is a flag whether a client code calls RequestStart/Stop(). So, also from the semantics point of view, |session_started_| should not be changed here, and it keeps the original semantics. For example, in the original code, "start -> ready -> crash -> (re-)start -> ready -> crash -> (re-)start -> ..." is one of expected scenarios, which is not reproducible if we reset |session_started_| here. In this new code, |reconnect_| flag is corresponding to "state() == READY". SetState(STOPPED) below is corresponding to |reconnect_ = false| conceptually. I have left the current code and comment as is, because actually no semantics change for |session_started_|. Of course welcome better way (like renaming var, better comment), but I'd like to address it in the following CL, which focuses on the state machine change. Actually the |session_started_| will be renamed to |run_requested_| in the CL (thank you for suggestion!). Maybe another way is change the condition as follows; if (state() == State::READY || (state() == State::STOPPING && session_started_)) { DCHECK(session_started_); : } The first condition (state == READY) is corresponding to |reconnect_ = true| case. The second condition (state == STOPPING && session_started_) is for RequestStop() immediately followed by RequestStart() case. Note that, if state == READY, session_started_ must be true always, because 1) to be READY, RequestStart() must be called once, and 2) if even at least once RequestStop() is called, the state is set to STOPPING. WDYT?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2582003002/diff/1/components/arc/arc_bridge_s... File components/arc/arc_bridge_service_impl.cc (right): https://codereview.chromium.org/2582003002/diff/1/components/arc/arc_bridge_s... components/arc/arc_bridge_service_impl.cc:168: if (session_started_ && On 2016/12/19 07:41:21, hidehiko wrote: > On 2016/12/16 23:02:13, Luis Héctor Chávez wrote: > > In the same spirit as we did reset |reconnect_|, shouldn't you set > > |session_started_ = false| here? Otherwise it's possible to call > RequestStart() > > if the state wasn't READY or STOPPING and have that be ignored. At the very > > least it should be set to false in an else block. > > > > Otherwise, please mention that the semantics of |session_started_| have > changed > > to mean something like "the user has requested for the ARC instance to be up". > > IIUC, unlike |reconnect_|, |session_started_| should not be set to false here. > > |reconnect_| is a flag representing if the current ARC instance gets READY state > already. On the other hand |session_started_| is a flag whether a client code > calls RequestStart/Stop(). > So, also from the semantics point of view, |session_started_| should not be > changed here, and it keeps the original semantics. > > For example, in the original code, "start -> ready -> crash -> (re-)start -> > ready -> crash -> (re-)start -> ..." is one of expected scenarios, which is not > reproducible if we reset |session_started_| here. > > In this new code, |reconnect_| flag is corresponding to "state() == READY". > SetState(STOPPED) below is corresponding to |reconnect_ = false| conceptually. > > I have left the current code and comment as is, because actually no semantics > change for |session_started_|. Of course welcome better way (like renaming var, > better comment), but I'd like to address it in the following CL, which focuses > on the state machine change. Actually the |session_started_| will be renamed to > |run_requested_| in the CL (thank you for suggestion!). > > Maybe another way is change the condition as follows; > > if (state() == State::READY || > (state() == State::STOPPING && session_started_)) { > DCHECK(session_started_); > : > } > > The first condition (state == READY) is corresponding to |reconnect_ = true| > case. > The second condition (state == STOPPING && session_started_) is for > RequestStop() immediately followed by RequestStart() case. > > Note that, if state == READY, session_started_ must be true always, because 1) > to be READY, RequestStart() must be called once, and 2) if even at least once > RequestStop() is called, the state is set to STOPPING. > > WDYT? I like the proposed condition much better, since it more closely aligns with the new semantics. Then again, I still think there should be *something* to notify the upper layers that this won't be restarted anymore. Maybe a different |stop_reason|? In any case, I'm fine with this particular item being resolved in a future change since it's not a regression.
errr. forgot to lgtm.
The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you for review. https://codereview.chromium.org/2582003002/diff/1/components/arc/arc_bridge_s... File components/arc/arc_bridge_service_impl.cc (right): https://codereview.chromium.org/2582003002/diff/1/components/arc/arc_bridge_s... components/arc/arc_bridge_service_impl.cc:168: if (session_started_ && On 2016/12/19 22:23:38, Luis Héctor Chávez wrote: > On 2016/12/19 07:41:21, hidehiko wrote: > > On 2016/12/16 23:02:13, Luis Héctor Chávez wrote: > > > In the same spirit as we did reset |reconnect_|, shouldn't you set > > > |session_started_ = false| here? Otherwise it's possible to call > > RequestStart() > > > if the state wasn't READY or STOPPING and have that be ignored. At the very > > > least it should be set to false in an else block. > > > > > > Otherwise, please mention that the semantics of |session_started_| have > > changed > > > to mean something like "the user has requested for the ARC instance to be > up". > > > > IIUC, unlike |reconnect_|, |session_started_| should not be set to false here. > > > > |reconnect_| is a flag representing if the current ARC instance gets READY > state > > already. On the other hand |session_started_| is a flag whether a client code > > calls RequestStart/Stop(). > > So, also from the semantics point of view, |session_started_| should not be > > changed here, and it keeps the original semantics. > > > > For example, in the original code, "start -> ready -> crash -> (re-)start -> > > ready -> crash -> (re-)start -> ..." is one of expected scenarios, which is > not > > reproducible if we reset |session_started_| here. > > > > In this new code, |reconnect_| flag is corresponding to "state() == READY". > > SetState(STOPPED) below is corresponding to |reconnect_ = false| conceptually. > > > > I have left the current code and comment as is, because actually no semantics > > change for |session_started_|. Of course welcome better way (like renaming > var, > > better comment), but I'd like to address it in the following CL, which focuses > > on the state machine change. Actually the |session_started_| will be renamed > to > > |run_requested_| in the CL (thank you for suggestion!). > > > > Maybe another way is change the condition as follows; > > > > if (state() == State::READY || > > (state() == State::STOPPING && session_started_)) { > > DCHECK(session_started_); > > : > > } > > > > The first condition (state == READY) is corresponding to |reconnect_ = true| > > case. > > The second condition (state == STOPPING && session_started_) is for > > RequestStop() immediately followed by RequestStart() case. > > > > Note that, if state == READY, session_started_ must be true always, because 1) > > to be READY, RequestStart() must be called once, and 2) if even at least once > > RequestStop() is called, the state is set to STOPPING. > > > > WDYT? > > I like the proposed condition much better, since it more closely aligns with the > new semantics. > Sure, done. Just in case, could you take another look? With the change, we need SetState(STOPPING) in Shutdown(). > Then again, I still think there should be *something* to notify the upper layers > that this won't be restarted anymore. Maybe a different |stop_reason|? In any > case, I'm fine with this particular item being resolved in a future change since > it's not a regression. +1. I added TODO for the record. Maybe new stop_reason value, or alternatively adding bool |retry_scheduled|? Anyway, let's discuss it separately.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm++!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hidehiko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1482258176478160,
"parent_rev": "abb23da7de6747261e637d18228648cd9bba4962", "commit_rev":
"cbd3f8be0b9e88e820035654460d4ec0daeffb06"}
Message was sent while issue was closed.
Description was changed from ========== Refactor ArcBridgeServiceImpl part 1. This CL refactors ArcBridgeServiceImpl focusing on the retry mechanism. - With this CL, it starts to use OneShotTimer, which supports cancel operation. - Rename "reconnect" to "restart" for wording consistency. - Introduce restart_delay_ for testing, instead of special bool flag. Also, for restarting of testing, get rid of synchronous PrerequisitesChanged() call, instead PostTask always. - Extract StartArcSession() for sharing PrerequisitesChanged() and restarting. - Added more DCHECKs. BUG=657687 BUG=b/31079732 TEST=Ran bots. ========== to ========== Refactor ArcBridgeServiceImpl part 1. This CL refactors ArcBridgeServiceImpl focusing on the retry mechanism. - With this CL, it starts to use OneShotTimer, which supports cancel operation. - Rename "reconnect" to "restart" for wording consistency. - Introduce restart_delay_ for testing, instead of special bool flag. Also, for restarting of testing, get rid of synchronous PrerequisitesChanged() call, instead PostTask always. - Extract StartArcSession() for sharing PrerequisitesChanged() and restarting. - Added more DCHECKs. BUG=657687 BUG=b/31079732 TEST=Ran bots. Review-Url: https://codereview.chromium.org/2582003002 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Refactor ArcBridgeServiceImpl part 1. This CL refactors ArcBridgeServiceImpl focusing on the retry mechanism. - With this CL, it starts to use OneShotTimer, which supports cancel operation. - Rename "reconnect" to "restart" for wording consistency. - Introduce restart_delay_ for testing, instead of special bool flag. Also, for restarting of testing, get rid of synchronous PrerequisitesChanged() call, instead PostTask always. - Extract StartArcSession() for sharing PrerequisitesChanged() and restarting. - Added more DCHECKs. BUG=657687 BUG=b/31079732 TEST=Ran bots. Review-Url: https://codereview.chromium.org/2582003002 ========== to ========== Refactor ArcBridgeServiceImpl part 1. This CL refactors ArcBridgeServiceImpl focusing on the retry mechanism. - With this CL, it starts to use OneShotTimer, which supports cancel operation. - Rename "reconnect" to "restart" for wording consistency. - Introduce restart_delay_ for testing, instead of special bool flag. Also, for restarting of testing, get rid of synchronous PrerequisitesChanged() call, instead PostTask always. - Extract StartArcSession() for sharing PrerequisitesChanged() and restarting. - Added more DCHECKs. BUG=657687 BUG=b/31079732 TEST=Ran bots. Committed: https://crrev.com/e18e9bcb729ce33f96a7b90c659cb5016bf97dbc Cr-Commit-Position: refs/heads/master@{#439850} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e18e9bcb729ce33f96a7b90c659cb5016bf97dbc Cr-Commit-Position: refs/heads/master@{#439850} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
