Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2766953002:
[HttpCache::Transaction] Force states to set the next state (Closed)
DescriptionThis change forces states called by DoLoop to set the next state before returning. The real difference is that before there was a default next_state_ = STATE_NONE, and now the state must set that itself, which improves readability and ensures that the state machine isn't escaping unexpectedly.
BUG=703923
Review-Url: https://codereview.chromium.org/2766953002
Cr-Commit-Position: refs/heads/master@{#458763}
Committed: https://chromium.googlesource.com/chromium/src/+/fa6b5af00244a43fcfd8ca0bacd7f44e11d590b2
Patch Set 1 #Patch Set 2 : Rename to STATE_UNSET #Patch Set 3 : Nit #
Total comments: 15
Patch Set 4 : Address comments from PS3 #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 36 (22 generated)
The CQ bit was checked by jkarlin@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...
Description was changed from ========== [HttpCache::Transaction] More explicit do loop states Require all states to explicitly set the next state, including STATE_NONE. BUG=703923 ========== to ========== Removes the default next_state_ = STATE_NONE. Sets it to an invalid state instead. This requires all states to explicitly set the next state, including STATE_NONE. BUG=703923 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
The CQ bit was checked by jkarlin@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...
Patchset #2 (id:20001) has been deleted
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jkarlin@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...
Description was changed from ========== Removes the default next_state_ = STATE_NONE. Sets it to an invalid state instead. This requires all states to explicitly set the next state, including STATE_NONE. BUG=703923 ========== to ========== This change forces states called by DoLoop to set the next state before returning. The real difference is that before there was a default next_state_ = STATE_NONE, and now the state must set that itself, which improves readability and ensures that the state machine isn't escaping unexpectedly. BUG=703923 ==========
The CQ bit was checked by jkarlin@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...
jkarlin@chromium.org changed reviewers: + rdsmith@chromium.org, shivanisha@chromium.org
shivanisha@ PTAL rdsmith@ PTAL for OWNERS Thanks!
On 2017/03/22 at 14:31:46, jkarlin wrote: > shivanisha@ PTAL > rdsmith@ PTAL for OWNERS > > Thanks! lgtm % clarification Thanks for this change!
https://codereview.chromium.org/2766953002/diff/60001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2766953002/diff/60001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1194: // TODO(jkarlin): We should either handle the case or DCHECK. Not sure what the "todo" suggests.
What process did you go through to confirm that you caught all the escape hatches? I'm looking for an excuse to not re-do the work of checking all those functions :-}. https://codereview.chromium.org/2766953002/diff/60001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2766953002/diff/60001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:716: DCHECK(next_state_ != STATE_NONE); Suggestion: DCHECK_NE for both? https://codereview.chromium.org/2766953002/diff/60001/net/http/http_cache_tra... File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/2766953002/diff/60001/net/http/http_cache_tra... net/http/http_cache_transaction.h:1: nit: Why?
Oh, I'm just here for *OWNERS* :-}. LGTM; I'm happy to rely on Shivani's review.
On 2017/03/22 at 14:48:29, shivanisha wrote: > https://codereview.chromium.org/2766953002/diff/60001/net/http/http_cache_tra... > File net/http/http_cache_transaction.cc (right): > > https://codereview.chromium.org/2766953002/diff/60001/net/http/http_cache_tra... > net/http/http_cache_transaction.cc:1194: // TODO(jkarlin): We should either handle the case or DCHECK. > Not sure what the "todo" suggests. Giving a second look to check for any missed cases.
The process I went through: 1) Manually went through every function looking for places that the state is unset and set it to STATE_NONE 2) Relied on tests to find anything that I missed. I explicitly did *not* look for logic errors (e.g., should it actually be STATE_NONE here?). https://codereview.chromium.org/2766953002/diff/60001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2766953002/diff/60001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:716: DCHECK(next_state_ != STATE_NONE); On 2017/03/22 14:49:44, Randy Smith (Not in Mondays) wrote: > Suggestion: DCHECK_NE for both? Done. https://codereview.chromium.org/2766953002/diff/60001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1194: // TODO(jkarlin): We should either handle the case or DCHECK. On 2017/03/22 14:48:29, shivanisha wrote: > Not sure what the "todo" suggests. It's against Chromium style to NOTREACHED/DCHECK if you also handle the case (e.g., return something) (see the condition below). So the TODO is to either DCHECK or just handle the condition. https://codereview.chromium.org/2766953002/diff/60001/net/http/http_cache_tra... File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/2766953002/diff/60001/net/http/http_cache_tra... net/http/http_cache_transaction.h:1: On 2017/03/22 14:49:44, Randy Smith (Not in Mondays) wrote: > nit: Why? Done.
The CQ bit was checked by jkarlin@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...
No misses found :) Some suggestions added. https://codereview.chromium.org/2766953002/diff/60001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2766953002/diff/60001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1329: return result; Since this Do function is setting the state in the helper functions, a dcheck before return would be good. DCHECK_NE(next_state_, STATE_UNSET) https://codereview.chromium.org/2766953002/diff/60001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1344: return ValidateEntryHeadersAndContinue(); Since this Do function is setting the state in the helper functions, a dcheck before return would be good. rv = ValidateEntryHeadersAndContinue(); DCHECK_NE(next_state_, STATE_UNSET) return rv; https://codereview.chromium.org/2766953002/diff/60001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1384: return BeginCacheValidation(); Since this Do function is setting the state in the helper functions, a dcheck before return would be good. https://codereview.chromium.org/2766953002/diff/60001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1597: next_state_ = STATE_CACHE_WRITE_UPDATED_RESPONSE; Nothing to change here but pointing out for reference, this is one scenario where a Do function is setting a state in the beginning and then changing it based on a condition.
Thanks! https://codereview.chromium.org/2766953002/diff/60001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2766953002/diff/60001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1329: return result; On 2017/03/22 15:19:48, shivanisha wrote: > Since this Do function is setting the state in the helper functions, a dcheck > before return would be good. > DCHECK_NE(next_state_, STATE_UNSET) That seems a bit redundant to me as the DCHECK will happen as soon as this returns. https://codereview.chromium.org/2766953002/diff/60001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1344: return ValidateEntryHeadersAndContinue(); On 2017/03/22 15:19:48, shivanisha wrote: > Since this Do function is setting the state in the helper functions, a dcheck > before return would be good. > rv = ValidateEntryHeadersAndContinue(); > DCHECK_NE(next_state_, STATE_UNSET) > return rv; Acknowledged. https://codereview.chromium.org/2766953002/diff/60001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1384: return BeginCacheValidation(); On 2017/03/22 15:19:48, shivanisha wrote: > Since this Do function is setting the state in the helper functions, a dcheck > before return would be good. Acknowledged. https://codereview.chromium.org/2766953002/diff/60001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1597: next_state_ = STATE_CACHE_WRITE_UPDATED_RESPONSE; On 2017/03/22 15:19:48, shivanisha wrote: > Nothing to change here but pointing out for reference, this is one scenario > where a Do function is setting a state in the beginning and then changing it > based on a condition. Yep, it's not so great. Addressed in the follow-up CL: https://codereview.chromium.org/2767033003/
https://codereview.chromium.org/2766953002/diff/60001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2766953002/diff/60001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1329: return result; On 2017/03/22 at 15:25:02, jkarlin wrote: > On 2017/03/22 15:19:48, shivanisha wrote: > > Since this Do function is setting the state in the helper functions, a dcheck > > before return would be good. > > DCHECK_NE(next_state_, STATE_UNSET) > > That seems a bit redundant to me as the DCHECK will happen as soon as this returns. Makes sense.
On 2017/03/22 at 15:31:53, shivanisha wrote: > https://codereview.chromium.org/2766953002/diff/60001/net/http/http_cache_tra... > File net/http/http_cache_transaction.cc (right): > > https://codereview.chromium.org/2766953002/diff/60001/net/http/http_cache_tra... > net/http/http_cache_transaction.cc:1329: return result; > On 2017/03/22 at 15:25:02, jkarlin wrote: > > On 2017/03/22 15:19:48, shivanisha wrote: > > > Since this Do function is setting the state in the helper functions, a dcheck > > > before return would be good. > > > DCHECK_NE(next_state_, STATE_UNSET) > > > > That seems a bit redundant to me as the DCHECK will happen as soon as this returns. > > Makes sense. lgtm
The CQ bit was unchecked by jkarlin@chromium.org
The CQ bit was checked by jkarlin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org Link to the patchset: https://codereview.chromium.org/2766953002/#ps80001 (title: "Address comments from PS3")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/22 15:04:26, jkarlin wrote: > The process I went through: > > 1) Manually went through every function looking for places that the state is > unset and set it to STATE_NONE > > 2) Relied on tests to find anything that I missed. > > I explicitly did *not* look for logic errors (e.g., should it actually be > STATE_NONE here?). The failure mode that concerns me about this process is missing a subroutine call that set the state and thus accidentally overriding a state transition with setting to NONE. I'd recommend following up this CL with the next one that DCHECKs that the state setting isn't overriding a previous state fairly soon.
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490196778352220,
"parent_rev": "588960fc36ee4335083ae671f7109b5b5f1d4d08", "commit_rev":
"fa6b5af00244a43fcfd8ca0bacd7f44e11d590b2"}
Message was sent while issue was closed.
Description was changed from ========== This change forces states called by DoLoop to set the next state before returning. The real difference is that before there was a default next_state_ = STATE_NONE, and now the state must set that itself, which improves readability and ensures that the state machine isn't escaping unexpectedly. BUG=703923 ========== to ========== This change forces states called by DoLoop to set the next state before returning. The real difference is that before there was a default next_state_ = STATE_NONE, and now the state must set that itself, which improves readability and ensures that the state machine isn't escaping unexpectedly. BUG=703923 Review-Url: https://codereview.chromium.org/2766953002 Cr-Commit-Position: refs/heads/master@{#458763} Committed: https://chromium.googlesource.com/chromium/src/+/fa6b5af00244a43fcfd8ca0bacd7... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/fa6b5af00244a43fcfd8ca0bacd7... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
