Resume the main job immediately when QUIC job fails, not wait for the head start timer to exprie

Resume the main job immediately when QUIC job fails, not wait for the head start timer to exprie

BUG=691688
Review-Url: https://codereview.chromium.org/2693043008
Cr-Commit-Position: refs/heads/master@{#450705}
Committed: https://chromium.googlesource.com/chromium/src/+/ff2bfbfd76f3c43e5618addd34c82fed42d2a6f5

[Zhongyi Shi, 2017-02-14 23:12:26]

Patchset #1 (id:1) has been deleted

[Zhongyi Shi, 2017-02-14 23:13:31]

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

[commit-bot: I haz the power, 2017-02-14 23:14:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Zhongyi Shi, 2017-02-14 23:16:45]

zhongyi@chromium.org changed reviewers:
+ rch@chromium.org

[Zhongyi Shi, 2017-02-14 23:16:46]

Ryan, PTAL! 

Let me know whether adding the boolean to indicate whether the
main_job_->Resume() is executed works.
In theory, we could simply use the main_job_->is_waiting() to replace the
main_job_is_resumed_. However, in tests, the main_job are mocked, and
main_job_->Resume() is mocked out, and main_job will always wait :(

[Ryan Hamilton, 2017-02-15 00:07:02]

In addition to the offline conversation, one suggestion.

https://codereview.chromium.org/2693043008/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2693043008/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.cc:581: DCHECK(delay ==
base::TimeDelta());
I don't quite understand this block. I would think that we would unconditionally
do:

main_job_wait_time_ = delay;

https://codereview.chromium.org/2693043008/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job_controller.h (right):

https://codereview.chromium.org/2693043008/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.h:309: bool
main_job_is_resumed_;
nit: newline after 

(or no newline before, but might as well add it)

[Zhongyi Shi, 2017-02-15 00:26:58]

https://codereview.chromium.org/2693043008/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2693043008/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.cc:581: DCHECK(delay ==
base::TimeDelta());
On 2017/02/15 00:07:02, Ryan Hamilton wrote:
> I don't quite understand this block. I would think that we would
unconditionally
> do:
> 
> main_job_wait_time_ = delay;

Done.

https://codereview.chromium.org/2693043008/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job_controller.h (right):

https://codereview.chromium.org/2693043008/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.h:309: bool
main_job_is_resumed_;
On 2017/02/15 00:07:02, Ryan Hamilton wrote:
> nit: newline after 
> 
> (or no newline before, but might as well add it)

Done.

https://codereview.chromium.org/2693043008/diff/40001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2693043008/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.cc:578: //   2) The main job
has passed waiting state, no-op.
main_job_.is_waiting() will be false
1. main job hasn't started yet, not in WAIT state, just unblock it, main job
then will start to proceed without blocking.
2. main job has passed WAIT state, which is resumed.

[Ryan Hamilton, 2017-02-15 00:52:09]

Thanks!

https://codereview.chromium.org/2693043008/diff/40001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2693043008/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.cc:573: main_job_is_blocked_ =
false;
Might as well just set main_job_is_blocked_ = false, without the guard.

https://codereview.chromium.org/2693043008/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.cc:577: //   1) The main job
hasn't got to waiting state, simply mark it unblocked.
If it's not blocked, but main_job_wait_time_ is not zero, then the job will wait
for that time once it calls ShouldWait(), right?

In any case, the body of this block does not "mark it unblocked" so I'd be
inclined to say something like " so do not yet post a task to resume since that
will happen in ShouldWait()" or some such.

https://codereview.chromium.org/2693043008/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.cc:578: //   2) The main job
has passed waiting state, no-op.
Instead of "no-op" I would say something like, "so the job does not need to be
resumed"

https://codereview.chromium.org/2693043008/diff/40001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job_controller_unittest.cc (right):

https://codereview.chromium.org/2693043008/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller_unittest.cc:946: }
Does this old test fail with the new code (And so you've needed to change the
test) or are these change to make the test better by testing more conditions?

https://codereview.chromium.org/2693043008/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller_unittest.cc:949:
ResumeMainJobImmediatelyOnStreamFailed) {
I assume this fails with the old code?

https://codereview.chromium.org/2693043008/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller_unittest.cc:990:
base::TimeDelta::FromMicroseconds(0))));
How does this verify that the main job is started? (Does it check
main_job_wait_time_ or actually that the main job resumes?)

