Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2774603003:
Doom and create new entry when validation is not a match (Closed)
|
Created:
3 years, 9 months ago by shivanisha Modified:
3 years, 5 months ago CC:
cbentzel+watch_chromium.org, chromium-reviews, gavinp+disk_chromium.org, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionIf parallel validation leads to a no match then in addition to dooming this entry, also create a new entry.
BUG=472740
Review-Url: https://codereview.chromium.org/2774603003
Cr-Commit-Position: refs/heads/master@{#484365}
Committed: https://chromium.googlesource.com/chromium/src/+/36a816e7d1de8c3e1a2a720b05743ceb8405653d
Patch Set 1 #Patch Set 2 : Used TransitionToState in DoSuccessfulSendRequest #
Total comments: 8
Patch Set 3 : Rebased with parent branch #Patch Set 4 : Added a new state + tests #
Total comments: 10
Patch Set 5 : Rebased with parent branch #Patch Set 6 : Feedback addressed #Patch Set 7 : Race handling in doom and create case. #Patch Set 8 : Rebased with parent branch #
Total comments: 9
Patch Set 9 : Refactored/Changed no-match handling in HttpCache. #Patch Set 10 : Rebased with parent branch #
Total comments: 13
Patch Set 11 : Feedback addressed. #
Total comments: 24
Patch Set 12 : Rebased with refs/heads/master@{#482382} #Patch Set 13 : Feedback addressed, test added #Patch Set 14 : Rebased with refs/heads/master@{#484325} #
Dependent Patchsets: Messages
Total messages: 81 (62 generated)
shivanisha@chromium.org changed reviewers: + jkarlin@chromium.org, rdsmith@chromium.org
This is a follow up CL to parallel cache validation. If parallel validation leads to a no match then in addition to dooming this entry, also create a new entry. Tests will follow.
First pass comments. https://codereview.chromium.org/2774603003/diff/20001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2774603003/diff/20001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1140: mode_ = NONE; I presume this means we're transitioning into a mode where we aren't touching the cache, just piping through the results of a network transaction? Comment to that effect? (Not in code you're touching, but it's a simple cleanup.) https://codereview.chromium.org/2774603003/diff/20001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1620: if (!entry_) { Could this conditional be more succinctly implemented by eliminate this line and the next and testing for entry in the conditional below? https://codereview.chromium.org/2774603003/diff/20001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1632: TransitionToState(STATE_CREATE_ENTRY); As I understand the logic that follows on from this, it's dependent on the response_code() being 200, not just not being 304? https://codereview.chromium.org/2774603003/diff/20001/net/http/http_cache_tra... File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/2774603003/diff/20001/net/http/http_cache_tra... net/http/http_cache_transaction.h:483: bool done_headers_; // Headers have been received from the network. Alternative design--I'm not sure this is a good idea, I'm raising it in a brainstorming mode: done_headers_ is in some ways a state variable--it's saying "This transaction already has a request attached to it on which full data for this entry is incoming." This CL is using that variable for tweaking behavior in the CREATE_ENTRY, ADD_TRANSACTION_TO_ENTRY, and ADD_TRANSACTION_TO_ENTRY_COMPLETE states. An alternative design would be to duplicate those three states for the doom-and-create path. I think the code for those three state functions would be simpler than the ones it would be copied from (no range requests, no bypass_lock_for_test_), and if thinking about the operation of this class at the state machine abstraction, I think it would be simpler and clearer. WDYT?
Patchset #3 (id:40001) has been deleted
Patchset #4 (id:80001) has been deleted
PTAL, Thanks. https://codereview.chromium.org/2774603003/diff/20001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2774603003/diff/20001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1140: mode_ = NONE; On 2017/04/04 at 21:54:03, Randy Smith (Not in Mondays) wrote: > I presume this means we're transitioning into a mode where we aren't touching the cache, just piping through the results of a network transaction? Comment to that effect? (Not in code you're touching, but it's a simple cleanup.) done https://codereview.chromium.org/2774603003/diff/20001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1620: if (!entry_) { On 2017/04/04 at 21:54:03, Randy Smith (Not in Mondays) wrote: > Could this conditional be more succinctly implemented by eliminate this line and the next and testing for entry in the conditional below? N/A now that this code is implemented in DoCacheWriteResponse as part of changes in the parent branch. https://codereview.chromium.org/2774603003/diff/20001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1632: TransitionToState(STATE_CREATE_ENTRY); On 2017/04/04 at 21:54:03, Randy Smith (Not in Mondays) wrote: > As I understand the logic that follows on from this, it's dependent on the response_code() being 200, not just not being 304? Note that the state OVERWRITE_CACHED_RESPONSE being reached implies that the headers should be written (either to this entry or to a new entry). If the response is 304, its ok to write to this entry but in any other case it should be checked if it is to be written to this entry or another entry. https://codereview.chromium.org/2774603003/diff/20001/net/http/http_cache_tra... File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/2774603003/diff/20001/net/http/http_cache_tra... net/http/http_cache_transaction.h:483: bool done_headers_; // Headers have been received from the network. On 2017/04/04 at 21:54:03, Randy Smith (Not in Mondays) wrote: > Alternative design--I'm not sure this is a good idea, I'm raising it in a brainstorming mode: done_headers_ is in some ways a state variable--it's saying "This transaction already has a request attached to it on which full data for this entry is incoming." This CL is using that variable for tweaking behavior in the CREATE_ENTRY, ADD_TRANSACTION_TO_ENTRY, and ADD_TRANSACTION_TO_ENTRY_COMPLETE states. An alternative design would be to duplicate those three states for the doom-and-create path. I think the code for those three state functions would be simpler than the ones it would be copied from (no range requests, no bypass_lock_for_test_), and if thinking about the operation of this class at the state machine abstraction, I think it would be simpler and clearer. WDYT? done_headers_ is used in CREATE_ENTRY_COMPLETE and ADD_TO_ENTRY_COMPLETE. Created a DONE_HEADERS_ADD_TO_ENTRY_COMPLETE state since that's the state which differs the most from the original ADD_TO_ENTRY_COMPLETE state. Renamed done_headers_ to done_headers_create_new_entry_ to be more specific of its purpose and also included its resetting after use.
Pretty close to stamp; the only thing stopping me from a conditional stamp is
the apparent double post.
(I still feel a little tempted by the three new states--I dislike having
information that at a top level of about the overall state of the object both
represented via a state_ variable and by extra booleans. But that way can lie
combinatorial explosions :-} :-{.
https://codereview.chromium.org/2774603003/diff/100001/net/http/http_cache_tr...
File net/http/http_cache_transaction.cc (right):
https://codereview.chromium.org/2774603003/diff/100001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:1119: // Change the mode to not write
anything to the cache.
This comment doesn't really add anything to the code; that's what "mode_ =
NONE;" means. "Switching into passing through data directly from the network,
avoiding the cache entry" was more what I had in mind.
https://codereview.chromium.org/2774603003/diff/100001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:1121: if (done_headers_create_new_entry_) {
Maybe a comment here something like "The headers to use to write the response
have already been received as a result of validation, triggering the doom of the
old entry. So no network request needs to be sent."?
That comment's not ideal because the setting of the STATE_SEND_REQUEST is a ways
away from the point of comment, so if you want to fix that (by changing the
comment or restructuring the code) I'm good with that. But I'd like to indicate
in comment what (at a high level, not just repeating the information in the
code) is being done by the conditional.
https://codereview.chromium.org/2774603003/diff/100001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:1148:
TransitionToState(STATE_ADD_TO_ENTRY_COMPLETE);
The naming makes it clear that these are parallel states, which is going to
raise the question in a code reader why the code within this state isn't
parallel. Could you either make the code paths parallel (i.e. I'm not sure
there's harm in using the timeout path for the doom-and-create state, though we
know that the cache lock won't be held if we're creating a new entry) or put in
a comment about why they aren't parallel?
https://codereview.chromium.org/2774603003/diff/100001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:1180:
TimeDelta::FromMilliseconds(timeout_milliseconds));
Why is this pulled out of the conditional? It looks like there'll be a double
post in the test case.
https://codereview.chromium.org/2774603003/diff/100001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:1245: int
HttpCache::Transaction::DoDoneHeadersAddToEntryComplete(int result) {
Comment somewhere as to the context/motivation for the differences between this
and DoAddToEntryComplete?
The CQ bit was checked by shivanisha@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 checked by shivanisha@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 checked by shivanisha@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 #6 (id:140001) has been deleted
Patchset #6 (id:160001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_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 shivanisha@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: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by shivanisha@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 #7 (id:200001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by shivanisha@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...
Please note the following major changes in addition to feedback and rebasing with parent branch: 1. Its possible that when done_headers_create_new_entry_ case is true, apart from the current entry being doomed, there was another entry in existence. That can happen because transactions that have WRITE mode set can doom and create a new entry even before getting added to the original entry. In those cases our transaction should not be added to that 2nd entry but should doom that as well and create a third entry. This can be done by setting the mode_ as WRITE and state as STATE_INIT_ENTRY instead of STATE_CREATE_ENTRY. 2. Also its possible that this transaction gets ERR_CACHE_RACE in either DoCreateEntryComplete or DoDoomEntryComplete. In both cases we cannot invoke STATE_HEADERS_PHASE_CANNOT_PROCEED as that state resets all the fields of the transaction. Thus instead of having a condition based on done_headers_create_new_entry_, its better to just go to STATE_INIT_ENTRY unconditionally on getting ERR_CACHE_RACE because the resetting being done in STATE_HEADERS_PHASE_CANNOT_PROCEED is anyways a no-op for a transaction before it is added to entry. It was being invoked there only for consistency. 3. In HttpCache::ProcessEntryFailure instead of invoking the callbacks of all queued transactions with ERR_CACHE_RACE as function calls, I have added a post task. This is done because if its a doom and create entry case, then a function call will mean a queued transaction's state machine gets to create a new entry sooner leading to a race condition for the transaction that was doing the doom and create and it will then create another entry because of 2. https://codereview.chromium.org/2774603003/diff/100001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2774603003/diff/100001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1119: // Change the mode to not write anything to the cache. On 2017/04/11 at 19:41:07, Randy Smith (Not in Mondays) wrote: > This comment doesn't really add anything to the code; that's what "mode_ = NONE;" means. "Switching into passing through data directly from the network, avoiding the cache entry" was more what I had in mind. Done https://codereview.chromium.org/2774603003/diff/100001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1121: if (done_headers_create_new_entry_) { On 2017/04/11 at 19:41:07, Randy Smith (Not in Mondays) wrote: > Maybe a comment here something like "The headers to use to write the response have already been received as a result of validation, triggering the doom of the old entry. So no network request needs to be sent."? > > That comment's not ideal because the setting of the STATE_SEND_REQUEST is a ways away from the point of comment, so if you want to fix that (by changing the comment or restructuring the code) I'm good with that. But I'd like to indicate in comment what (at a high level, not just repeating the information in the code) is being done by the conditional. Comment added and restructured the code. https://codereview.chromium.org/2774603003/diff/100001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1148: TransitionToState(STATE_ADD_TO_ENTRY_COMPLETE); On 2017/04/11 at 19:41:07, Randy Smith (Not in Mondays) wrote: > The naming makes it clear that these are parallel states, which is going to raise the question in a code reader why the code within this state isn't parallel. Could you either make the code paths parallel (i.e. I'm not sure there's harm in using the timeout path for the doom-and-create state, though we know that the cache lock won't be held if we're creating a new entry) or put in a comment about why they aren't parallel? Added a comment in the new Do* function. https://codereview.chromium.org/2774603003/diff/100001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1180: TimeDelta::FromMilliseconds(timeout_milliseconds)); On 2017/04/11 at 19:41:07, Randy Smith (Not in Mondays) wrote: > Why is this pulled out of the conditional? It looks like there'll be a double post in the test case. It is not out of the else conditional. This post task is part of the else but out of the if (partial ...) case as in original code, so no double post. I think the confusion arises because the diff view and that's because the indentation changed from original as there isn't an if (rv == ERR_IO_PENDING) outside the whole post-task logic. https://codereview.chromium.org/2774603003/diff/100001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1245: int HttpCache::Transaction::DoDoneHeadersAddToEntryComplete(int result) { On 2017/04/11 at 19:41:07, Randy Smith (Not in Mondays) wrote: > Comment somewhere as to the context/motivation for the differences between this and DoAddToEntryComplete? done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #8 (id:240001) has been deleted
The CQ bit was checked by shivanisha@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: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #8 (id:260001) has been deleted
Patchset #8 (id:280001) has been deleted
The CQ bit was checked by shivanisha@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: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
LGTM--all of the below are suggestions/thoughts for the future. https://codereview.chromium.org/2774603003/diff/300001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2774603003/diff/300001/net/http/http_cache.cc... net/http/http_cache.cc:986: FROM_HERE, base::Bind(transaction->io_callback(), net::ERR_CACHE_RACE)); Random thought, not for this CL: If we do this everywhere where we call the io_callback(), would that let us switch over to a loop based approach for managing the queues? Maybe even just loop on the main thread, pushing callbacks off until after we've done all state transitions? We could even batch the callbacks, queueing them up and then having a single post task that runs them all after the queue processing is complete. https://codereview.chromium.org/2774603003/diff/300001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2774603003/diff/300001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1151: TransitionToState(STATE_DONE_HEADERS_ADD_TO_ENTRY_COMPLETE); It's my belief that short-circuiting all the logic below about the cache lock in this case is ok because we know that there aren't any other transactions racing on this entry, since we just created it. If that's true, could you put in a comment about it? (I'd love to have a DCHECK as well, but that probably requires reaching into the cache's active entry which might be a layering violation, so I'll just mention it rather than request it.) https://codereview.chromium.org/2774603003/diff/300001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1260: // the newly created entry. Preface second sentence with something like "A response of no-match from a validation request also includes the full contents of the URL, so ..."? (Suggestion.) Possibly (also suggestion) also add a note about how a no-match will always return the full contents of the entry, so the logic around partial and truncated responses in DoAddToEntryComplete isn't necessary in this version. https://codereview.chromium.org/2774603003/diff/300001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1772: next_state_ = STATE_INIT_ENTRY; nit: TransitionToState? (& move the TransitionToState() above after the if block). https://codereview.chromium.org/2774603003/diff/300001/net/http/http_cache_tr... File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/2774603003/diff/300001/net/http/http_cache_tr... net/http/http_cache_transaction.h:201: STATE_DONE_HEADERS_ADD_TO_ENTRY_COMPLETE, I'm not completely happy with the "done headers" nomenclature (here and in the boolean); it doesn't have high level semantic meaning to me. Maybe replace it with "failed/no match overwrite/replace"? Just a thought; up to you.
After this patch is rebased I'll take a pass as well.
On 2017/05/26 at 13:22:34, jkarlin wrote: > After this patch is rebased I'll take a pass as well. That's great. I will be adding a patch to it based on CL1.
Patchset #9 (id:320001) has been deleted
Patchset #9 (id:340001) has been deleted
Patchset #9 (id:360001) has been deleted
Patchset #9 (id:340002) has been deleted
PTAL, Thanks! A lot of changes happened in the parent branch after this was last reviewed. It was tricky to separate the rebasing changes with the changes in this patch set so the changes only in this patch set are explained below. Sorry about that. The latest patch has changes in HttpCache's handling of no-match, because of the following issues in the earlier patch: Issues: 1. When a transaction T1 gets a validation no-match it dooms the entry and creates a new entry. In addition to dooming the old entry, add_to_entry_queue transactions are also restarted. To make sure these transactions do not race with T1 to create an entry, I posted their restart as a task instead of a function call. This can lead to a race where if one of these pending transactions' destructor is called. The destructor will see cache_pending_ set and try to search it in one of the entries asserting it should be found. Solution: Called ResetCachePendingState() while posting task for the queued transactions. 2. Earlier, the "validation and no-match" case was handled in ProcessEntryFailure but there are 2 issues with using the ProcessEntryFailure logic: done_headers_queue transactions are also restarted which should not happen with validation-no-match case and tasks are posted for all add_to_entry_queue transactions which can be avoided (for performance reasons) in case its an actual failure case. Solution: Added a new function DoomEntryValidationNoMatch() for this case. https://codereview.chromium.org/2774603003/diff/300001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2774603003/diff/300001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1151: TransitionToState(STATE_DONE_HEADERS_ADD_TO_ENTRY_COMPLETE); On 2017/04/27 at 17:47:12, Randy Smith (Not in Mondays) wrote: > It's my belief that short-circuiting all the logic below about the cache lock in this case is ok because we know that there aren't any other transactions racing on this entry, since we just created it. If that's true, could you put in a comment about it? (I'd love to have a DCHECK as well, but that probably requires reaching into the cache's active entry which might be a layering violation, so I'll just mention it rather than request it.) Added a comment. https://codereview.chromium.org/2774603003/diff/300001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1260: // the newly created entry. On 2017/04/27 at 17:47:12, Randy Smith (Not in Mondays) wrote: > Preface second sentence with something like "A response of no-match from a validation request also includes the full contents of the URL, so ..."? (Suggestion.) > Done > Possibly (also suggestion) also add a note about how a no-match will always return the full contents of the entry, so the logic around partial and truncated responses in DoAddToEntryComplete isn't necessary in this version. The logic around partial is also to do with cache lock timeout and since this flow should never be subjected to cache lock, none of that logic makes sense for this flow. Added a comment for that. https://codereview.chromium.org/2774603003/diff/300001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1772: next_state_ = STATE_INIT_ENTRY; On 2017/04/27 at 17:47:12, Randy Smith (Not in Mondays) wrote: > nit: TransitionToState? (& move the TransitionToState() above after the if block). done https://codereview.chromium.org/2774603003/diff/300001/net/http/http_cache_tr... File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/2774603003/diff/300001/net/http/http_cache_tr... net/http/http_cache_transaction.h:201: STATE_DONE_HEADERS_ADD_TO_ENTRY_COMPLETE, On 2017/04/27 at 17:47:12, Randy Smith (Not in Mondays) wrote: > I'm not completely happy with the "done headers" nomenclature (here and in the boolean); it doesn't have high level semantic meaning to me. Maybe replace it with "failed/no match overwrite/replace"? Just a thought; up to you. I am inclined to keep it because it is in line with the various ways this state machine is now demarcating headers vs response body phases. Other places where we are using "headers" is STATE_FINISH_HEADERS* and DoneWithResponseHeaders()
On 2017/05/31 15:51:47, shivanisha wrote: > PTAL, Thanks! > A lot of changes happened in the parent branch after this was last reviewed. It > was tricky to separate the rebasing changes with the changes in this patch set > so the changes only in this patch set are explained below. Sorry about that. > > The latest patch has changes in HttpCache's handling of no-match, because of the > following issues in the earlier patch: > > Issues: > 1. When a transaction T1 gets a validation no-match it dooms the entry and > creates a new entry. In addition to dooming the old entry, add_to_entry_queue > transactions are also restarted. To make sure these transactions do not race > with T1 to create an entry, I posted their restart as a task instead of a > function call. This can lead to a race where if one of these pending > transactions' destructor is called. The destructor will see cache_pending_ set > and try to search it in one of the entries asserting it should be found. > > Solution: Called ResetCachePendingState() while posting task for the queued > transactions. > > 2. Earlier, the "validation and no-match" case was handled in > ProcessEntryFailure but there are 2 issues with using the ProcessEntryFailure > logic: done_headers_queue transactions are also restarted which should not > happen with validation-no-match case and tasks are posted for all > add_to_entry_queue transactions which can be avoided (for performance reasons) > in case its an actual failure case. > > Solution: Added a new function DoomEntryValidationNoMatch() for this case. > > https://codereview.chromium.org/2774603003/diff/300001/net/http/http_cache_tr... > File net/http/http_cache_transaction.cc (right): > > https://codereview.chromium.org/2774603003/diff/300001/net/http/http_cache_tr... > net/http/http_cache_transaction.cc:1151: > TransitionToState(STATE_DONE_HEADERS_ADD_TO_ENTRY_COMPLETE); > On 2017/04/27 at 17:47:12, Randy Smith (Not in Mondays) wrote: > > It's my belief that short-circuiting all the logic below about the cache lock > in this case is ok because we know that there aren't any other transactions > racing on this entry, since we just created it. If that's true, could you put > in a comment about it? (I'd love to have a DCHECK as well, but that probably > requires reaching into the cache's active entry which might be a layering > violation, so I'll just mention it rather than request it.) > > Added a comment. > > https://codereview.chromium.org/2774603003/diff/300001/net/http/http_cache_tr... > net/http/http_cache_transaction.cc:1260: // the newly created entry. > On 2017/04/27 at 17:47:12, Randy Smith (Not in Mondays) wrote: > > Preface second sentence with something like "A response of no-match from a > validation request also includes the full contents of the URL, so ..."? > (Suggestion.) > > > Done > > > Possibly (also suggestion) also add a note about how a no-match will always > return the full contents of the entry, so the logic around partial and truncated > responses in DoAddToEntryComplete isn't necessary in this version. > > The logic around partial is also to do with cache lock timeout and since this > flow should never be subjected to cache lock, none of that logic makes sense for > this flow. Added a comment for that. > > https://codereview.chromium.org/2774603003/diff/300001/net/http/http_cache_tr... > net/http/http_cache_transaction.cc:1772: next_state_ = STATE_INIT_ENTRY; > On 2017/04/27 at 17:47:12, Randy Smith (Not in Mondays) wrote: > > nit: TransitionToState? (& move the TransitionToState() above after the if > block). > > done > > https://codereview.chromium.org/2774603003/diff/300001/net/http/http_cache_tr... > File net/http/http_cache_transaction.h (right): > > https://codereview.chromium.org/2774603003/diff/300001/net/http/http_cache_tr... > net/http/http_cache_transaction.h:201: STATE_DONE_HEADERS_ADD_TO_ENTRY_COMPLETE, > On 2017/04/27 at 17:47:12, Randy Smith (Not in Mondays) wrote: > > I'm not completely happy with the "done headers" nomenclature (here and in the > boolean); it doesn't have high level semantic meaning to me. Maybe replace it > with "failed/no match overwrite/replace"? Just a thought; up to you. > > I am inclined to keep it because it is in line with the various ways this state > machine is now demarcating headers vs response body phases. Other places where > we are using "headers" is STATE_FINISH_HEADERS* and DoneWithResponseHeaders() Shivani, Josh: Do you want me to take another look at this based on the changes, or do you think my stamp + Josh's evaluation is good enough? I'm obviously biased towards lazy :-}, but I'm happy to take another look if you'd like me to.
On 2017/05/31 at 16:04:36, rdsmith wrote: > On 2017/05/31 15:51:47, shivanisha wrote: > > PTAL, Thanks! > > A lot of changes happened in the parent branch after this was last reviewed. It > > was tricky to separate the rebasing changes with the changes in this patch set > > so the changes only in this patch set are explained below. Sorry about that. > > > > The latest patch has changes in HttpCache's handling of no-match, because of the > > following issues in the earlier patch: > > > > Issues: > > 1. When a transaction T1 gets a validation no-match it dooms the entry and > > creates a new entry. In addition to dooming the old entry, add_to_entry_queue > > transactions are also restarted. To make sure these transactions do not race > > with T1 to create an entry, I posted their restart as a task instead of a > > function call. This can lead to a race where if one of these pending > > transactions' destructor is called. The destructor will see cache_pending_ set > > and try to search it in one of the entries asserting it should be found. > > > > Solution: Called ResetCachePendingState() while posting task for the queued > > transactions. > > > > 2. Earlier, the "validation and no-match" case was handled in > > ProcessEntryFailure but there are 2 issues with using the ProcessEntryFailure > > logic: done_headers_queue transactions are also restarted which should not > > happen with validation-no-match case and tasks are posted for all > > add_to_entry_queue transactions which can be avoided (for performance reasons) > > in case its an actual failure case. > > > > Solution: Added a new function DoomEntryValidationNoMatch() for this case. > > > > https://codereview.chromium.org/2774603003/diff/300001/net/http/http_cache_tr... > > File net/http/http_cache_transaction.cc (right): > > > > https://codereview.chromium.org/2774603003/diff/300001/net/http/http_cache_tr... > > net/http/http_cache_transaction.cc:1151: > > TransitionToState(STATE_DONE_HEADERS_ADD_TO_ENTRY_COMPLETE); > > On 2017/04/27 at 17:47:12, Randy Smith (Not in Mondays) wrote: > > > It's my belief that short-circuiting all the logic below about the cache lock > > in this case is ok because we know that there aren't any other transactions > > racing on this entry, since we just created it. If that's true, could you put > > in a comment about it? (I'd love to have a DCHECK as well, but that probably > > requires reaching into the cache's active entry which might be a layering > > violation, so I'll just mention it rather than request it.) > > > > Added a comment. > > > > https://codereview.chromium.org/2774603003/diff/300001/net/http/http_cache_tr... > > net/http/http_cache_transaction.cc:1260: // the newly created entry. > > On 2017/04/27 at 17:47:12, Randy Smith (Not in Mondays) wrote: > > > Preface second sentence with something like "A response of no-match from a > > validation request also includes the full contents of the URL, so ..."? > > (Suggestion.) > > > > > Done > > > > > Possibly (also suggestion) also add a note about how a no-match will always > > return the full contents of the entry, so the logic around partial and truncated > > responses in DoAddToEntryComplete isn't necessary in this version. > > > > The logic around partial is also to do with cache lock timeout and since this > > flow should never be subjected to cache lock, none of that logic makes sense for > > this flow. Added a comment for that. > > > > https://codereview.chromium.org/2774603003/diff/300001/net/http/http_cache_tr... > > net/http/http_cache_transaction.cc:1772: next_state_ = STATE_INIT_ENTRY; > > On 2017/04/27 at 17:47:12, Randy Smith (Not in Mondays) wrote: > > > nit: TransitionToState? (& move the TransitionToState() above after the if > > block). > > > > done > > > > https://codereview.chromium.org/2774603003/diff/300001/net/http/http_cache_tr... > > File net/http/http_cache_transaction.h (right): > > > > https://codereview.chromium.org/2774603003/diff/300001/net/http/http_cache_tr... > > net/http/http_cache_transaction.h:201: STATE_DONE_HEADERS_ADD_TO_ENTRY_COMPLETE, > > On 2017/04/27 at 17:47:12, Randy Smith (Not in Mondays) wrote: > > > I'm not completely happy with the "done headers" nomenclature (here and in the > > boolean); it doesn't have high level semantic meaning to me. Maybe replace it > > with "failed/no match overwrite/replace"? Just a thought; up to you. > > > > I am inclined to keep it because it is in line with the various ways this state > > machine is now demarcating headers vs response body phases. Other places where > > we are using "headers" is STATE_FINISH_HEADERS* and DoneWithResponseHeaders() > > Shivani, Josh: Do you want me to take another look at this based on the changes, or do you think my stamp + Josh's evaluation is good enough? I'm obviously biased towards lazy :-}, but I'm happy to take another look if you'd like me to. Randy, I would be fine with your stamp + Josh's review.
The CQ bit was checked by shivanisha@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: 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_...)
Looking good https://codereview.chromium.org/2774603003/diff/410001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2774603003/diff/410001/net/http/http_cache.cc... net/http/http_cache.cc:953: // TODO(shivanisha) Can transaction be a writer in case of partial requests? s/a writer/the writer/ https://codereview.chromium.org/2774603003/diff/410001/net/http/http_cache.cc... net/http/http_cache.cc:954: // Handlie the writer case if that's possible. Seems like we need to answer that question before we land this CL. https://codereview.chromium.org/2774603003/diff/410001/net/http/http_cache.cc... net/http/http_cache.cc:954: // Handlie the writer case if that's possible. s/Handlie/Handle/ https://codereview.chromium.org/2774603003/diff/410001/net/http/http_cache.cc... net/http/http_cache.cc:955: CHECK(transaction == entry->headers_transaction); Let's DCHECK this and save CHECK for cases where we're debugging known problems. https://codereview.chromium.org/2774603003/diff/410001/net/http/http_cache.cc... net/http/http_cache.cc:972: transaction->ResetCachePendingState(); Why is this necessary? Won't they all have this reset in DoAddToEntryComplete? https://codereview.chromium.org/2774603003/diff/410001/net/http/http_cache_tr... File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/2774603003/diff/410001/net/http/http_cache_tr... net/http/http_cache_transaction.h:501: // Headers have been received from the network and it's not a match with the newline above this comment https://codereview.chromium.org/2774603003/diff/410001/net/http/http_cache_tr... net/http/http_cache_transaction.h:503: bool done_headers_create_new_entry_; newline below the member
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
PTAL, Thanks! Addressed latest feedback. https://codereview.chromium.org/2774603003/diff/410001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2774603003/diff/410001/net/http/http_cache.cc... net/http/http_cache.cc:953: // TODO(shivanisha) Can transaction be a writer in case of partial requests? On 2017/06/09 at 17:04:49, jkarlin wrote: > s/a writer/the writer/ done https://codereview.chromium.org/2774603003/diff/410001/net/http/http_cache.cc... net/http/http_cache.cc:954: // Handlie the writer case if that's possible. On 2017/06/09 at 17:04:49, jkarlin wrote: > s/Handlie/Handle/ Handled this TODO in CanTransactionWriteResponseHeaders in the parent branch so that this function should not be invoked in that scenario. https://codereview.chromium.org/2774603003/diff/410001/net/http/http_cache.cc... net/http/http_cache.cc:955: CHECK(transaction == entry->headers_transaction); On 2017/06/09 at 17:04:49, jkarlin wrote: > Let's DCHECK this and save CHECK for cases where we're debugging known problems. done https://codereview.chromium.org/2774603003/diff/410001/net/http/http_cache.cc... net/http/http_cache.cc:972: transaction->ResetCachePendingState(); On 2017/06/09 at 17:04:49, jkarlin wrote: > Why is this necessary? Won't they all have this reset in DoAddToEntryComplete? I am thinking of the race where we remove them from entry here and post the task. Before the task is processed , that transaction's destructor is invoked. The destructor assumes that if cache_pending is true then the transaction must be present in the entry. https://codereview.chromium.org/2774603003/diff/410001/net/http/http_cache_tr... File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/2774603003/diff/410001/net/http/http_cache_tr... net/http/http_cache_transaction.h:501: // Headers have been received from the network and it's not a match with the On 2017/06/09 at 17:04:50, jkarlin wrote: > newline above this comment done https://codereview.chromium.org/2774603003/diff/410001/net/http/http_cache_tr... net/http/http_cache_transaction.h:503: bool done_headers_create_new_entry_; On 2017/06/09 at 17:04:49, jkarlin wrote: > newline below the member done
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #11 (id:430001) has been deleted
The CQ bit was checked by shivanisha@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.
lgtm with some comments. Mostly comments on comment clarity. One new test that I'd like to see. https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache.cc... net/http/http_cache.cc:952: Transaction* transaction) { transaction is unused except for a dcheck, let's remove it. https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache.cc... net/http/http_cache.cc:964: // Restart only add_to_entry_queue transactions. Seems like this comment should be the first line of the large comment below. https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache.cc... net/http/http_cache.cc:969: // cache pending state so that in case their destructor is invoked, it's ok s/their/its/ https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache.cc... net/http/http_cache.cc:995: // Writer failed to completely write the response to The writer failed ... https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1122: // Switching into passing through data directly from the network, avoiding Suggest: Set the mode to NONE in order to bypass the cache entry and read from the network directly. https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1133: // be sent. Note that since mode_ is set to pass-through, response will Suggest: Note that since mode_ is NONE, the response won't be written to cache. Transition to STATE_CACHE_WRITE_RESPONSE as that's the state the transaction left off on when it tried to create the new entry. https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1154: // to any cache lock delays, thus returning early from here. This is a very long sentence, please break it up. https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1265: // to a no-match with original entry which was doomed and |new_entry_| was Suggest: This transaction's response headers did not match its ActiveEntry so it created a new ActiveEntry (new_entry_) to write to (and doomed the old one). Now that the new entry has been created, start writing the response. https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1278: bool is_match = response_.headers->response_code() == 304; Hmm, can it be a 304? If it were a 304 then the transaction wouldn't have overwritten the old response right? Can we DCHECK_NE? https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1782: // WRITE will take care of dooming if any other entry exists. Suggest: The transaction needs to overwrite this response. Doom the current entry, create a new one (by going to STATE_INIT_ENTRY), and then jump straight to writing out the response, bypassing the headers checks. The mode_ is set to WRITE in order to doom any other existing entries that might exist so that this transaction can go straight to writing a response. https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache_tr... File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache_tr... net/http/http_cache_transaction.h:176: // added to an entry and the entry. s/removes/remove/ https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache_un... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache_un... net/http/http_cache_unittest.cc:9072: Please add a test in which there are 5 transactions, the first writes, the second doesn't match, and the remaining 3 match the second. So we can see that after the 2nd transaction creates a new entry, that the following three use the new entry and not the old one.
The CQ bit was checked by shivanisha@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...
Thanks Josh, Randy! https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache.cc... net/http/http_cache.cc:952: Transaction* transaction) { On 2017/06/16 at 18:28:01, jkarlin_slow wrote: > transaction is unused except for a dcheck, let's remove it. done https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache.cc... net/http/http_cache.cc:964: // Restart only add_to_entry_queue transactions. On 2017/06/16 at 18:28:01, jkarlin_slow wrote: > Seems like this comment should be the first line of the large comment below. done https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache.cc... net/http/http_cache.cc:969: // cache pending state so that in case their destructor is invoked, it's ok On 2017/06/16 at 18:28:01, jkarlin_slow wrote: > s/their/its/ done https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache.cc... net/http/http_cache.cc:995: // Writer failed to completely write the response to On 2017/06/16 at 18:28:01, jkarlin_slow wrote: > The writer failed ... done https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1122: // Switching into passing through data directly from the network, avoiding On 2017/06/16 at 18:28:01, jkarlin_slow wrote: > Suggest: Set the mode to NONE in order to bypass the cache entry and read from the network directly. done https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1133: // be sent. Note that since mode_ is set to pass-through, response will On 2017/06/16 at 18:28:01, jkarlin_slow wrote: > Suggest: Note that since mode_ is NONE, the response won't be written to cache. Transition to STATE_CACHE_WRITE_RESPONSE as that's the state the transaction left off on when it tried to create the new entry. done https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1154: // to any cache lock delays, thus returning early from here. On 2017/06/16 at 18:28:01, jkarlin_slow wrote: > This is a very long sentence, please break it up. done https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1265: // to a no-match with original entry which was doomed and |new_entry_| was On 2017/06/16 at 18:28:01, jkarlin_slow wrote: > Suggest: This transaction's response headers did not match its ActiveEntry so it created a new ActiveEntry (new_entry_) to write to (and doomed the old one). Now that the new entry has been created, start writing the response. done https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1278: bool is_match = response_.headers->response_code() == 304; On 2017/06/16 at 18:28:01, jkarlin_slow wrote: > Hmm, can it be a 304? If it were a 304 then the transaction wouldn't have overwritten the old response right? Can we DCHECK_NE? done https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1782: // WRITE will take care of dooming if any other entry exists. On 2017/06/16 at 18:28:01, jkarlin_slow wrote: > Suggest: > > The transaction needs to overwrite this response. Doom the current entry, create a new one (by going to STATE_INIT_ENTRY), and then jump straight to writing out the response, bypassing the headers checks. The mode_ is set to WRITE in order to doom any other existing entries that might exist so that this transaction can go straight to writing a response. done https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache_tr... File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache_tr... net/http/http_cache_transaction.h:176: // added to an entry and the entry. On 2017/06/16 at 18:28:01, jkarlin_slow wrote: > s/removes/remove/ done https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache_un... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/2774603003/diff/450001/net/http/http_cache_un... net/http/http_cache_unittest.cc:9072: On 2017/06/16 at 18:28:01, jkarlin_slow wrote: > Please add a test in which there are 5 transactions, the first writes, the second doesn't match, and the remaining 3 match the second. So we can see that after the 2nd transaction creates a new entry, that the following three use the new entry and not the old one. test added.
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 shivanisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org, jkarlin@chromium.org Link to the patchset: https://codereview.chromium.org/2774603003/#ps510001 (title: "Rebased with refs/heads/master@{#484325}")
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
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by shivanisha@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": 510001, "attempt_start_ts": 1499293261707810,
"parent_rev": "c76fe2d15d81e2e125ad5efc57a70ab143870a6e", "commit_rev":
"36a816e7d1de8c3e1a2a720b05743ceb8405653d"}
Message was sent while issue was closed.
Description was changed from ========== If parallel validation leads to a no match then in addition to dooming this entry, also create a new entry. BUG=472740 ========== to ========== If parallel validation leads to a no match then in addition to dooming this entry, also create a new entry. BUG=472740 Review-Url: https://codereview.chromium.org/2774603003 Cr-Commit-Position: refs/heads/master@{#484365} Committed: https://chromium.googlesource.com/chromium/src/+/36a816e7d1de8c3e1a2a720b0574... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:510001) as https://chromium.googlesource.com/chromium/src/+/36a816e7d1de8c3e1a2a720b0574... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
