Move the logic to cancel main job in OrphanUnboundJob when the alt job succeed while main job is bl…

Move the logic to cancel main job in OrphanUnboundJob when the alt job succeed while main job is blocked.

When the alt job succeeds while the main job hasn't, the alt job will be bound the request by HttpStreamFactoryImpl::JobController::BindJob(). 
And JobController will orphan the main job. If the main job 
- is still blocked(i.e, not started yet), it should be cancelled immediately.
- is no longer blocked
  - but is for websocket, it should be cancelled immediately.
  - and is not for websocket, no-op. The job will continue connecting until it finishes. When it's finishing, it will call one of the callbacks and 
notifying the JobController of the Job's completion. The JobController will then check whether this job has been orphaned, and calls 
OnOrphanedJobComplete to delete the main job.

BUG=678768
Review-Url: https://codereview.chromium.org/2625133006
Cr-Commit-Position: refs/heads/master@{#444090}
Committed: https://chromium.googlesource.com/chromium/src/+/910b9b9050f70a0a8602a8cfcb0ac45b9bcdd723

[Zhongyi Shi, 2017-01-12 22:39:37]

Description was changed from

==========
Move the logic to cancel main job in OrphanUnboundJob when the alt job succeed
while main job is blocked.

BUG=678768
==========

to

==========
Move the logic to cancel main job in OrphanUnboundJob when the alt job succeed
while main job is blocked.

BUG=678768
==========

[Zhongyi Shi, 2017-01-12 22:39:37]

zhongyi@chromium.org changed reviewers:
+ mmenke@chromium.org, xunjieli@chromium.org

[Zhongyi Shi, 2017-01-12 22:41:23]

Helen and Matt,

I moved the logic up to OrphanUnboundJob so that we delete the main job more
timely rather than delay until the request completes. PTAL!

[mmenke, 2017-01-12 22:46:48]

https://codereview.chromium.org/2625133006/diff/1/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2625133006/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job_controller.cc:744: if (main_job_ &&
(main_job_is_blocked_ || !main_job_wait_time_.is_zero())) {
DCHECK(alternative_job_)?  (If alternate_job_ is removed before this call, we'd
need a MaybeNotifyFactoryOfCompletion(); call after destroying the job)

[xunjieli, 2017-01-12 23:00:33]

https://codereview.chromium.org/2625133006/diff/1/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2625133006/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job_controller.cc:747: main_job_->Orphan();
Should HttpStreamFactoryImpl::Job::Orphan() also call OnOrphanedJobComplete() in
the non-websockets case?
If this |main_job_| does complete, how will it be removed if not by
MaybeNotifyFactoryOfCompletion through OnOrphanedJobComplete()?

[xunjieli, 2017-01-12 23:04:30]

https://codereview.chromium.org/2625133006/diff/1/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2625133006/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job_controller.cc:747: main_job_->Orphan();
On 2017/01/12 23:00:32, xunjieli wrote:
> Should HttpStreamFactoryImpl::Job::Orphan() also call OnOrphanedJobComplete()
in
> the non-websockets case?
> If this |main_job_| does complete, how will it be removed if not by
> MaybeNotifyFactoryOfCompletion through OnOrphanedJobComplete()?

Think that the old code calls OnOrphanedJobComplete() here directly.

[Zhongyi Shi, 2017-01-12 23:17:50]

https://codereview.chromium.org/2625133006/diff/1/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2625133006/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job_controller.cc:744: if (main_job_ &&
(main_job_is_blocked_ || !main_job_wait_time_.is_zero())) {
On 2017/01/12 22:46:48, mmenke wrote:
> DCHECK(alternative_job_)?  (If alternate_job_ is removed before this call,
we'd
> need a MaybeNotifyFactoryOfCompletion(); call after destroying the job)

Done.
bound_job_ will only be reset to nullptr when the Job it's referencing is
deleted. Thus it should be guaranteed that the alternative_job_ is not removed.
But DCHECK here is a good idea!

https://codereview.chromium.org/2625133006/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job_controller.cc:747: main_job_->Orphan();
On 2017/01/12 23:04:30, xunjieli wrote:
> On 2017/01/12 23:00:32, xunjieli wrote:
> > Should HttpStreamFactoryImpl::Job::Orphan() also call
OnOrphanedJobComplete()
> in
> > the non-websockets case?
> > If this |main_job_| does complete, how will it be removed if not by
> > MaybeNotifyFactoryOfCompletion through OnOrphanedJobComplete()?

For non-websockets case, the Job will continue connecting until it finishes.
When it's finishing, it will call one of the callbacks and notifying the
JobController of the Job's completion. The JobController will then check whether
this job has been orphaned, and calls OnOrphanedJobComplete.

https://cs.chromium.org/chromium/src/net/http/http_stream_factory_impl_job_co...

> 
> Think that the old code calls OnOrphanedJobComplete() here directly.
Nope. The old code only calls OnOrphanedJobComplete if there's a blocking job
(which means the main job is blocked) or the main job is for websocket. That is,
if the main job is not blocked, and the main job is for non-websocket, it won't
call OnOrphanedJobComplte().
https://codereview.chromium.org/1952423002/diff/460001/net/http/http_stream_f...

[xunjieli, 2017-01-13 15:29:59]

lgtm. It will be nice if the CL description can mention why the logic is moved
:)

