New issue
Advanced search Search tips

Issue 605398 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocking:
issue 455054



Sign in to add a comment

Add JobController to manage cross-job interactions

Project Member Reported by zhongyi@chromium.org, Apr 21 2016

Issue description

Refactor HttpStreamFactoryImpl and HttpStreamFactoryImpl::Job so that the main and alternate jobs do not know about each other, by adding a new JobController class to manage the cross-job interactions

 
By adding the JobController, we should be able to eliminate the cross-reference between jobs. In addition, HttpStreamFactoryImpl won't need to know jobs for a request, instead it should know the job_controller for a request. Request don't need to know jobs, but only the job_controller. 

JobController, then as the center of management, will know everything, the request it's serving, jobs (both alternative/non-alternative) serving the request and the factoryimpl. 

The lifetime of a job controller is the lifetime of the longer job. 

Comment 2 by b...@chromium.org, Apr 26 2016

Don't know if rch@ shared it with you, but they have this draft from two years ago, it might help: https://crrev.com/265853012.
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 16 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/efbff200c26039dcd1ea0a4d585662629a8b9e69

commit efbff200c26039dcd1ea0a4d585662629a8b9e69
Author: zhongyi <zhongyi@chromium.org>
Date: Thu Jun 16 04:47:04 2016

JobController 1: Remove cross reference between Request, Job, and Impl.

BUG= 605398 

