Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2260623002:
Race TCP connection to proxies with QUIC connections (Closed)
DescriptionRace TCP connections to proxies with QUIC connections
After the default job finishes proxy resolution, it queries
ProxyDelegate on whether the resolved proxy supports an
alternative proxy server or not. If yes, a new job is
started. This job is provided the alternative proxy server,
and races with the main job.
If the alternative proxy server is found to be broken, the
ProxyDelegate is notified.
Changes to DataReductionProxyDelegate will be in a
subsequent CL.
BUG=343579
Committed: https://crrev.com/c3308d79532cd022c3394d6f3b0773958307d2be
Cr-Commit-Position: refs/heads/master@{#414884}
Patch Set 1 #
Total comments: 26
Patch Set 2 : Rebased, Addressed Ryan, Cherie comments #
Total comments: 25
Patch Set 3 : Addressed Cherie's comments #
Total comments: 10
Patch Set 4 : Addressed ryansturm and Cherie's comments #
Total comments: 4
Patch Set 5 : Changed ProxyDelegate functions to pure virtual #
Total comments: 2
Patch Set 6 : Rebased #
Total comments: 6
Patch Set 7 : Fixed failing test, addressed bengr comments #Patch Set 8 : PS #Patch Set 9 : Test fix #Messages
Total messages: 72 (48 generated)
Description was changed from ========== Race TCP connection to proxies with QUIC connections Race TCP connections to proxies with QUIC connections. After the default job finishes proxy resolution, it queries ProxyDelegate on whether the resolved proxy supports an alternative proxy server or not. If yes, a new job is started. This job is provided the alternative proxy server, and races with the default job. If the alternative proxy server is found to be broken, the ProxyDelegate is notified. BUG=343579 ========== to ========== Race TCP connections to proxies with QUIC connections After the default job finishes proxy resolution, it queries ProxyDelegate on whether the resolved proxy supports an alternative proxy server or not. If yes, a new job is started. This job is provided the alternative proxy server, and races with the main job. If the alternative proxy server is found to be broken, the ProxyDelegate is notified. Changes to DataReductionProxyDelegate will be in a subsequent CL. BUG=343579 ==========
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tbansal@chromium.org changed reviewers: + bengr@chromium.org, rch@chromium.org
rch, bengr: ptal. The core changes are in net/http/http_stream_factory_impl_job.cc and net/http/http_stream_factory_impl_job_controller.cc. The rest of the files contain either plumbing or tests.
rch@chromium.org changed reviewers: + zhongyi@chromium.org
+zhongyi: PTAL. Once you're happy, I'll dive in.
On 2016/08/18 22:17:29, Ryan Hamilton wrote: > +zhongyi: PTAL. Once you're happy, I'll dive in. zhongyi, link to design doc: http://shortn/_DSRQS5xvGW
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
ryansturm@chromium.org changed reviewers: + ryansturm@chromium.org
https://codereview.chromium.org/2260623002/diff/1/net/base/proxy_delegate.h File net/base/proxy_delegate.h (right): https://codereview.chromium.org/2260623002/diff/1/net/base/proxy_delegate.h#n... net/base/proxy_delegate.h:71: // GetAlternativeProxy is called after the proxy is resolved but before the s/GetAlternativeProxy is called/Called https://codereview.chromium.org/2260623002/diff/1/net/base/proxy_delegate.h#n... net/base/proxy_delegate.h:73: // resolved by the proxy service for fetching |url|. GetAlternativeProxy s/GetAlternativeProxy should set/ Sets https://codereview.chromium.org/2260623002/diff/1/net/base/test_proxy_delegat... File net/base/test_proxy_delegate.cc (right): https://codereview.chromium.org/2260623002/diff/1/net/base/test_proxy_delegat... net/base/test_proxy_delegate.cc:10: #include "url/gurl.h" Not sure you really need this header since you don't use the methods of GURL and it's passed by reference, but I'm not sure what the chromium style guide says about that. https://codereview.chromium.org/2260623002/diff/1/net/base/test_proxy_delegate.h File net/base/test_proxy_delegate.h (right): https://codereview.chromium.org/2260623002/diff/1/net/base/test_proxy_delegat... net/base/test_proxy_delegate.h:75: void SetAlternativeProxy(const ProxyServer& alternative_proxy_server); Can this be inlined like alternative_proxy_server()? s/SetAlternativeProxy/set_alternative_proxy_server https://codereview.chromium.org/2260623002/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/2260623002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job.h:105: const SSLConfig& server_ssl_config, nit: I wouldn't complain if you forward declared SSLConfig, it's getting pulled in via ssl_config_service.h https://codereview.chromium.org/2260623002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job.h:381: // May notify proxy delegate that the alternative proxy server is broken. The "May" here is somewhat confusing. It should be clear about when it notifies the proxy delegate. Can you change the to something more descriptive about when it informs the proxy delegate? https://codereview.chromium.org/2260623002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job.h:518: const ProxyServer& proxy_server, s/proxy_server/alternative_proxy_server
Looking good. I haven't look on the tests yet, let's work on the job controller work first. https://codereview.chromium.org/2260623002/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/2260623002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job.h:100: // Invoked when |job| has completed proxy resolution. nit: maybe mention it's default proxy resolution. https://codereview.chromium.org/2260623002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job.h:174: // are not used. The comments are a little confusing. If I understand correctly, QUIC proxy connection will create a alternative job with alternative proxy if there's only main job created, right? Thus the alternative job might be: 1. a job running with alternative service 2. a job running with alternative proxy And I think what you meant to say is if the alternative_service is provided, it will use the alternative_service. Otherwise, if the alternative_proxy_server is provided, it will use the the alternative_proxy_server. https://codereview.chromium.org/2260623002/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2260623002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job_controller.cc:369: It seems like this method is only expected to be called when job controller only creates main job. And the main job want to run this code to create an alt_job with alternative proxy server. How about this: s/maybe_create_alt_proxy_job_/started_alternative_proxy_server_job_ default to false. set maybe_create_alt_proxy_job_ to TRUE when CreateJobs creates ONLY main job. In this method if (！maybe_create_alt_proxy_job_) return; DCHECK_EQ(job, main_job_.get()); DCHECK(!alternative_job_.get()); maybe_create_alt_proxy_job_ = false; // Perform checkings on whether we should create alt proxy job. ... // Create alt proxy job. https://codereview.chromium.org/2260623002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job_controller.cc:946: ProxyServer* alternative_proxy_server) const { This is only called on OnResolveProxyComplete() right? How about merge the two into one？ With the change I proposed. You can avoid invoking this method when proxy resolution is complete for alt job or preconnect job.
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Ryan, Cherie: PTAL. Thanks. https://codereview.chromium.org/2260623002/diff/1/net/base/proxy_delegate.h File net/base/proxy_delegate.h (right): https://codereview.chromium.org/2260623002/diff/1/net/base/proxy_delegate.h#n... net/base/proxy_delegate.h:71: // GetAlternativeProxy is called after the proxy is resolved but before the On 2016/08/19 19:03:23, RyanSturm wrote: > s/GetAlternativeProxy is called/Called Done. https://codereview.chromium.org/2260623002/diff/1/net/base/proxy_delegate.h#n... net/base/proxy_delegate.h:73: // resolved by the proxy service for fetching |url|. GetAlternativeProxy On 2016/08/19 19:03:23, RyanSturm wrote: > s/GetAlternativeProxy should set/ Sets Done. https://codereview.chromium.org/2260623002/diff/1/net/base/test_proxy_delegat... File net/base/test_proxy_delegate.cc (right): https://codereview.chromium.org/2260623002/diff/1/net/base/test_proxy_delegat... net/base/test_proxy_delegate.cc:10: #include "url/gurl.h" On 2016/08/19 19:03:23, RyanSturm wrote: > Not sure you really need this header since you don't use the methods of GURL and > it's passed by reference, but I'm not sure what the chromium style guide says > about that. Done. https://codereview.chromium.org/2260623002/diff/1/net/base/test_proxy_delegate.h File net/base/test_proxy_delegate.h (right): https://codereview.chromium.org/2260623002/diff/1/net/base/test_proxy_delegat... net/base/test_proxy_delegate.h:75: void SetAlternativeProxy(const ProxyServer& alternative_proxy_server); On 2016/08/19 19:03:23, RyanSturm wrote: > Can this be inlined like alternative_proxy_server()? > s/SetAlternativeProxy/set_alternative_proxy_server Done. https://codereview.chromium.org/2260623002/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/2260623002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job.h:100: // Invoked when |job| has completed proxy resolution. On 2016/08/19 23:08:42, Zhongyi Shi wrote: > nit: maybe mention it's default proxy resolution. I do not understand this comment. You mean it is called only by the main job? What is "default" proxy resolution? https://codereview.chromium.org/2260623002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job.h:105: const SSLConfig& server_ssl_config, On 2016/08/19 19:03:23, RyanSturm wrote: > nit: I wouldn't complain if you forward declared SSLConfig, it's getting pulled > in via ssl_config_service.h Done. https://codereview.chromium.org/2260623002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job.h:174: // are not used. On 2016/08/19 23:08:42, Zhongyi Shi wrote: > The comments are a little confusing. If I understand correctly, QUIC proxy > connection will create a alternative job with alternative proxy if there's only > main job created, right? > > Thus the alternative job might be: > 1. a job running with alternative service > 2. a job running with alternative proxy > > And I think what you meant to say is if the alternative_service is provided, it > will use the alternative_service. Otherwise, if the alternative_proxy_server is > provided, it will use the the alternative_proxy_server. Done. https://codereview.chromium.org/2260623002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job.h:381: // May notify proxy delegate that the alternative proxy server is broken. On 2016/08/19 19:03:23, RyanSturm wrote: > The "May" here is somewhat confusing. It should be clear about when it notifies > the proxy delegate. Can you change the to something more descriptive about when > it informs the proxy delegate? Done. https://codereview.chromium.org/2260623002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job.h:518: const ProxyServer& proxy_server, On 2016/08/19 19:03:23, RyanSturm wrote: > s/proxy_server/alternative_proxy_server Done. https://codereview.chromium.org/2260623002/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2260623002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job_controller.cc:369: On 2016/08/19 23:08:42, Zhongyi Shi wrote: > It seems like this method is only expected to be called when job controller only > creates main job. And the main job want to run this code to create an alt_job > with alternative proxy server. > > How about this: > > s/maybe_create_alt_proxy_job_/started_alternative_proxy_server_job_ > default to false. > > set maybe_create_alt_proxy_job_ to TRUE when CreateJobs creates ONLY main job. > > In this method > if (！maybe_create_alt_proxy_job_) > return; > DCHECK_EQ(job, main_job_.get()); > DCHECK(!alternative_job_.get()); > maybe_create_alt_proxy_job_ = false; > // Perform checkings on whether we should create alt proxy job. > ... > // Create alt proxy job. This is a good approach. Done what you suggested but used a slightly different variable name. https://codereview.chromium.org/2260623002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job_controller.cc:946: ProxyServer* alternative_proxy_server) const { On 2016/08/19 23:08:42, Zhongyi Shi wrote: > This is only called on OnResolveProxyComplete() right? How about merge the two > into one？ > > With the change I proposed. You can avoid invoking this method when proxy > resolution is complete for alt job or preconnect job. I can merge but that will make one single method too long, and this feels like a good abstraction which can be separated out in a const-method. Also, in future, the Controller may want to do some other shiny things in OnProxyResolveComplete(), so it may not be a good idea to make it too long (I think).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The tests looking good, the FailedAlternativeProxy might need more work after we figure out how to start the alt proxy job. Feel free to snag my calendar if you need a back o back review. https://codereview.chromium.org/2260623002/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/2260623002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job.h:100: // Invoked when |job| has completed proxy resolution. On 2016/08/20 01:53:39, tbansal1 wrote: > On 2016/08/19 23:08:42, Zhongyi Shi wrote: > > nit: maybe mention it's default proxy resolution. > > I do not understand this comment. You mean it is called only by the main job? > What is "default" proxy resolution? Yeah, just mention that this method is trying to set a alt proxy jov with a alternative proxy server if it's a HTTP request. https://codereview.chromium.org/2260623002/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2260623002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job_controller_unittest.cc:80: This probably needs a rebase as you have already remove those dead code in another CL. https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:718: if (alternative_proxy_server_.is_valid()) { nit: DCHECK_EQ(job_type_, ALTERNATIVE); https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:1551: !other_job_alternative_proxy_server_.is_valid()) { nit: DCHECK_EQ(alternative_service_.protocol != UNINITIALIZED_ALTERNATE_PROTOCOL || other_job_alternative_service_.protocol != UNINITIALIZED_ALTERNATE_PROTOCOL); https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.h:170: // Constructor for alternative Job. The Job is owned by |delegate|, hence Reading the comments. I think it might be better to provide another constructor for alternative proxy job so that we prevent the case where both |alternative_service| and |alternative_proxy_server| are valid. It also helps us to ready the code whether we are creating a alt job with alternative service or alternative proxy server. https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:391: main_job_is_blocked_ = true; I am a little bit concerned on starting the alternative job while the main job is actually not IO_PENDING. Can we post a task here to start the alternative job? https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:647: } else { Rather than wait until later check, how about set the variable to true if the request is using HTTP? https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:943: ProxyServer* alternative_proxy_server) const { nit: DCHECK(!alternative_proxy_server); https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:969: if (!can_start_alternative_proxy_job_) { I think this is more like a meta bit of information. Probably better to move it to the very beginning of this method. An early return is always preferred. https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:691: TEST_F(HttpStreamFactoryImplJobControllerTest, DelayedTCPAlternativeProxy) { Now that you are here, shall we add a test case covers the following case: HTTPS request, not alt service. verify there's no alt job set? https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:735: // main job is not blocked. nit: remove redundant space in comments.
Patchset #3 (id:60001) has been deleted
ryansturm, zhongyi: ptal. Thanks! https://codereview.chromium.org/2260623002/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/2260623002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job.h:100: // Invoked when |job| has completed proxy resolution. On 2016/08/20 06:15:40, Zhongyi Shi wrote: > On 2016/08/20 01:53:39, tbansal1 wrote: > > On 2016/08/19 23:08:42, Zhongyi Shi wrote: > > > nit: maybe mention it's default proxy resolution. > > > > I do not understand this comment. You mean it is called only by the main job? > > What is "default" proxy resolution? > > Yeah, just mention that this method is trying to set a alt proxy jov with a > alternative proxy server if it's a HTTP request. Done. I did not mention the specifics ("a HTTP request.") because that seems like an internal detail. https://codereview.chromium.org/2260623002/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2260623002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job_controller_unittest.cc:80: On 2016/08/20 06:15:40, Zhongyi Shi wrote: > This probably needs a rebase as you have already remove those dead code in > another CL. Acknowledged. I rebased in PS 2. https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:718: if (alternative_proxy_server_.is_valid()) { On 2016/08/20 06:15:40, Zhongyi Shi wrote: > nit: DCHECK_EQ(job_type_, ALTERNATIVE); good idea, I added a similar DCHECK in the constructor of the Job above. https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:1551: !other_job_alternative_proxy_server_.is_valid()) { On 2016/08/20 06:15:40, Zhongyi Shi wrote: > nit: > DCHECK_EQ(alternative_service_.protocol != UNINITIALIZED_ALTERNATE_PROTOCOL || > other_job_alternative_service_.protocol != UNINITIALIZED_ALTERNATE_PROTOCOL); Isn't this the same as the DCHECKs in lInes 1558-1560 below? https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.h:170: // Constructor for alternative Job. The Job is owned by |delegate|, hence On 2016/08/20 06:15:40, Zhongyi Shi wrote: > Reading the comments. I think it might be better to provide another constructor > for alternative proxy job so that we prevent the case where both > |alternative_service| and |alternative_proxy_server| are valid. It also helps us > to ready the code whether we are creating a alt job with alternative service or > alternative proxy server. I thought about it, and I had initially written code exactly like that. But that meant that the Job class had 4 constructors: (i) Main job (ii) Alternative service job that takes alternative service as an argument (iii) alternative proxy server job that takes alternative proxy server job as an argument (iv) A private constructor that is called by the other 3 constructos, and does all the heavy lifting. I am okay with this approach, but 4 constructors seemed like too much repetition? wdyt? https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:391: main_job_is_blocked_ = true; On 2016/08/20 06:15:40, Zhongyi Shi wrote: > I am a little bit concerned on starting the alternative job while the main job > is actually not IO_PENDING. Can we post a task here to start the alternative > job? Done. https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:647: } else { On 2016/08/20 06:15:40, Zhongyi Shi wrote: > Rather than wait until later check, how about set the variable to true if the > request is using HTTP? I do not understand this comment. It seems that here we should just be checking if the alt service job was created or not. And, the URL is http or not can be checked later. https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:943: ProxyServer* alternative_proxy_server) const { On 2016/08/20 06:15:40, Zhongyi Shi wrote: > nit: DCHECK(!alternative_proxy_server); Done. https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:969: if (!can_start_alternative_proxy_job_) { On 2016/08/20 06:15:40, Zhongyi Shi wrote: > I think this is more like a meta bit of information. Probably better to move it > to the very beginning of this method. An early return is always preferred. Done. https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:691: TEST_F(HttpStreamFactoryImplJobControllerTest, DelayedTCPAlternativeProxy) { On 2016/08/20 06:15:40, Zhongyi Shi wrote: > Now that you are here, shall we add a test case covers the following case: > HTTPS request, not alt service. verify there's no alt job set? Done. https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:735: // main job is not blocked. On 2016/08/20 06:15:40, Zhongyi Shi wrote: > nit: remove redundant space in comments. Done.
lgtm % 1 comment https://codereview.chromium.org/2260623002/diff/80001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2260623002/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:689: // Verifies that the alternative proxty server job is not created if the URL s/proxty/proxy
Thanks for the patience. Almost there :) I am always a little picky on the job controller unittest as the scheduling mechanism in job controller is really complicated and we should have more test coverage to ensure it's correctly functioning. Feel free to push back if you feel there's not much sense doing some of them. https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:1551: !other_job_alternative_proxy_server_.is_valid()) { On 2016/08/22 15:42:33, tbansal1 wrote: > On 2016/08/20 06:15:40, Zhongyi Shi wrote: > > nit: > > DCHECK_EQ(alternative_service_.protocol != UNINITIALIZED_ALTERNATE_PROTOCOL || > > other_job_alternative_service_.protocol != UNINITIALIZED_ALTERNATE_PROTOCOL); > > Isn't this the same as the DCHECKs in lInes 1558-1560 below? You won't be able to run the checks on line 1558-1560 if you are early return here, right? https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:1551: !other_job_alternative_proxy_server_.is_valid()) { On 2016/08/22 15:42:33, tbansal1 wrote: > On 2016/08/20 06:15:40, Zhongyi Shi wrote: > > nit: > > DCHECK_EQ(alternative_service_.protocol != UNINITIALIZED_ALTERNATE_PROTOCOL || > > other_job_alternative_service_.protocol != UNINITIALIZED_ALTERNATE_PROTOCOL); > > Isn't this the same as the DCHECKs in lInes 1558-1560 below? I don't think those two checks has any correlation. Checking on line 1558-1560 doesn't guarantee the checks in this early return. I was trying to confirm that one of the two jobs are using alternative service. https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.h:170: // Constructor for alternative Job. The Job is owned by |delegate|, hence On 2016/08/22 15:42:33, tbansal1 wrote: > On 2016/08/20 06:15:40, Zhongyi Shi wrote: > > Reading the comments. I think it might be better to provide another > constructor > > for alternative proxy job so that we prevent the case where both > > |alternative_service| and |alternative_proxy_server| are valid. It also helps > us > > to ready the code whether we are creating a alt job with alternative service > or > > alternative proxy server. > > I thought about it, and I had initially written code exactly like that. But that > meant that the Job class had 4 constructors: > (i) Main job > (ii) Alternative service job that takes alternative service as an argument > (iii) alternative proxy server job that takes alternative proxy server job as an > argument > (iv) A private constructor that is called by the other 3 constructos, and does > all the heavy lifting. > > I am okay with this approach, but 4 constructors seemed like too much > repetition? wdyt? The old code is not well written. You could possibly have only one generic constructor for HttpStreamFactoryImpl::Job (we currently have two) but have 3 different APIs for HttpStreamFactoryImpl::JobFactory::CreateJob(we currently have two). Does that sounds plausible? https://cs.chromium.org/chromium/src/net/http/http_stream_factory_impl_job.h?... https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:647: } else { On 2016/08/22 15:42:33, tbansal1 wrote: > On 2016/08/20 06:15:40, Zhongyi Shi wrote: > > Rather than wait until later check, how about set the variable to true if the > > request is using HTTP? > > I do not understand this comment. It seems that here we should just be checking > if the alt service job was created or not. And, the URL is http or not can be > checked later. Okay. I thought the url scheme check can be part of the meta bit to decide whether start a proxy server job or not. But fine, you're right, not too much overhead to delay to ShouldCreate..Job(). https://codereview.chromium.org/2260623002/diff/80001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2260623002/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:690: // scheme is HTTPS. nit: shall we change this test or add another test to cover: we start 2 jobs: main job, alt service job. And alt service job fails really early due to invalid port. main job resumes, make sure main job won't create another alt proxy server job? https://codereview.chromium.org/2260623002/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:699: // Enable delayed TCP and set time delay for waiting job. You probably don't need this delayed TCP setting up as you are only verifying that no alt job is created if the URL scheme is HTTPS. https://codereview.chromium.org/2260623002/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:723: Shall we add a test case where HTTP request doesn't have a proxy server set shouldn't create a alt proxy server job created? https://codereview.chromium.org/2260623002/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:796: EXPECT_TRUE(job_controller_->main_job()); nit: EXPECT_TRUE(job_controller_->main_job()->is_waiting());
Patchset #4 (id:100001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #4 (id:120001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:140001) has been deleted
Patchset #4 (id:160001) has been deleted
Patchset #4 (id:180001) has been deleted
Cherie: ptal. https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:1551: !other_job_alternative_proxy_server_.is_valid()) { On 2016/08/23 05:42:33, Zhongyi Shi wrote: > On 2016/08/22 15:42:33, tbansal1 wrote: > > On 2016/08/20 06:15:40, Zhongyi Shi wrote: > > > nit: > > > DCHECK_EQ(alternative_service_.protocol != UNINITIALIZED_ALTERNATE_PROTOCOL > || > > > other_job_alternative_service_.protocol != > UNINITIALIZED_ALTERNATE_PROTOCOL); > > > > Isn't this the same as the DCHECKs in lInes 1558-1560 below? > > I don't think those two checks has any correlation. Checking on line 1558-1560 > doesn't guarantee the checks in this early return. I was trying to confirm that > one of the two jobs are using alternative service. I think what you meant was: DCHECK(alternative_service_.protocol == UNINITIALIZED_ALTERNATE_PROTOCOL || other_job_alternative_service_.protocol == UNINITIALIZED_ALTERNATE_PROTOCOL); Done. https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.h:170: // Constructor for alternative Job. The Job is owned by |delegate|, hence On 2016/08/23 05:42:33, Zhongyi Shi wrote: > On 2016/08/22 15:42:33, tbansal1 wrote: > > On 2016/08/20 06:15:40, Zhongyi Shi wrote: > > > Reading the comments. I think it might be better to provide another > > constructor > > > for alternative proxy job so that we prevent the case where both > > > |alternative_service| and |alternative_proxy_server| are valid. It also > helps > > us > > > to ready the code whether we are creating a alt job with alternative service > > or > > > alternative proxy server. > > > > I thought about it, and I had initially written code exactly like that. But > that > > meant that the Job class had 4 constructors: > > (i) Main job > > (ii) Alternative service job that takes alternative service as an argument > > (iii) alternative proxy server job that takes alternative proxy server job as > an > > argument > > (iv) A private constructor that is called by the other 3 constructos, and does > > all the heavy lifting. > > > > I am okay with this approach, but 4 constructors seemed like too much > > repetition? wdyt? > > The old code is not well written. You could possibly have only one generic > constructor for HttpStreamFactoryImpl::Job (we currently have two) but have 3 > different APIs for HttpStreamFactoryImpl::JobFactory::CreateJob(we currently > have two). Does that sounds plausible? > > https://cs.chromium.org/chromium/src/net/http/http_stream_factory_impl_job.h?... Done. https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2260623002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:647: } else { On 2016/08/23 05:42:33, Zhongyi Shi wrote: > On 2016/08/22 15:42:33, tbansal1 wrote: > > On 2016/08/20 06:15:40, Zhongyi Shi wrote: > > > Rather than wait until later check, how about set the variable to true if > the > > > request is using HTTP? > > > > I do not understand this comment. It seems that here we should just be > checking > > if the alt service job was created or not. And, the URL is http or not can be > > checked later. > > Okay. I thought the url scheme check can be part of the meta bit to decide > whether start a proxy server job or not. But fine, you're right, not too much > overhead to delay to ShouldCreate..Job(). Acknowledged. https://codereview.chromium.org/2260623002/diff/80001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2260623002/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:689: // Verifies that the alternative proxty server job is not created if the URL On 2016/08/22 16:24:41, RyanSturm wrote: > s/proxty/proxy Done. https://codereview.chromium.org/2260623002/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:690: // scheme is HTTPS. On 2016/08/23 05:42:33, Zhongyi Shi wrote: > nit: shall we change this test or add another test to cover: > we start 2 jobs: main job, alt service job. And alt service job fails really > early due to invalid port. main job resumes, make sure main job won't create > another alt proxy server job? I modified WithQUICAlternativeProxyMarkedAsBad in http_stream_factory_impl_unittest.cc to test this. Please let me know what you think. https://codereview.chromium.org/2260623002/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:699: // Enable delayed TCP and set time delay for waiting job. On 2016/08/23 05:42:33, Zhongyi Shi wrote: > You probably don't need this delayed TCP setting up as you are only verifying > that no alt job is created if the URL scheme is HTTPS. Done. https://codereview.chromium.org/2260623002/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:723: On 2016/08/23 05:42:33, Zhongyi Shi wrote: > Shall we add a test case where HTTP request doesn't have a proxy server set > shouldn't create a alt proxy server job created? Good point. Done. Added test HttpURLWithNoProxy above. https://codereview.chromium.org/2260623002/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:796: EXPECT_TRUE(job_controller_->main_job()); On 2016/08/23 05:42:33, Zhongyi Shi wrote: > nit: EXPECT_TRUE(job_controller_->main_job()->is_waiting()); Done.
job controller (+unittests) lgtm. Sorry about the delay. Recommend rch@ to review the rest.
rch: ptal. thanks.
Looks great! One nit. https://codereview.chromium.org/2260623002/diff/200001/net/base/proxy_delegate.h File net/base/proxy_delegate.h (right): https://codereview.chromium.org/2260623002/diff/200001/net/base/proxy_delegat... net/base/proxy_delegate.h:80: ProxyServer* alternative_proxy_server) const {} nit: The other methods in this class are pure so I think these should probably be so as well.
https://codereview.chromium.org/2260623002/diff/200001/net/base/proxy_delegate.h File net/base/proxy_delegate.h (right): https://codereview.chromium.org/2260623002/diff/200001/net/base/proxy_delegat... net/base/proxy_delegate.h:80: ProxyServer* alternative_proxy_server) const {} On 2016/08/25 18:39:28, Ryan Hamilton wrote: > nit: The other methods in this class are pure so I think these should probably > be so as well. Is it okay if I change the other methods to be not-pure (in a different CL)? It would reduce the overhead of adding empty implementations of the method at 3-4 different places. (But, I am okay with whatever you suggest finally.)
https://codereview.chromium.org/2260623002/diff/200001/net/base/proxy_delegate.h File net/base/proxy_delegate.h (right): https://codereview.chromium.org/2260623002/diff/200001/net/base/proxy_delegat... net/base/proxy_delegate.h:80: ProxyServer* alternative_proxy_server) const {} On 2016/08/25 18:42:02, tbansal1 wrote: > On 2016/08/25 18:39:28, Ryan Hamilton wrote: > > nit: The other methods in this class are pure so I think these should probably > > be so as well. > > Is it okay if I change the other methods to be not-pure (in a different CL)? It > would reduce the overhead of adding empty implementations of the method at 3-4 > different places. (But, I am okay with whatever you suggest finally.) I think it's best to do it now because it forces us to look at those call sites and make sure they don't actually need to do something. (I'm 99% sure they don't, but it'd be worth looking at them to make sure) Sorry for the added headache!
rch: ptal. thanks. https://codereview.chromium.org/2260623002/diff/200001/net/base/proxy_delegate.h File net/base/proxy_delegate.h (right): https://codereview.chromium.org/2260623002/diff/200001/net/base/proxy_delegat... net/base/proxy_delegate.h:80: ProxyServer* alternative_proxy_server) const {} On 2016/08/25 18:50:26, Ryan Hamilton wrote: > On 2016/08/25 18:42:02, tbansal1 wrote: > > On 2016/08/25 18:39:28, Ryan Hamilton wrote: > > > nit: The other methods in this class are pure so I think these should > probably > > > be so as well. > > > > Is it okay if I change the other methods to be not-pure (in a different CL)? > It > > would reduce the overhead of adding empty implementations of the method at 3-4 > > different places. (But, I am okay with whatever you suggest finally.) > > I think it's best to do it now because it forces us to look at those call sites > and make sure they don't actually need to do something. (I'm 99% sure they > don't, but it'd be worth looking at them to make sure) > > Sorry for the added headache! Going with your first suggestion of changing them to pure virtual to be consistent with other functions.
lgtm! https://codereview.chromium.org/2260623002/diff/220001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc (right): https://codereview.chromium.org/2260623002/diff/220001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc:101: net::ProxyServer* alternative_proxy_server) const {} Presumably you're doing to cook up a follow-up CL which does something here, right?
https://codereview.chromium.org/2260623002/diff/220001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc (right): https://codereview.chromium.org/2260623002/diff/220001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc:101: net::ProxyServer* alternative_proxy_server) const {} On 2016/08/25 21:09:34, Ryan Hamilton wrote: > Presumably you're doing to cook up a follow-up CL which does something here, > right? Yes, that CL would be Flywheel specific, but I will cc' you. That CL is coming soon too (I already have it working with the tests etc.)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
lgtm https://codereview.chromium.org/2260623002/diff/240001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.h (right): https://codereview.chromium.org/2260623002/diff/240001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.h:68: void OnAlternativeProxyBroken( The term "Bad" is used in the network stack. Perhaps we should call this OnAlternativeProxyBad? https://codereview.chromium.org/2260623002/diff/240001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/2260623002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:171: // Constructor for alternative Job. The Job is owned by |delegate|, hence for -> for the https://codereview.chromium.org/2260623002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:176: // Further, if |alternative_proxy_server| is valid but bad proxy, then is -> is a
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2260623002/diff/240001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.h (right): https://codereview.chromium.org/2260623002/diff/240001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.h:68: void OnAlternativeProxyBroken( On 2016/08/26 17:34:10, bengr wrote: > The term "Bad" is used in the network stack. Perhaps we should call this > OnAlternativeProxyBad? I think the network stack uses "Bad" for proxy and "Broken" for alternative protocols (e.g., QUIC). It is not clear what to use for alternative protocol proxy. I prefer "Broken" because (to me) it implies that the failure is related to the alternative protocol, and the proxy may still be usable (through traditional H2 protocol). https://codereview.chromium.org/2260623002/diff/240001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/2260623002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:171: // Constructor for alternative Job. The Job is owned by |delegate|, hence On 2016/08/26 17:34:11, bengr wrote: > for -> for the Done. https://codereview.chromium.org/2260623002/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:176: // Further, if |alternative_proxy_server| is valid but bad proxy, then On 2016/08/26 17:34:10, bengr wrote: > is -> is a Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Patchset #8 (id:280001) has been deleted
Patchset #9 (id:320001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #9 (id:340001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ryansturm@chromium.org, zhongyi@chromium.org, rch@chromium.org, bengr@chromium.org Link to the patchset: https://codereview.chromium.org/2260623002/#ps360001 (title: "Test fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Race TCP connections to proxies with QUIC connections After the default job finishes proxy resolution, it queries ProxyDelegate on whether the resolved proxy supports an alternative proxy server or not. If yes, a new job is started. This job is provided the alternative proxy server, and races with the main job. If the alternative proxy server is found to be broken, the ProxyDelegate is notified. Changes to DataReductionProxyDelegate will be in a subsequent CL. BUG=343579 ========== to ========== Race TCP connections to proxies with QUIC connections After the default job finishes proxy resolution, it queries ProxyDelegate on whether the resolved proxy supports an alternative proxy server or not. If yes, a new job is started. This job is provided the alternative proxy server, and races with the main job. If the alternative proxy server is found to be broken, the ProxyDelegate is notified. Changes to DataReductionProxyDelegate will be in a subsequent CL. BUG=343579 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:360001)
Message was sent while issue was closed.
Description was changed from ========== Race TCP connections to proxies with QUIC connections After the default job finishes proxy resolution, it queries ProxyDelegate on whether the resolved proxy supports an alternative proxy server or not. If yes, a new job is started. This job is provided the alternative proxy server, and races with the main job. If the alternative proxy server is found to be broken, the ProxyDelegate is notified. Changes to DataReductionProxyDelegate will be in a subsequent CL. BUG=343579 ========== to ========== Race TCP connections to proxies with QUIC connections After the default job finishes proxy resolution, it queries ProxyDelegate on whether the resolved proxy supports an alternative proxy server or not. If yes, a new job is started. This job is provided the alternative proxy server, and races with the main job. If the alternative proxy server is found to be broken, the ProxyDelegate is notified. Changes to DataReductionProxyDelegate will be in a subsequent CL. BUG=343579 Committed: https://crrev.com/c3308d79532cd022c3394d6f3b0773958307d2be Cr-Commit-Position: refs/heads/master@{#414884} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/c3308d79532cd022c3394d6f3b0773958307d2be Cr-Commit-Position: refs/heads/master@{#414884} |