[Zhongyi Shi, 2017-02-15 01:38:06]

Patchset #3 (id:60001) has been deleted

[Zhongyi Shi, 2017-02-15 01:39:10]

ptal, thanks!

https://codereview.chromium.org/2693043008/diff/40001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2693043008/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.cc:573: main_job_is_blocked_ =
false;
On 2017/02/15 00:52:08, Ryan Hamilton wrote:
> Might as well just set main_job_is_blocked_ = false, without the guard.

Done.

https://codereview.chromium.org/2693043008/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.cc:577: //   1) The main job
hasn't got to waiting state, simply mark it unblocked.
On 2017/02/15 00:52:08, Ryan Hamilton wrote:
> If it's not blocked, but main_job_wait_time_ is not zero, then the job will
wait
> for that time once it calls ShouldWait(), right?
Yup.
> In any case, the body of this block does not "mark it unblocked" so I'd be
> inclined to say something like " so do not yet post a task to resume since
that
> will happen in ShouldWait()" or some such.
Thanks for helping clarify the case.

https://codereview.chromium.org/2693043008/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.cc:578: //   2) The main job
has passed waiting state, no-op.
On 2017/02/15 00:52:08, Ryan Hamilton wrote:
> Instead of "no-op" I would say something like, "so the job does not need to be
> resumed"

Done.

https://codereview.chromium.org/2693043008/diff/40001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job_controller_unittest.cc (right):

https://codereview.chromium.org/2693043008/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller_unittest.cc:946: }
On 2017/02/15 00:52:08, Ryan Hamilton wrote:
> Does this old test fail with the new code (And so you've needed to change the
> test) or are these change to make the test better by testing more conditions?

The old test won't fail with the new code. I just added more testing to this
one, so in this test we'll have an extra call to OnStreamFailed when the main
job is actually resumed(Resume is verified to be invoked on line 923) should not
resume the job again(expectation on line 942).

https://codereview.chromium.org/2693043008/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller_unittest.cc:949:
ResumeMainJobImmediatelyOnStreamFailed) {
On 2017/02/15 00:52:08, Ryan Hamilton wrote:
> I assume this fails with the old code?

It'll, this one now expects the main job to be resumed with main_job_wait_time =
0.

https://codereview.chromium.org/2693043008/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller_unittest.cc:990:
base::TimeDelta::FromMicroseconds(0))));
On 2017/02/15 00:52:08, Ryan Hamilton wrote:
> How does this verify that the main job is started? (Does it check
> main_job_wait_time_ or actually that the main job resumes?)

This method check Resume() is called on main_job. I have added expectations on
main job to check is_waiting() is true so that we could verify the main job
starts.

[Ryan Hamilton, 2017-02-15 01:48:23]

lgtm

[Zhongyi Shi, 2017-02-15 01:57:08]

I'll need to wait the NetLog cl landed and rebase this one before landing.

[Zhongyi Shi, 2017-02-15 13:49:52]

The CQ bit was checked by zhongyi@chromium.org

[Zhongyi Shi, 2017-02-15 13:49:53]

The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org
Link to the patchset: https://codereview.chromium.org/2693043008/#ps100001
(title: "sync with master")

[commit-bot: I haz the power, 2017-02-15 13:50:20]

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

[Ryan Hamilton, 2017-02-15 14:53:55]

https://codereview.chromium.org/2693043008/diff/100001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2693043008/diff/100001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller.cc:585: main_job_wait_time_ =
delay;
Does this need to happen before the early return on line 582 to handle the case
where |delay| is zero, and the main job has not yet made it do wait?

[commit-bot: I haz the power, 2017-02-15 15:23:09]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487166592789920,
"parent_rev": "a87a279a7fd622ca4130806c22e6cf2c9b3bc32e", "commit_rev":
"ff2bfbfd76f3c43e5618addd34c82fed42d2a6f5"}

[commit-bot: I haz the power, 2017-02-15 15:23:46]

Description was changed from

==========
Resume the main job immediately when QUIC job fails, not wait for the head start
timer to exprie

BUG=691688
==========

to

==========
Resume the main job immediately when QUIC job fails, not wait for the head start
timer to exprie

BUG=691688

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

[commit-bot: I haz the power, 2017-02-15 15:23:47]

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