[Zhongyi Shi, 2017-01-13 19:57:49]

Description was changed from

==========
Move the logic to cancel main job in OrphanUnboundJob when the alt job succeed
while main job is blocked.

BUG=678768
==========

to

==========
Move the logic to cancel main job in OrphanUnboundJob when the alt job succeed
while main job is blocked.

When the alt job succeeds while the main job hasn't, the alt job will be bound
the request by HttpStreamFactoryImpl::JobController::BindJob(). 
- The main job if is still blocked(i.e, not started yet) should be cancelled
immediately.
- The main job if is no longer blocked
  - but is for websocket, should be cancelled immediately.
  - and is not for websocket, the job will continue connecting until it
finishes. When it's finishing, it will call one of the callbacks and notifying
the JobController of the Job's completion. The JobController will then check
whether this job has been orphaned, and calls OnOrphanedJobComplete to delete
the main job.

BUG=678768
==========

[Zhongyi Shi, 2017-01-13 19:58:12]

Description was changed from

==========
Move the logic to cancel main job in OrphanUnboundJob when the alt job succeed
while main job is blocked.

When the alt job succeeds while the main job hasn't, the alt job will be bound
the request by HttpStreamFactoryImpl::JobController::BindJob(). 
- The main job if is still blocked(i.e, not started yet) should be cancelled
immediately.
- The main job if is no longer blocked
  - but is for websocket, should be cancelled immediately.
  - and is not for websocket, the job will continue connecting until it
finishes. When it's finishing, it will call one of the callbacks and notifying
the JobController of the Job's completion. The JobController will then check
whether this job has been orphaned, and calls OnOrphanedJobComplete to delete
the main job.

BUG=678768
==========

to

==========
Move the logic to cancel main job in OrphanUnboundJob when the alt job succeed
while main job is blocked.

When the alt job succeeds while the main job hasn't, the alt job will be bound
the request by HttpStreamFactoryImpl::JobController::BindJob(). 
- The main job if is still blocked(i.e, not started yet) should be cancelled
immediately.
- The main job if is no longer blocked
  - but is for websocket, should be cancelled immediately.
  - and is not for websocket, the job will continue connecting until it
finishes. When it's finishing, it will call one of the callbacks and notifying
the JobController of the Job's completion. 
The JobController will then check whether this job has been orphaned, and calls
OnOrphanedJobComplete to delete the main job.

BUG=678768
==========

[Zhongyi Shi, 2017-01-13 19:58:42]

Description was changed from

==========
Move the logic to cancel main job in OrphanUnboundJob when the alt job succeed
while main job is blocked.

When the alt job succeeds while the main job hasn't, the alt job will be bound
the request by HttpStreamFactoryImpl::JobController::BindJob(). 
- The main job if is still blocked(i.e, not started yet) should be cancelled
immediately.
- The main job if is no longer blocked
  - but is for websocket, should be cancelled immediately.
  - and is not for websocket, the job will continue connecting until it
finishes. When it's finishing, it will call one of the callbacks and notifying
the JobController of the Job's completion. 
The JobController will then check whether this job has been orphaned, and calls
OnOrphanedJobComplete to delete the main job.

BUG=678768
==========

to

==========
Move the logic to cancel main job in OrphanUnboundJob when the alt job succeed
while main job is blocked.

When the alt job succeeds while the main job hasn't, the alt job will be bound
the request by HttpStreamFactoryImpl::JobController::BindJob(). 
- The main job if is still blocked(i.e, not started yet) should be cancelled
immediately.
- The main job if is no longer blocked
  - but is for websocket, should be cancelled immediately.
  - and is not for websocket, the job will continue connecting until it
finishes. When it's finishing, it will call one of the callbacks and 
notifying the JobController of the Job's completion. The JobController 
will then check whether this job has been orphaned, and calls 
OnOrphanedJobComplete to delete the main job.

BUG=678768
==========

[Zhongyi Shi, 2017-01-13 19:59:01]

Description was changed from

==========
Move the logic to cancel main job in OrphanUnboundJob when the alt job succeed
while main job is blocked.

