Adds cache lock timeout handling after finishing headers phase.

Adds cache lock timeout handling after finishing headers phase.

Since now there is parallel validation of requests but the cache lock
still applies when the writer transaction is reading the response body
we need to have a lock timeout handling when a transaction is waiting
in the done_headers_queue as well.

This Cl also removes the unused RestartInfo structure definition.

BUG=735998
Review-Url: https://codereview.chromium.org/2953983003
Cr-Commit-Position: refs/heads/master@{#482148}
Committed: https://chromium.googlesource.com/chromium/src/+/bc3b71b348b6073d188bb815443190ec73bf84a3

[shivanisha, 2017-06-23 01:28:05]

The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-23 01:28:28]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shivanisha, 2017-06-23 01:34:09]

shivanisha@chromium.org changed reviewers:
+ rdsmith@chromium.org

[shivanisha, 2017-06-23 01:34:09]

Randy, PTAL (P1 fix), Thanks!

[commit-bot: I haz the power, 2017-06-23 02:53:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-23 02:53:26]

Dry run: This issue passed the CQ dry run.

[Randy Smith (Not in Mondays), 2017-06-23 17:38:02]

This basic approach looks fine to me.  The issues I care about below for this CL
are the entry_lock_waiting_since_ issue and the mode_ = NONE question (the
latter on general principles: If I don't understand it, I consider it dangerous
:-}).  But I may want to push for changes to code structure in a followup CL.

https://codereview.chromium.org/2953983003/diff/1/net/http/http_cache.h
File net/http/http_cache.h (right):

https://codereview.chromium.org/2953983003/diff/1/net/http/http_cache.h#newco...
net/http/http_cache.h:201: void SimulateCacheLockTimeoutAfterHeaders() {
nit: Can this method have a ForTesting suffix?  I think there are some automated
tools that make sure such methods are only called from test code.

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

https://codereview.chromium.org/2953983003/diff/1/net/http/http_cache_transac...
net/http/http_cache_transaction.cc:929: mode_ = NONE;
Was there a specific failure that this was addressing, or is this just trying to
make the function more clearly and reliably execute its intent?

https://codereview.chromium.org/2953983003/diff/1/net/http/http_cache_transac...
net/http/http_cache_transaction.cc:1145: if (bypass_lock_for_test_ ||
bypass_lock_after_headers_for_test_) {
How is the effect of these two booleans different?  If it isn't different, why
can't they be combined?

https://codereview.chromium.org/2953983003/diff/1/net/http/http_cache_transac...
net/http/http_cache_transaction.cc:2823: if (entry_lock_waiting_since_ !=
start_time)
AddToEntry->AddToEntryComplete sets and then resets entry_lock_waiting_since_,
allowing this conditional to be a good test to make sure the delayed task has
fired during the async operation we're trying to time out.  Doesn't the same
structure need to be in place across DoFinishHeaders/DoFinishHeadersComplete?

[shivanisha, 2017-06-23 19:16:08]

Feedback addressed, PTAL, Thanks!

https://codereview.chromium.org/2953983003/diff/1/net/http/http_cache.h
File net/http/http_cache.h (right):

https://codereview.chromium.org/2953983003/diff/1/net/http/http_cache.h#newco...
net/http/http_cache.h:201: void SimulateCacheLockTimeoutAfterHeaders() {
On 2017/06/23 at 17:38:01, Randy Smith (Not in Mondays) wrote:
> nit: Can this method have a ForTesting suffix?  I think there are some
automated tools that make sure such methods are only called from test code.

Done for both this and the existing SimulateCacheLockTimeout method

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

https://codereview.chromium.org/2953983003/diff/1/net/http/http_cache_transac...
net/http/http_cache_transaction.cc:929: mode_ = NONE;
On 2017/06/23 at 17:38:01, Randy Smith (Not in Mondays) wrote:
> Was there a specific failure that this was addressing, or is this just trying
to make the function more clearly and reliably execute its intent?

Since we now have another flow that comes to this state and in this flow its
possible that mode_ was set to something and this function assumes that it is
set to NONE (There is no default path setting it to NONE below when
ShouldPassThrough() returns true.)

https://codereview.chromium.org/2953983003/diff/1/net/http/http_cache_transac...
net/http/http_cache_transaction.cc:1145: if (bypass_lock_for_test_ ||
bypass_lock_after_headers_for_test_) {
On 2017/06/23 at 17:38:01, Randy Smith (Not in Mondays) wrote:
> How is the effect of these two booleans different?  If it isn't different, why
can't they be combined?

bypass_lock_for_test_ calls the timeout handler when the transaction is in
add_to_entry_queue and bypass_lock_after_headers_for_test_ calls the timeout
handler when the transaction is in done_headers_queue. A test that needs to test
the latter should not use the time out handler when the transaction is in
add_to_entry_queue.

https://codereview.chromium.org/2953983003/diff/1/net/http/http_cache_transac...
net/http/http_cache_transaction.cc:2823: if (entry_lock_waiting_since_ !=
start_time)
On 2017/06/23 at 17:38:02, Randy Smith (Not in Mondays) wrote:
> AddToEntry->AddToEntryComplete sets and then resets entry_lock_waiting_since_,
allowing this conditional to be a good test to make sure the delayed task has
fired during the async operation we're trying to time out.  Doesn't the same
structure need to be in place across DoFinishHeaders/DoFinishHeadersComplete?

Good idea, done

[shivanisha, 2017-06-23 20:01:36]

The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-23 20:02:01]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shivanisha, 2017-06-23 20:03:07]

This patch fixes the condition for bypassing timer for testing and renames
AddTimeoutHandler to AddCacheLockTimeoutHandler. 

PTAL, Thanks!

[Randy Smith (Not in Mondays), 2017-06-23 20:20:47]

LGTM.  Thanks!

https://codereview.chromium.org/2953983003/diff/40001/net/http/http_cache_tra...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/2953983003/diff/40001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:1145: if ((bypass_lock_for_test_ &&
next_state_ == STATE_ADD_TO_ENTRY_COMPLETE) ||
suggestion: Add DCHECK that we're in one of those two states.

https://codereview.chromium.org/2953983003/diff/40001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:1145: if ((bypass_lock_for_test_ &&
next_state_ == STATE_ADD_TO_ENTRY_COMPLETE) ||
suggestion, no need for it to be in this CL if you like it: Change the variables
to be a single "bypass_lock_for_state_" which would be set to a particular state
and then just checked against the current state here.  The positive side of this
is that it's less test code in the production binary, the negative is that it
requires any test that uses it to be a friend (so that it has visibility into
HC::T state names).

