Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
478763004:
Bypass http cache for concurrent range requests. (Closed)
|
Created:
6 years, 4 months ago by DaleCurtis Modified:
6 years, 3 months ago Reviewers:
rileya1, scherkus (not reviewing), rvargas (doing something else), rileya (GONE FROM CHROMIUM) CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionCommitted: https://crrev.com/fa29c3e30198fcdb462ec65f0627a5e5f524effb
Cr-Commit-Position: refs/heads/master@{#292031}
Patch Set 1 #Patch Set 2 : Reflow. #
Total comments: 7
Patch Set 3 : Narrow scope. #
Total comments: 2
Patch Set 4 : Nits. #
Messages
Total messages: 22 (0 generated)
Looks like the required hack is much simpler now that ERR_CACHE_LOCK_TIMEOUT is a viable code path. In general though, this needs the meme dog: _i have no idea what i'm doing_ :o
FYI, this passes several basic tests, including those rileya@ reported issues with previously, as well as a custom test page provided by vine.co
https://codereview.chromium.org/478763004/diff/20001/net/http/http_cache_tran... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/478763004/diff/20001/net/http/http_cache_tran... net/http/http_cache_transaction.cc:1398: base::Bind(&HttpCache::Transaction::OnAddToEntryTimeout, what is the end-user visible effect caused by timing out http cache transactions?
https://codereview.chromium.org/478763004/diff/20001/net/http/http_cache_tran... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/478763004/diff/20001/net/http/http_cache_tran... net/http/http_cache_transaction.cc:1395: const int kTimeoutSeconds = partial_ ? 0 : 20; What happens if this is (partial_ && new_entry_->writer && new_entry_->writer->range_requested_) instead?
https://codereview.chromium.org/478763004/diff/20001/net/http/http_cache_tran... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/478763004/diff/20001/net/http/http_cache_tran... net/http/http_cache_transaction.cc:1398: base::Bind(&HttpCache::Transaction::OnAddToEntryTimeout, On 2014/08/22 21:01:51, scherkus wrote: > what is the end-user visible effect caused by timing out http cache > transactions? The transaction is not timed out, just waiting on the lock. The effect is that the transaction will ignore the cache, so the response will not update the cached entry in any way.... which may lead to consistency issues because a future request may end up returning things "from the past" from the point of view of the renderer.
https://codereview.chromium.org/478763004/diff/20001/net/http/http_cache_tran... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/478763004/diff/20001/net/http/http_cache_tran... net/http/http_cache_transaction.cc:1395: const int kTimeoutSeconds = partial_ ? 0 : 20; On 2014/08/22 21:06:13, rvargas wrote: > What happens if this is (partial_ && new_entry_->writer && > new_entry_->writer->range_requested_) instead? Passes all the test the previous patch did. Do you want me to switch to that? https://codereview.chromium.org/478763004/diff/20001/net/http/http_cache_tran... net/http/http_cache_transaction.cc:1398: base::Bind(&HttpCache::Transaction::OnAddToEntryTimeout, On 2014/08/22 21:01:51, scherkus wrote: > what is the end-user visible effect caused by timing out http cache > transactions? Nothing from a casual user perspective, even for developers nothing obvious happens in the inspector view. To my untrained eye, there's nothing obvious even in net-internals.
https://codereview.chromium.org/478763004/diff/20001/net/http/http_cache_tran... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/478763004/diff/20001/net/http/http_cache_tran... net/http/http_cache_transaction.cc:1395: const int kTimeoutSeconds = partial_ ? 0 : 20; On 2014/08/22 21:25:52, DaleCurtis wrote: > On 2014/08/22 21:06:13, rvargas wrote: > > What happens if this is (partial_ && new_entry_->writer && > > new_entry_->writer->range_requested_) instead? > > Passes all the test the previous patch did. Do you want me to switch to that? If all test pages etc behave the same, yes, I would prefer to make sure that the first request was also for a range.
https://codereview.chromium.org/478763004/diff/20001/net/http/http_cache_tran... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/478763004/diff/20001/net/http/http_cache_tran... net/http/http_cache_transaction.cc:1395: const int kTimeoutSeconds = partial_ ? 0 : 20; On 2014/08/22 21:44:40, rvargas wrote: > On 2014/08/22 21:25:52, DaleCurtis wrote: > > On 2014/08/22 21:06:13, rvargas wrote: > > > What happens if this is (partial_ && new_entry_->writer && > > > new_entry_->writer->range_requested_) instead? > > > > Passes all the test the previous patch did. Do you want me to switch to that? > > If all test pages etc behave the same, yes, I would prefer to make sure that the > first request was also for a range. Done.
lgtm after nit https://codereview.chromium.org/478763004/diff/40001/net/http/http_cache_tran... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/478763004/diff/40001/net/http/http_cache_tran... net/http/http_cache_transaction.cc:1395: const int kTimeoutSeconds = (partial_ && new_entry_->writer && nit: int timeout_seconds = 20; if (partial_ && new_entry_->writer && new_entry_->writer->range_requested_) { // Immediately bypass the cache for range requests. Doing so... timeout_seconds = 0; }
https://codereview.chromium.org/478763004/diff/40001/net/http/http_cache_tran... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/478763004/diff/40001/net/http/http_cache_tran... net/http/http_cache_transaction.cc:1395: const int kTimeoutSeconds = (partial_ && new_entry_->writer && On 2014/08/22 22:19:24, rvargas wrote: > nit: > int timeout_seconds = 20; > if (partial_ && new_entry_->writer && > new_entry_->writer->range_requested_) { > // Immediately bypass the cache for range requests. Doing so... > timeout_seconds = 0; > } Done.
lgtm!
scherkus: Any more comments?
nope! lgtm
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/478763004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to commit the patch.
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/478763004/60001
The CQ bit was unchecked by dalecurtis@chromium.org
Wait... this says it failed to commit, yet committed it as https://chromium.googlesource.com/chromium/src.git/+/fa29c3e30198fcdb462ec65f... O_o Closing and notifying trooper.
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/fa29c3e30198fcdb462ec65f0627a5e5f524effb Cr-Commit-Position: refs/heads/master@{#292031} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