Review-Url: https://codereview.chromium.org/1941083002
Cr-Commit-Position: refs/heads/master@{#400086}

[modify] https://crrev.com/efbff200c26039dcd1ea0a4d585662629a8b9e69/net/BUILD.gn
[modify] https://crrev.com/efbff200c26039dcd1ea0a4d585662629a8b9e69/net/http/http_basic_stream.h
[modify] https://crrev.com/efbff200c26039dcd1ea0a4d585662629a8b9e69/net/http/http_stream_factory.cc
[modify] https://crrev.com/efbff200c26039dcd1ea0a4d585662629a8b9e69/net/http/http_stream_factory.h
[modify] https://crrev.com/efbff200c26039dcd1ea0a4d585662629a8b9e69/net/http/http_stream_factory_impl.cc
[modify] https://crrev.com/efbff200c26039dcd1ea0a4d585662629a8b9e69/net/http/http_stream_factory_impl.h
[modify] https://crrev.com/efbff200c26039dcd1ea0a4d585662629a8b9e69/net/http/http_stream_factory_impl_job.cc
[modify] https://crrev.com/efbff200c26039dcd1ea0a4d585662629a8b9e69/net/http/http_stream_factory_impl_job.h
[add] https://crrev.com/efbff200c26039dcd1ea0a4d585662629a8b9e69/net/http/http_stream_factory_impl_job_controller.cc
[add] https://crrev.com/efbff200c26039dcd1ea0a4d585662629a8b9e69/net/http/http_stream_factory_impl_job_controller.h
[add] https://crrev.com/efbff200c26039dcd1ea0a4d585662629a8b9e69/net/http/http_stream_factory_impl_job_controller_unittest.cc
[modify] https://crrev.com/efbff200c26039dcd1ea0a4d585662629a8b9e69/net/http/http_stream_factory_impl_request.cc
[modify] https://crrev.com/efbff200c26039dcd1ea0a4d585662629a8b9e69/net/http/http_stream_factory_impl_request.h
[modify] https://crrev.com/efbff200c26039dcd1ea0a4d585662629a8b9e69/net/http/http_stream_factory_impl_request_unittest.cc
[modify] https://crrev.com/efbff200c26039dcd1ea0a4d585662629a8b9e69/net/http/http_stream_factory_impl_unittest.cc
[add] https://crrev.com/efbff200c26039dcd1ea0a4d585662629a8b9e69/net/http/http_stream_factory_test_util.cc
[add] https://crrev.com/efbff200c26039dcd1ea0a4d585662629a8b9e69/net/http/http_stream_factory_test_util.h
[modify] https://crrev.com/efbff200c26039dcd1ea0a4d585662629a8b9e69/net/net.gyp
[modify] https://crrev.com/efbff200c26039dcd1ea0a4d585662629a8b9e69/net/net.gypi

Project Member

Comment 4 by bugdroid1@chromium.org, Jun 17 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3c520078a25a61ccef467e00b3906fb8a98bcc1d

commit 3c520078a25a61ccef467e00b3906fb8a98bcc1d
Author: zhongyi <zhongyi@chromium.org>
Date: Fri Jun 17 22:24:54 2016

Revert of JobController 1: Remove cross reference between Request, Job, and Impl (patchset #23 id:660001 of https://codereview.chromium.org/1941083002/ )

Reason for revert:
This CL is top 1 causing crash.

Revert this patch and re-land once we fixed the bug.

Original issue's description:
> JobController 1: Remove cross reference between Request, Job, and Impl.
>
> BUG= 605398 
>
> Committed: https://crrev.com/efbff200c26039dcd1ea0a4d585662629a8b9e69
> Cr-Commit-Position: refs/heads/master@{#400086}

TBR=rch@chromium.org,rdsmith@chromium.org,tbansal@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 605398 

Review-Url: https://codereview.chromium.org/2073293002
Cr-Commit-Position: refs/heads/master@{#400525}

[modify] https://crrev.com/3c520078a25a61ccef467e00b3906fb8a98bcc1d/net/BUILD.gn
[modify] https://crrev.com/3c520078a25a61ccef467e00b3906fb8a98bcc1d/net/http/http_basic_stream.h
[modify] https://crrev.com/3c520078a25a61ccef467e00b3906fb8a98bcc1d/net/http/http_stream_factory.cc
[modify] https://crrev.com/3c520078a25a61ccef467e00b3906fb8a98bcc1d/net/http/http_stream_factory.h
[modify] https://crrev.com/3c520078a25a61ccef467e00b3906fb8a98bcc1d/net/http/http_stream_factory_impl.cc
[modify] https://crrev.com/3c520078a25a61ccef467e00b3906fb8a98bcc1d/net/http/http_stream_factory_impl.h
[modify] https://crrev.com/3c520078a25a61ccef467e00b3906fb8a98bcc1d/net/http/http_stream_factory_impl_job.cc
[modify] https://crrev.com/3c520078a25a61ccef467e00b3906fb8a98bcc1d/net/http/http_stream_factory_impl_job.h
[delete] https://crrev.com/ea63767b61ae1a8935b420685bbee58467b68b5b/net/http/http_stream_factory_impl_job_controller.cc
[delete] https://crrev.com/ea63767b61ae1a8935b420685bbee58467b68b5b/net/http/http_stream_factory_impl_job_controller.h
[delete] https://crrev.com/ea63767b61ae1a8935b420685bbee58467b68b5b/net/http/http_stream_factory_impl_job_controller_unittest.cc
[modify] https://crrev.com/3c520078a25a61ccef467e00b3906fb8a98bcc1d/net/http/http_stream_factory_impl_request.cc
[modify] https://crrev.com/3c520078a25a61ccef467e00b3906fb8a98bcc1d/net/http/http_stream_factory_impl_request.h
[modify] https://crrev.com/3c520078a25a61ccef467e00b3906fb8a98bcc1d/net/http/http_stream_factory_impl_request_unittest.cc
[modify] https://crrev.com/3c520078a25a61ccef467e00b3906fb8a98bcc1d/net/http/http_stream_factory_impl_unittest.cc
[delete] https://crrev.com/ea63767b61ae1a8935b420685bbee58467b68b5b/net/http/http_stream_factory_test_util.cc
[delete] https://crrev.com/ea63767b61ae1a8935b420685bbee58467b68b5b/net/http/http_stream_factory_test_util.h
[modify] https://crrev.com/3c520078a25a61ccef467e00b3906fb8a98bcc1d/net/net.gyp
[modify] https://crrev.com/3c520078a25a61ccef467e00b3906fb8a98bcc1d/net/net.gypi

Project Member

Comment 5 by bugdroid1@chromium.org, Jun 18 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3c41298615d9dc25bd02bc3f3b1169268af000c6

commit 3c41298615d9dc25bd02bc3f3b1169268af000c6
Author: zhongyi <zhongyi@chromium.org>
Date: Sat Jun 18 00:34:30 2016

JobController 1: Adding a new class HttpStreamFactoryImpl::JobController and remove cross reference between HttpStreamFactoryImpl::Request, HttpStreamFactoryImpl::Job, and HttpStreamFactoryImpl::Impl.

BUG= 605398 

Committed: https://crrev.com/efbff200c26039dcd1ea0a4d585662629a8b9e69
Review-Url: https://codereview.chromium.org/1941083002
Cr-Original-Commit-Position: refs/heads/master@{#400086}
Cr-Commit-Position: refs/heads/master@{#400552}

[modify] https://crrev.com/3c41298615d9dc25bd02bc3f3b1169268af000c6/net/BUILD.gn
[modify] https://crrev.com/3c41298615d9dc25bd02bc3f3b1169268af000c6/net/http/http_basic_stream.h
[modify] https://crrev.com/3c41298615d9dc25bd02bc3f3b1169268af000c6/net/http/http_stream_factory.cc
[modify] https://crrev.com/3c41298615d9dc25bd02bc3f3b1169268af000c6/net/http/http_stream_factory.h
[modify] https://crrev.com/3c41298615d9dc25bd02bc3f3b1169268af000c6/net/http/http_stream_factory_impl.cc
[modify] https://crrev.com/3c41298615d9dc25bd02bc3f3b1169268af000c6/net/http/http_stream_factory_impl.h
[modify] https://crrev.com/3c41298615d9dc25bd02bc3f3b1169268af000c6/net/http/http_stream_factory_impl_job.cc
[modify] https://crrev.com/3c41298615d9dc25bd02bc3f3b1169268af000c6/net/http/http_stream_factory_impl_job.h
[add] https://crrev.com/3c41298615d9dc25bd02bc3f3b1169268af000c6/net/http/http_stream_factory_impl_job_controller.cc
[add] https://crrev.com/3c41298615d9dc25bd02bc3f3b1169268af000c6/net/http/http_stream_factory_impl_job_controller.h
[add] https://crrev.com/3c41298615d9dc25bd02bc3f3b1169268af000c6/net/http/http_stream_factory_impl_job_controller_unittest.cc
[modify] https://crrev.com/3c41298615d9dc25bd02bc3f3b1169268af000c6/net/http/http_stream_factory_impl_request.cc
[modify] https://crrev.com/3c41298615d9dc25bd02bc3f3b1169268af000c6/net/http/http_stream_factory_impl_request.h
[modify] https://crrev.com/3c41298615d9dc25bd02bc3f3b1169268af000c6/net/http/http_stream_factory_impl_request_unittest.cc
[modify] https://crrev.com/3c41298615d9dc25bd02bc3f3b1169268af000c6/net/http/http_stream_factory_impl_unittest.cc
[add] https://crrev.com/3c41298615d9dc25bd02bc3f3b1169268af000c6/net/http/http_stream_factory_test_util.cc
[add] https://crrev.com/3c41298615d9dc25bd02bc3f3b1169268af000c6/net/http/http_stream_factory_test_util.h
[modify] https://crrev.com/3c41298615d9dc25bd02bc3f3b1169268af000c6/net/net.gyp
[modify] https://crrev.com/3c41298615d9dc25bd02bc3f3b1169268af000c6/net/net.gypi

Blocking: 455054
Project Member

Comment 7 by bugdroid1@chromium.org, Jul 26 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5489ba4e6d4fa5ef7be7b8bbb8521f75addab7f5

commit 5489ba4e6d4fa5ef7be7b8bbb8521f75addab7f5
Author: zhongyi <zhongyi@chromium.org>
Date: Tue Jul 26 17:47:29 2016

JobController 2: Remove reference between HttpStreamFactoryImpl::Jobs. HttpStreamFactoryImpl::JobController will take over scheduling between main job and alternative job.

BUG= 605398 

Review-Url: https://codereview.chromium.org/1952423002
Cr-Commit-Position: refs/heads/master@{#407846}

[modify] https://crrev.com/5489ba4e6d4fa5ef7be7b8bbb8521f75addab7f5/net/http/http_stream_factory_impl_job.cc
[modify] https://crrev.com/5489ba4e6d4fa5ef7be7b8bbb8521f75addab7f5/net/http/http_stream_factory_impl_job.h
[modify] https://crrev.com/5489ba4e6d4fa5ef7be7b8bbb8521f75addab7f5/net/http/http_stream_factory_impl_job_controller.cc
[modify] https://crrev.com/5489ba4e6d4fa5ef7be7b8bbb8521f75addab7f5/net/http/http_stream_factory_impl_job_controller.h
[modify] https://crrev.com/5489ba4e6d4fa5ef7be7b8bbb8521f75addab7f5/net/http/http_stream_factory_impl_job_controller_unittest.cc
[modify] https://crrev.com/5489ba4e6d4fa5ef7be7b8bbb8521f75addab7f5/net/http/http_stream_factory_impl_request_unittest.cc
[modify] https://crrev.com/5489ba4e6d4fa5ef7be7b8bbb8521f75addab7f5/net/http/http_stream_factory_test_util.cc
[modify] https://crrev.com/5489ba4e6d4fa5ef7be7b8bbb8521f75addab7f5/net/http/http_stream_factory_test_util.h
[modify] https://crrev.com/5489ba4e6d4fa5ef7be7b8bbb8521f75addab7f5/net/net.gyp

Status: Fixed (was: Assigned)
We also removed the MarkAlternativeServiceBroken from the Job to JobController in codereview.chromium.org/2332193003.
JobController3: Move MarkAlternativeServiceBroken to job controller

commit	a692903a8ea6b8bfa576a29d64b413aeae641432
Author:	zhongyi <zhongyi@chromium.org>	
Date: Thu Sep 15 03:02:57 2016

JobController3: Move MarkAlternativeServiceBroken to job controller

Review-Url: https://codereview.chromium.org/2332193003
Cr-Commit-Position: refs/heads/master@{#418768}
Components: Internals>Network
Components: -Internals>Network>HTTP

Sign in to add a comment