When the alt job succeeds while the main job hasn't, the alt job will be bound
the request by HttpStreamFactoryImpl::JobController::BindJob(). 
- The main job if is still blocked(i.e, not started yet) should be cancelled
immediately.
- The main job if is no longer blocked
  - but is for websocket, should be cancelled immediately.
  - and is not for websocket, the job will continue connecting until it
finishes. When it's finishing, it will call one of the callbacks and 
notifying the JobController of the Job's completion. The JobController 
will then check whether this job has been orphaned, and calls 
OnOrphanedJobComplete to delete the main job.

BUG=678768
==========

to

==========
Move the logic to cancel main job in OrphanUnboundJob when the alt job succeed
while main job is blocked.

When the alt job succeeds while the main job hasn't, the alt job will be bound
the request by HttpStreamFactoryImpl::JobController::BindJob(). 
- The main job if is still blocked(i.e, not started yet) should be cancelled
immediately.
- The main job if is no longer blocked
  - but is for websocket, should be cancelled immediately.
  - and is not for websocket, the job will continue connecting until it
finishes. When it's finishing, it will call one of the callbacks and 
notifying the JobController of the Job's completion. The JobController will then
check whether this job has been orphaned, and calls 
OnOrphanedJobComplete to delete the main job.

BUG=678768
==========

[Zhongyi Shi, 2017-01-13 20:00:00]

Description was changed from

==========
Move the logic to cancel main job in OrphanUnboundJob when the alt job succeed
while main job is blocked.

When the alt job succeeds while the main job hasn't, the alt job will be bound
the request by HttpStreamFactoryImpl::JobController::BindJob(). 
- The main job if is still blocked(i.e, not started yet) should be cancelled
immediately.
- The main job if is no longer blocked
  - but is for websocket, should be cancelled immediately.
  - and is not for websocket, the job will continue connecting until it
finishes. When it's finishing, it will call one of the callbacks and 
notifying the JobController of the Job's completion. The JobController will then
check whether this job has been orphaned, and calls 
OnOrphanedJobComplete to delete the main job.

BUG=678768
==========

to

==========
Move the logic to cancel main job in OrphanUnboundJob when the alt job succeed
while main job is blocked.

When the alt job succeeds while the main job hasn't, the alt job will be bound
the request by HttpStreamFactoryImpl::JobController::BindJob(). And
JobController will orphan the main job. If the main job 
- is still blocked(i.e, not started yet) should be cancelled immediately.
- is no longer blocked
  - but is for websocket, should be cancelled immediately.
  - and is not for websocket, the job will continue connecting until it
finishes. When it's finishing, it will call one of the callbacks and 
notifying the JobController of the Job's completion. The JobController will then
check whether this job has been orphaned, and calls 
OnOrphanedJobComplete to delete the main job.

BUG=678768
==========

[Zhongyi Shi, 2017-01-13 20:00:44]

Description was changed from

==========
Move the logic to cancel main job in OrphanUnboundJob when the alt job succeed
while main job is blocked.

When the alt job succeeds while the main job hasn't, the alt job will be bound
the request by HttpStreamFactoryImpl::JobController::BindJob(). And
JobController will orphan the main job. If the main job 
- is still blocked(i.e, not started yet) should be cancelled immediately.
- is no longer blocked
  - but is for websocket, should be cancelled immediately.
  - and is not for websocket, the job will continue connecting until it
finishes. When it's finishing, it will call one of the callbacks and 
notifying the JobController of the Job's completion. The JobController will then
check whether this job has been orphaned, and calls 
OnOrphanedJobComplete to delete the main job.

BUG=678768
==========

to

==========
Move the logic to cancel main job in OrphanUnboundJob when the alt job succeed
while main job is blocked.

When the alt job succeeds while the main job hasn't, the alt job will be bound
the request by HttpStreamFactoryImpl::JobController::BindJob(). 
And JobController will orphan the main job. If the main job 
- is still blocked(i.e, not started yet) should be cancelled immediately.
- is no longer blocked
  - but is for websocket, should be cancelled immediately.
  - and is not for websocket, the job will continue connecting until it
finishes. When it's finishing, it will call one of the callbacks and 
notifying the JobController of the Job's completion. The JobController will then
check whether this job has been orphaned, and calls 
OnOrphanedJobComplete to delete the main job.

BUG=678768
==========

[Zhongyi Shi, 2017-01-13 20:01:31]

Description was changed from

==========
Move the logic to cancel main job in OrphanUnboundJob when the alt job succeed
while main job is blocked.

When the alt job succeeds while the main job hasn't, the alt job will be bound
the request by HttpStreamFactoryImpl::JobController::BindJob(). 
And JobController will orphan the main job. If the main job 
- is still blocked(i.e, not started yet) should be cancelled immediately.
- is no longer blocked
  - but is for websocket, should be cancelled immediately.
  - and is not for websocket, the job will continue connecting until it
