Bounce all requests off the cache lock after 25ms.

Bounce all requests off the cache lock after 25ms.

The cache lock keeps causing us problems and the existing 20 second timeout is
too long to be useful, so we have the worst of both worlds. However, UMA
reports that we only ever go beyond 25ms 0.89% of the time over the past 28
days. (See HttpCache.EntryLockWait.)

Extend the range request 25ms hack to all requests. This should more usefully
mitigate the various deadlocks and priority inversions and worse (fetch() means
mutually-distrusting origins can lock each other out of a resource) until a
better design is prioritized.

BUG=472740, 6697, 458620, 535793
TEST=load https://expired.badssl.com in two tabs. Both should load.

[davidben, 2015-11-25 20:49:35]

Description was changed from

==========
Bounce all requests off the cache lock after 250ms.

The cache lock keeps causing us problems and the existing 20 second timeout is
too long to be useful, so we have the worst of both worlds. However, UMA
reports that we only ever go beyond 250ms 0.42% of the time.

Extend the range request 250ms hack to all requests. This should more usefully
mitigate the various deadlocks and priority inversions and worse (fetch() means
mutually-distrusting origins can lock each other out of a resource) until a
better design is prioritized.

BUG=472740,6697,458620,535793
TEST=load https://expired.badssl.com in two tabs. Both should load.
==========

to

==========
Bounce all requests off the cache lock after 250ms.

The cache lock keeps causing us problems and the existing 20 second timeout is
too long to be useful, so we have the worst of both worlds. However, UMA
reports that we only ever go beyond 250ms 0.42% of the time over the past 28
days. (See HttpCache.EntryLockWait.)

Extend the range request 250ms hack to all requests. This should more usefully
mitigate the various deadlocks and priority inversions and worse (fetch() means
mutually-distrusting origins can lock each other out of a resource) until a
better design is prioritized.

BUG=472740,6697,458620,535793
TEST=load https://expired.badssl.com in two tabs. Both should load.
==========

[davidben, 2015-11-25 20:53:45]

Description was changed from

==========
Bounce all requests off the cache lock after 250ms.

The cache lock keeps causing us problems and the existing 20 second timeout is
too long to be useful, so we have the worst of both worlds. However, UMA
reports that we only ever go beyond 250ms 0.42% of the time over the past 28
days. (See HttpCache.EntryLockWait.)

Extend the range request 250ms hack to all requests. This should more usefully
mitigate the various deadlocks and priority inversions and worse (fetch() means
mutually-distrusting origins can lock each other out of a resource) until a
better design is prioritized.

BUG=472740,6697,458620,535793
TEST=load https://expired.badssl.com in two tabs. Both should load.
==========

to

==========
Bounce all requests off the cache lock after 25ms.

The cache lock keeps causing us problems and the existing 20 second timeout is
too long to be useful, so we have the worst of both worlds. However, UMA
reports that we only ever go beyond 25ms 0.89% of the time over the past 28
days. (See HttpCache.EntryLockWait.)

Extend the range request 25ms hack to all requests. This should more usefully
mitigate the various deadlocks and priority inversions and worse (fetch() means
mutually-distrusting origins can lock each other out of a resource) until a
better design is prioritized.

BUG=472740,6697,458620,535793
TEST=load https://expired.badssl.com in two tabs. Both should load.
==========

[mmenke, 2015-11-25 20:55:38]

mmenke@chromium.org changed reviewers:
+ mmenke@chromium.org

[mmenke, 2015-11-25 20:55:39]

https://codereview.chromium.org/1476123002/diff/1/net/http/http_cache_transac...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/1476123002/diff/1/net/http/http_cache_transac...
net/http/http_cache_transaction.cc:1170: if (bypass_lock_for_test_) {
I think we can get rid of this, and use the real timeout on that test now?  This
also seems a really bad way of simulating timeouts, anyways.

https://codereview.chromium.org/1476123002/diff/1/net/http/http_cache_transac...
net/http/http_cache_transaction.cc:1195: TimeDelta::FromMilliseconds(25));
The CL description and comment above say 250, this is 25.

[davidben, 2015-11-25 21:27:39]

https://codereview.chromium.org/1476123002/diff/1/net/http/http_cache_transac...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/1476123002/diff/1/net/http/http_cache_transac...
net/http/http_cache_transaction.cc:1170: if (bypass_lock_for_test_) {
On 2015/11/25 20:55:39, mmenke wrote:
> I think we can get rid of this, and use the real timeout on that test now? 
This
> also seems a really bad way of simulating timeouts, anyways.

Done.

https://codereview.chromium.org/1476123002/diff/1/net/http/http_cache_transac...
net/http/http_cache_transaction.cc:1195: TimeDelta::FromMilliseconds(25));
On 2015/11/25 20:55:39, mmenke wrote:
> The CL description and comment above say 250, this is 25.

Fixed to 25 since that's what range requests do. Updated description and
comment.

[mmenke, 2015-11-25 22:09:26]

LGTM, but should also be reviewed by someone more familiar with the cache code.

[mmenke, 2016-05-17 18:49:41]

Removing myself as reviewer from this one...  Too many pending reviews

[mmenke, 2016-05-17 18:49:46]

mmenke@chromium.org changed reviewers:
- mmenke@chromium.org

[davidben, 2016-05-17 18:59:54]

Closing this issue anyway. This is going to need something cleverer to avoid the
time travel issue Ricardo mentioned.