https://codereview.chromium.org/2953983003/diff/40001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:1874: AddCacheLockTimeoutHandler(entry_);
nit, suggestion, doesn't need to be in this CL if you like it: Put the
assignment to entry_lock_waiting_since_ inside of AddCacheLockTimeoutHandler()
and have a matching function (DisableCacheLockTimeoutHandler()) which resets it.

[shivanisha, 2017-06-23 20:25:53]

The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-23 20:26:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shivanisha, 2017-06-23 20:28:00]

On 2017/06/23 at 20:20:47, rdsmith wrote:
> LGTM.  Thanks!
> 
>
https://codereview.chromium.org/2953983003/diff/40001/net/http/http_cache_tra...
> File net/http/http_cache_transaction.cc (right):
> 
>
https://codereview.chromium.org/2953983003/diff/40001/net/http/http_cache_tra...
> net/http/http_cache_transaction.cc:1145: if ((bypass_lock_for_test_ &&
next_state_ == STATE_ADD_TO_ENTRY_COMPLETE) ||
> suggestion: Add DCHECK that we're in one of those two states.
> 
>
https://codereview.chromium.org/2953983003/diff/40001/net/http/http_cache_tra...
> net/http/http_cache_transaction.cc:1145: if ((bypass_lock_for_test_ &&
next_state_ == STATE_ADD_TO_ENTRY_COMPLETE) ||
> suggestion, no need for it to be in this CL if you like it: Change the
variables to be a single "bypass_lock_for_state_" which would be set to a
particular state and then just checked against the current state here.  The
positive side of this is that it's less test code in the production binary, the
negative is that it requires any test that uses it to be a friend (so that it
has visibility into HC::T state names).
> 
>
https://codereview.chromium.org/2953983003/diff/40001/net/http/http_cache_tra...
> net/http/http_cache_transaction.cc:1874: AddCacheLockTimeoutHandler(entry_);
> nit, suggestion, doesn't need to be in this CL if you like it: Put the
assignment to entry_lock_waiting_since_ inside of AddCacheLockTimeoutHandler()
and have a matching function (DisableCacheLockTimeoutHandler()) which resets it.

Thanks Randy, added the dcheck and will follow up with the rest of the comments
(if any changes, I'll do it in a separate CL).

[commit-bot: I haz the power, 2017-06-23 22:49:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-23 22:49:10]

Dry run: This issue passed the CQ dry run.

[shivanisha, 2017-06-24 06:05:27]

The CQ bit was checked by shivanisha@chromium.org

[shivanisha, 2017-06-24 06:05:27]

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/2953983003/#ps60001
(title: "dcheck added")

[commit-bot: I haz the power, 2017-06-24 06:05:39]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-24 07:21:16]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498284327166530,
"parent_rev": "dacfd003d4b463152a155266d17c56504cfc6f12", "commit_rev":
"bc3b71b348b6073d188bb815443190ec73bf84a3"}

[commit-bot: I haz the power, 2017-06-24 07:21:27]

Description was changed from

==========
Adds cache lock timeout handling after finishing headers phase.

Since now there is parallel validation of requests but the cache lock
still applies when the writer transaction is reading the response body
we need to have a lock timeout handling when a transaction is waiting
in the done_headers_queue as well.

This Cl also removes the unused RestartInfo structure definition.

BUG=735998
==========

to

==========
Adds cache lock timeout handling after finishing headers phase.

Since now there is parallel validation of requests but the cache lock
still applies when the writer transaction is reading the response body
we need to have a lock timeout handling when a transaction is waiting
in the done_headers_queue as well.

This Cl also removes the unused RestartInfo structure definition.

BUG=735998

Review-Url: https://codereview.chromium.org/2953983003
Cr-Commit-Position: refs/heads/master@{#482148}
Committed:
https://chromium.googlesource.com/chromium/src/+/bc3b71b348b6073d188bb8154431...
==========

[commit-bot: I haz the power, 2017-06-24 07:21:28]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/bc3b71b348b6073d188bb8154431...