finishes. When it's finishing, it will call one of the callbacks and 
notifying the JobController of the Job's completion. The JobController will then
check whether this job has been orphaned, and calls 
OnOrphanedJobComplete to delete the main job.

BUG=678768
==========

to

==========
Move the logic to cancel main job in OrphanUnboundJob when the alt job succeed
while main job is blocked.

When the alt job succeeds while the main job hasn't, the alt job will be bound
the request by HttpStreamFactoryImpl::JobController::BindJob(). 
And JobController will orphan the main job. If the main job 
- is still blocked(i.e, not started yet), it should be cancelled immediately.
- is no longer blocked
  - but is for websocket, it should be cancelled immediately.
  - and is not for websocket, no-op. The job will continue connecting until it
finishes. When it's finishing, it will call one of the callbacks and 
notifying the JobController of the Job's completion. The JobController will then
check whether this job has been orphaned, and calls 
OnOrphanedJobComplete to delete the main job.

BUG=678768
==========

[Zhongyi Shi, 2017-01-13 20:03:35]

Thanks for help reviewing. I have just updated the CL description. PTAL!

Matt, mind taking a look again before I check in the code?

[mmenke, 2017-01-13 20:06:57]

On 2017/01/13 20:03:35, Zhongyi Shi wrote:
> Thanks for help reviewing. I have just updated the CL description. PTAL!
> 
> Matt, mind taking a look again before I check in the code?

LGTM!

[Zhongyi Shi, 2017-01-13 20:08:30]

The CQ bit was checked by zhongyi@chromium.org

[commit-bot: I haz the power, 2017-01-13 20:09:10]

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

[xunjieli, 2017-01-13 20:24:26]

For future reference, it is encouraged to have either 80 column width or 72
column width for commit message. It makes the CL description easier to read and
git-friendly
(https://groups.google.com/a/chromium.org/d/msg/chromium-dev/sRRWJwvy2C8/qIWO7...).

[commit-bot: I haz the power, 2017-01-13 20:43:05]

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

[commit-bot: I haz the power, 2017-01-13 20:43:06]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Zhongyi Shi, 2017-01-13 21:29:14]

Patchset #3 (id:40001) has been deleted

[xunjieli, 2017-01-17 14:32:16]

very sorry about the delay! Test LGTM

[Zhongyi Shi, 2017-01-17 17:01:43]

The CQ bit was checked by zhongyi@chromium.org

[Zhongyi Shi, 2017-01-17 17:01:44]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2625133006/#ps60001
(title: "fix test")

[commit-bot: I haz the power, 2017-01-17 17:02:07]

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-01-17 17:55:55]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484672503720740,
"parent_rev": "f66de0c8879b69c467daaf1f5900334de2548c3c", "commit_rev":
"910b9b9050f70a0a8602a8cfcb0ac45b9bcdd723"}

[commit-bot: I haz the power, 2017-01-17 17:56:26]

Description was changed from

==========
Move the logic to cancel main job in OrphanUnboundJob when the alt job succeed
while main job is blocked.

When the alt job succeeds while the main job hasn't, the alt job will be bound
the request by HttpStreamFactoryImpl::JobController::BindJob(). 
And JobController will orphan the main job. If the main job 
- is still blocked(i.e, not started yet), it should be cancelled immediately.
- is no longer blocked
  - but is for websocket, it should be cancelled immediately.
  - and is not for websocket, no-op. The job will continue connecting until it
finishes. When it's finishing, it will call one of the callbacks and 
notifying the JobController of the Job's completion. The JobController will then
check whether this job has been orphaned, and calls 
OnOrphanedJobComplete to delete the main job.

BUG=678768
==========

to

==========
Move the logic to cancel main job in OrphanUnboundJob when the alt job succeed
while main job is blocked.

When the alt job succeeds while the main job hasn't, the alt job will be bound
the request by HttpStreamFactoryImpl::JobController::BindJob(). 
And JobController will orphan the main job. If the main job 
- is still blocked(i.e, not started yet), it should be cancelled immediately.
- is no longer blocked
  - but is for websocket, it should be cancelled immediately.
  - and is not for websocket, no-op. The job will continue connecting until it
finishes. When it's finishing, it will call one of the callbacks and 
notifying the JobController of the Job's completion. The JobController will then
check whether this job has been orphaned, and calls 
OnOrphanedJobComplete to delete the main job.

BUG=678768

Review-Url: https://codereview.chromium.org/2625133006
Cr-Commit-Position: refs/heads/master@{#444090}
Committed:
https://chromium.googlesource.com/chromium/src/+/910b9b9050f70a0a8602a8cfcb0a...
==========

[commit-bot: I haz the power, 2017-01-17 17:56:27]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/910b9b9050f70a0a8602a8cfcb0a...