Chromium Code Reviews| Index: net/http/http_cache_transaction.cc |
| diff --git a/net/http/http_cache_transaction.cc b/net/http/http_cache_transaction.cc |
| index 8c19b9a592bd4fb8d491cfd2e5ea555b218b8082..3cbef7508829fb7a2416f787f2e42838f1b74c8b 100644 |
| --- a/net/http/http_cache_transaction.cc |
| +++ b/net/http/http_cache_transaction.cc |
| @@ -1170,30 +1170,29 @@ int HttpCache::Transaction::DoAddToEntry() { |
| if (bypass_lock_for_test_) { |
|
mmenke
2015/11/25 20:55:39
I think we can get rid of this, and use the real t
davidben
2015/11/25 21:27:38
Done.
|
| OnAddToEntryTimeout(entry_lock_waiting_since_); |
| } else { |
| - int timeout_milliseconds = 20 * 1000; |
| - if (partial_ && new_entry_->writer && |
| - new_entry_->writer->range_requested_) { |
| - // Quickly timeout and bypass the cache if we're a range request and |
| - // we're blocked by the reader/writer lock. Doing so eliminates a long |
| - // running issue, http://crbug.com/31014, where two of the same media |
| - // resources could not be played back simultaneously due to one locking |
| - // the cache entry until the entire video was downloaded. |
| - // |
| - // Bypassing the cache is not ideal, as we are now ignoring the cache |
| - // entirely for all range requests to a resource beyond the first. This |
| - // is however a much more succinct solution than the alternatives, which |
| - // would require somewhat significant changes to the http caching logic. |
| - // |
| - // Allow some timeout slack for the entry addition to complete in case |
| - // the writer lock is imminently released; we want to avoid skipping |
| - // the cache if at all possible. See http://crbug.com/408765 |
| - timeout_milliseconds = 25; |
| - } |
| + // Quickly timeout and bypass the cache if the blocked by the |
| + // reader/writer lock. Metrics indicate that only 0.42% of requests are |
| + // blocked on the cache lock for longer than 250ms. |
| + // |
| + // Bypassing the cache is not ideal, as we are now ignoring the cache |
| + // entirely for requests which hit a resource in |
| + // parallel. https://crbug.com/472740 tracks a better design for the |
| + // system. |
| + // |
| + // In the meantime, having a cache lock causes numerous priority inversion |
| + // problems and deadlocks, so it is better to ensure the system can make |
| + // forward progress. (See https://crbug.com/31014, |
| + // https://crbug.com/458620, https://crbug.com/535793, and |
| + // https://crbug.com/6697.) |
| + // |
| + // Allow some timeout slack for the entry addition to complete in case the |
| + // writer lock is imminently released; we want to avoid skipping the cache |
| + // if at all possible. See http://crbug.com/408765 |
| base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( |
| FROM_HERE, |
| base::Bind(&HttpCache::Transaction::OnAddToEntryTimeout, |
| weak_factory_.GetWeakPtr(), entry_lock_waiting_since_), |
| - TimeDelta::FromMilliseconds(timeout_milliseconds)); |
| + TimeDelta::FromMilliseconds(25)); |
|
mmenke
2015/11/25 20:55:39
The CL description and comment above say 250, this
davidben
2015/11/25 21:27:38
Fixed to 25 since that's what range requests do. U
|
| } |
| } |
| return rv; |