Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2814633003:
Extract Proxy Resolution out of HttpStreamFactoryImpl::Job (Closed)
DescriptionExtract Proxy Resolution out of HttpStreamFactoryImpl::Job
This is to extract proxy resolution out of HttpStreamFactoryImpl::Job
to simplify the logic in the Job class. A side benefit is that we will
have fewer proxy resolutions that we need to do. See linked doc for
more details.
Design doc:
https://docs.google.com/document/d/1xKeUygQRvYXeg55Jt_f0LYHAZsJ98O10uQPLmtmNGtQ/edit?usp=sharing
BUG=475060
Review-Url: https://codereview.chromium.org/2814633003
Cr-Commit-Position: refs/heads/master@{#471899}
Committed: https://chromium.googlesource.com/chromium/src/+/c9a993789f4732a105d5ede4f9f36e0f7e0ce9b9
Patch Set 1 : fix remaining tests #
Total comments: 10
Patch Set 2 : Sync #Patch Set 3 : add proxy resolution tests #Patch Set 4 : temp (do not review this patch. review next patch) #
Total comments: 2
Patch Set 5 : self #Patch Set 6 : add one more test for ReconsiderProxyAfterError #
Total comments: 4
Patch Set 7 : Sync #Patch Set 8 : Fix tests to use MockQuicData with Ryan's help #Patch Set 9 : address comments #Patch Set 10 : unstage unrelated changes #
Total comments: 64
Patch Set 11 : Sync #Patch Set 12 : address comments #Patch Set 13 : revert one change #
Total comments: 1
Patch Set 14 : fix alt proxy tests #
Total comments: 17
Patch Set 15 : round 2! #Patch Set 16 : fix re-entrancy and adds tests #Patch Set 17 : self #Patch Set 18 : minor test fix #Patch Set 19 : self #Patch Set 20 : Sync #Patch Set 21 : cleanup #Patch Set 22 : Sync #Patch Set 23 : for debugging on bot4 #Patch Set 24 : apply predictor fix #Patch Set 25 : unstage debug statements #Patch Set 26 : rebased #Patch Set 27 : rebased #Depends on Patchset: Messages
Total messages: 119 (90 generated)
Description was changed from ========== Refactory Proxy Resolution into HttpStreamFactoryImpl::JobController BUG= ========== to ========== Refactor Proxy Resolution into HttpStreamFactoryImpl::JobController BUG= ==========
The CQ bit was checked by xunjieli@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 xunjieli@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
Patchset #1 (id:180001) has been deleted
Patchset #1 (id:200001) has been deleted
Description was changed from ========== Refactor Proxy Resolution into HttpStreamFactoryImpl::JobController BUG= ========== to ========== Refactor Proxy Resolution into HttpStreamFactoryImpl::JobController This is to extract proxy resolution out of HttpStreamFactoryImpl::Job to simplify the logic in the Job class. A side benefit is that we will have fewer proxy resolutions that we need to do. See linked doc for more details. Design doc: https://docs.google.com/document/d/1xKeUygQRvYXeg55Jt_f0LYHAZsJ98O10uQPLmtmNG... BUG=475060 ==========
Description was changed from ========== Refactor Proxy Resolution into HttpStreamFactoryImpl::JobController This is to extract proxy resolution out of HttpStreamFactoryImpl::Job to simplify the logic in the Job class. A side benefit is that we will have fewer proxy resolutions that we need to do. See linked doc for more details. Design doc: https://docs.google.com/document/d/1xKeUygQRvYXeg55Jt_f0LYHAZsJ98O10uQPLmtmNG... BUG=475060 ========== to ========== Extract Proxy Resolution out of HttpStreamFactoryImpl::Job This is to extract proxy resolution out of HttpStreamFactoryImpl::Job to simplify the logic in the Job class. A side benefit is that we will have fewer proxy resolutions that we need to do. See linked doc for more details. Design doc: https://docs.google.com/document/d/1xKeUygQRvYXeg55Jt_f0LYHAZsJ98O10uQPLmtmNG... BUG=475060 ==========
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Patchset #1 (id:220001) 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...
xunjieli@chromium.org changed reviewers: + bnc@chromium.org
Bence, Please do a high-level review. This is by no means polished, but I'd like to get some early feedback. I think I fixed all unit tests.
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...)
On 2017/04/25 20:51:53, xunjieli wrote: > Bence, Please do a high-level review. This is by no means polished, but I'd like > to get some early feedback. I think I fixed all unit tests. I think this is in a pretty good shape. Thanks for all the gruntwork of fixing the test, factoring out and mocking out QuicStreamRequestFactory, wiring up reconsider_proxy, etc.!
And here are some comments. https://codereview.chromium.org/2814633003/diff/240001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2814633003/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:748: } while (next_state_ != STATE_NONE); Do you not need to exit the loop if ResolveProxy returns ERR_IO_PENDING? https://codereview.chromium.org/2814633003/diff/240001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2814633003/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:93: // A mock QuicStreamRequest that always succeeds. Always returns |net_error|? Or do we really need a comment here? https://codereview.chromium.org/2814633003/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:129: // A mock QuicStreamRequest that always succeeds. Please update comment. https://codereview.chromium.org/2814633003/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:568: // main job should not be blockd because alt job returned ERR_IO_PENDING. s/blockd/blocked/ https://codereview.chromium.org/2814633003/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:882: // Set a QUIC alternative service for the server. Optional: this comment does not add any value, you could remove it.
rch@chromium.org changed reviewers: + rch@chromium.org
The CQ bit was checked by xunjieli@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
One question for Ryan below. PTAL. Thanks! https://codereview.chromium.org/2814633003/diff/240001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2814633003/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:748: } while (next_state_ != STATE_NONE); On 2017/04/26 13:55:39, Bence wrote: > Do you not need to exit the loop if ResolveProxy returns ERR_IO_PENDING? Done. Good catch. I have added a test case for this. https://codereview.chromium.org/2814633003/diff/240001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2814633003/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:93: // A mock QuicStreamRequest that always succeeds. On 2017/04/26 13:55:39, Bence wrote: > Always returns |net_error|? Or do we really need a comment here? Acknowledged. No longer relevant. https://codereview.chromium.org/2814633003/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:129: // A mock QuicStreamRequest that always succeeds. On 2017/04/26 13:55:39, Bence wrote: > Please update comment. Acknowledged. No longer relevant. https://codereview.chromium.org/2814633003/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:568: // main job should not be blockd because alt job returned ERR_IO_PENDING. On 2017/04/26 13:55:39, Bence wrote: > s/blockd/blocked/ Done. https://codereview.chromium.org/2814633003/diff/240001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:882: // Set a QUIC alternative service for the server. On 2017/04/26 13:55:39, Bence wrote: > Optional: this comment does not add any value, you could remove it. Done. https://codereview.chromium.org/2814633003/diff/300001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2814633003/diff/300001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1317: client_maker_.MakeInitialSettingsPacket(1, nullptr)); Ryan: so I tried your suggestion. However, I think this pulls too many Quic dependencies. It's hard to write test expectations. For example, in fixing this test, I have to match the exact settings packet that is sent out when crypto handshake is confirmed, which is really hard to match. This test doesn't know the connection_id, for example. The expected packet is: Client: Appending header: { connection_id: 4560497761450018688, connection_id_length: 8, packet_number_length: 1, reset_flag: 0, version_flag: 1, version:, packet_number: 1 } Currently I have: Client: Appending header: { connection_id: 0, connection_id_length: 8, packet_number_length: 1, reset_flag: 0, version_flag: 1, version:, packet_number: 1 }
The CQ bit was checked by xunjieli@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...
xunjieli@chromium.org changed reviewers: + tbansal@chromium.org, zhongyi@chromium.org
xunjieli@chromium.org changed required reviewers: + bnc@chromium.org, rch@chromium.org
I am nervous about this refactor, so + Cherie and Tarun to get some extra scrutiny on this change. I added a few more tests. I will try to think what other tests that I can write. Thanks, PTAL.
On 2017/04/27 21:08:24, xunjieli wrote: > I am nervous about this refactor, so + Cherie and Tarun to get some extra > scrutiny on this change. > I added a few more tests. I will try to think what other tests that I can write. > Thanks, PTAL. QQ: If you are nervous about the change, then one possibility is to do this migration in steps using some sort of A/B testing (field trial groups). I am not sure about the implementation difficulty of that and probably the code will become more messy in the intermediate stage. However, it will probably help us catch any regressions. Thoughts?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2814633003/diff/300001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2814633003/diff/300001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1317: client_maker_.MakeInitialSettingsPacket(1, nullptr)); On 2017/04/27 18:15:54, xunjieli wrote: > Ryan: so I tried your suggestion. However, I think this pulls too many Quic > dependencies. It's hard to write test expectations. For example, in fixing this > test, I have to match the exact settings packet that is sent out when crypto > handshake is confirmed, which is really hard to match. This test doesn't know > the connection_id, for example. > > The expected packet is: > > Client: Appending header: { connection_id: 4560497761450018688, > connection_id_length: 8, packet_number_length: 1, reset_flag: 0, version_flag: > 1, version:, packet_number: 1 } > > Currently I have: > Client: Appending header: { connection_id: 0, connection_id_length: 8, > packet_number_length: 1, reset_flag: 0, version_flag: 1, version:, > packet_number: 1 } The connection ID is generated "randomly" each time from a QuicRandom object. Thankfully, we have a MockRandon you can use, as we do in QuicNetworkTransactionTest: https://cs.chromium.org/chromium/src/net/quic/chromium/quic_network_transacti... If you use the MockRandom, this should "just work". There might be a couple of other things we need to tweak that I'm not thinking about, but it should be doable. I'm at Take Your Child To Work Day today, but I'd be happy to sit down together tomorrow and get to the bottom of it.
Forgot to publish these two comments quite a few days ago, sorry for the delay. https://codereview.chromium.org/2814633003/diff/360001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2814633003/diff/360001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:735: int rv = result; I think it is okay to call the function argument rv and use that throughout the function. I mean, I know it's okay in terms of the language rules (as long as it is passed by value), and I believe it is not against the style guide either. https://codereview.chromium.org/2814633003/diff/360001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2814633003/diff/360001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:150: // A mock QuicStreamRequest that returns |net_error|. I was thinking about "A factory to create a MockQuicStreamRequest." or something similar. (Because this class is not a mock QuicStreamRequest.)
Thank you! With Ryan's help, I was able to use MockQuicData to migrate affected JobController tests. I think they are passing now. PTAL. > QQ: If you are nervous about the change, then one possibility is to do this > migration in steps using some sort of A/B testing (field trial groups). > I am not sure about the implementation difficulty of that and > probably the code will become more messy in the intermediate stage. > However, it will probably help us catch any regressions. Thoughts? I think keeping two proxy code paths at the same time will be extremely difficult and will introduce a lot of subtlety. I think it's not necessary safer than doing the refactor at one-go with a close watch of the metrics. https://codereview.chromium.org/2814633003/diff/360001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2814633003/diff/360001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:735: int rv = result; On 2017/05/03 14:11:10, Bence wrote: > I think it is okay to call the function argument rv and use that throughout the > function. I mean, I know it's okay in terms of the language rules (as long as > it is passed by value), and I believe it is not against the style guide either. Done. https://codereview.chromium.org/2814633003/diff/360001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2814633003/diff/360001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:150: // A mock QuicStreamRequest that returns |net_error|. On 2017/05/03 14:11:11, Bence wrote: > I was thinking about "A factory to create a MockQuicStreamRequest." or something > similar. (Because this class is not a mock QuicStreamRequest.) Acknowledged. With Ryan's help, I am able to use MockQuicData which is more consistent with other quic tests (e.g. QuicNetworkTransactionTest).
The CQ bit was checked by xunjieli@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
A friendly ping to all my reviewers! This CL is ready for review.
rch@chromium.org changed required reviewers: - rch@chromium.org
This looks pretty good to me. I have a few nits, but I'm happy to defer to others for a more thorough review. Thanks for working through the and getting to the bottom of the QUIC test setup! https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:38: job_factory_(new JobFactory()), Well that's a nice cleanup! I wonder what DefaultJobFactory was created for... https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:794: } else if (using_quic_ && nit: no else after return. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:446: bool reconsider_proxy_; nit: Consider "should_reconsider_proxy_" https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:497: virtual HttpStreamFactoryImpl::Job* CreateJob( nit: There are quite a few arguments in each of these 3 methods which makes it a bit non-obvious to tell the difference. Could you add short comments before each which describes how they're different? Or even better, consider renaming them to be a bit more descriptive? CreateMainJob CreateAlternativeJob CreateAlternativeProxyJob (Though maybe that'd be better in cleanup/followup) https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.h (right): https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:68: // Called when we encounter a network error that could be resolved by trying nit: avoid first person in comments. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:196: // STATE_CREATE_JOBS_COMPLETE, Should this be removed? https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:159: client_maker_(QUIC_VERSION_37, maybe we can get this from the default QUIC version in HttpNetworkSession::Params?
Thanks for cleaning this up. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (left): https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:152: std::unique_ptr<base::Value> NetLogHttpStreamJobProxyServerResolved( Is it possible to keep this here? This adds the proxy server used by a given job to the net log of this Job. When 2 proxy jobs are racing (e.g., for data saver), this was useful for determining which job is the H2 proxy job, and which one is QUIC proxy job. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:1382: if (job_type() == ALTERNATIVE && alternative_proxy_server().is_valid()) { why is the check for job_type needed? https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:156: ProxyInfo proxy_info, pass as const ref? here and in the ctor below. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:503: ProxyInfo proxy_info, nit: pass as const ref? here and below. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:33: // Returns parameters associated with the proxy resolution. nit: linebreak after namespace { https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:205: int HttpStreamFactoryImpl::JobController::ReconsiderProxyAfterError(Job* job, Consider passing |job| as const ref. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:801: GetAlternativeServiceFor(request_info_, delegate_, stream_type_); Well, now since the proxy resolution is done ahead of job creation, it is possible to do a further optimization. If |proxy_info_| points to a QUIC proxy, and the URL is an https url, then there is no need to create the alt job? https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:823: main_job_->Preconnect(num_streams_); may be return OK here, and remove the else below to reduce the nesting. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.h (right): https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:70: // then this method sets next_state_ appropriately and returns either OK or s/next_state_/|next_state_|/ https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:74: int ReconsiderProxyAfterError(Job* job, int error); Can this be a private method? https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:346: const NetLogWithSource net_log_; nit: |net_log_| should be the last entry (before WeakPtr factory).
Looking good, that's a huge cleanup! Excited to see the proxy resolution being extracted! I haven't looked at the unittests yet. I will take a look in the next iteration. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:38: job_factory_(new JobFactory()), On 2017/05/09 19:22:06, Ryan Hamilton wrote: > Well that's a nice cleanup! I wonder what DefaultJobFactory was created for... Ah, JobFactory used to be an interface, and production code uses DefaultJobFactory while unittests uses TestJobFactory. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:337: OnAlternativeProxyJobFailed(status); an alternative_job_ will be set if an alternative service or an alternative proxy server is available. Calling both methods here seems a little confusing. How about rename the OnAlternativeJobFailed to OnAlternativeServiceJobFailed and if (alternative_job_->alternative_proxy_server().is_valid()) { OnAlternativeProxyJobFailed(status); } else { OnAlternativeServiceJobFailed(status); } https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:782: NetLogEventType::HTTP_STREAM_JOB_PROXY_SERVER_RESOLVED, optional nit: this will add the log entry to JOB_CONTROLLER, might consider to rename as HTTP_STREAM_JOB_CONTROLLER_PROXY_SERVER_RESOLVED. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:801: GetAlternativeServiceFor(request_info_, delegate_, stream_type_); nit: this alt svc is only used in non-preconnect case, shall we move this to non-preconnect specific code block so that it's more clear? https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:828: enable_ip_based_pooling_, net_log_.net_log())); optional nit: you might consider move the creation of main job to line #877, which will make it more clear that the main job starts after alt job starts. NetLogs will also log creation of alt job first, and then main job. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:850: } Could you add some comments here explaining how we set the alternative job? Basically we set a alt job if alt svc is found for the origin. Otherwise we check whether a alt proxy server could be started. That being said, alt svc is preferred over alt proxy server, right? https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:866: can_start_alternative_proxy_job_ = false; Since we explicitly checks whether |alternative_job_| has been set before query ShouldCreateAlternativeProxyServerJob(), shall we get rid of |can_start_alternative_proxy_job_|? https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:872: ptr_factory_.GetWeakPtr())); Ummm, this might be a tbansal@ question: we used to post a task to start the alternative proxy server job because we create the alt proxy job until the main job starts and resolves the proxy. With your change, I think in this case we can simply start the alt proxy job immediately? https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:159: client_maker_(QUIC_VERSION_37, On 2017/05/09 19:22:07, Ryan Hamilton wrote: > maybe we can get this from the default QUIC version in > HttpNetworkSession::Params? +1.
https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:212: May be add: DCHECK(!job->alternative_proxy_server().is_valid()); https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:233: alternative_job_.reset(); IIUC, this is a functional change? Now, if the main job fails to connect to the first proxy, we kill the alternative job too? I think it's okay to do this. Just wanted to confirm. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:850: } On 2017/05/09 20:59:30, Zhongyi Shi wrote: > Could you add some comments here explaining how we set the alternative job? > Basically we set a alt job if alt svc is found for the origin. Otherwise we > check whether a alt proxy server could be started. That being said, alt svc is > preferred over alt proxy server, right? Right now, the decision is mutually exclusive. The alt svc can be set only for HTTPS requests. alt proxy server can only be set for HTTP requests. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:866: can_start_alternative_proxy_job_ = false; On 2017/05/09 20:59:30, Zhongyi Shi wrote: > Since we explicitly checks whether |alternative_job_| has been set before query > ShouldCreateAlternativeProxyServerJob(), shall we get rid of > |can_start_alternative_proxy_job_|? I think one of the purpose of |can_start_alternative_proxy_job_| was to make sure that alt proxy server job is started at most once per JobController (or URL request). I think we can revisit later if we want to keep that optimization. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:872: ptr_factory_.GetWeakPtr())); On 2017/05/09 20:59:30, Zhongyi Shi wrote: > Ummm, this might be a tbansal@ question: > we used to post a task to start the alternative proxy server job because we > create the alt proxy job until the main job starts and resolves the proxy. With > your change, I think in this case we can simply start the alt proxy job > immediately? Yup, I think this PostTask can be removed. With that, we can also get rid of the weak ptr factory.
Thanks everyone for the helpful comments! I believe I have addressed all. PTAL. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:38: job_factory_(new JobFactory()), On 2017/05/09 20:59:30, Zhongyi Shi wrote: > On 2017/05/09 19:22:06, Ryan Hamilton wrote: > > Well that's a nice cleanup! I wonder what DefaultJobFactory was created for... > > > Ah, JobFactory used to be an interface, and production code uses > DefaultJobFactory while unittests uses TestJobFactory. Cherie is right. The interface seems to be created for testing purpose. I am getting rid of it because I need a real JobFactory instead of TestJobFactory for JobControllerReconsiderProxyAfterErrorTest. The DefaultJobFactory was a private class. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (left): https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:152: std::unique_ptr<base::Value> NetLogHttpStreamJobProxyServerResolved( On 2017/05/09 20:38:48, tbansal1 wrote: > Is it possible to keep this here? This adds the proxy server used by a given job > to the net log of this Job. When 2 proxy jobs are racing (e.g., for data saver), > this was useful for determining which job is the H2 proxy job, and which one is > QUIC proxy job. I moved this to http_stream_factory_impl_job_controller.cc. Will that be enough? https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:794: } else if (using_quic_ && On 2017/05/09 19:22:06, Ryan Hamilton wrote: > nit: no else after return. Done. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:1382: if (job_type() == ALTERNATIVE && alternative_proxy_server().is_valid()) { On 2017/05/09 20:38:48, tbansal1 wrote: > why is the check for job_type needed? Done. Removed. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:156: ProxyInfo proxy_info, On 2017/05/09 20:38:48, tbansal1 wrote: > pass as const ref? > here and in the ctor below. Done. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:446: bool reconsider_proxy_; On 2017/05/09 19:22:06, Ryan Hamilton wrote: > nit: Consider "should_reconsider_proxy_" Done. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:497: virtual HttpStreamFactoryImpl::Job* CreateJob( On 2017/05/09 19:22:06, Ryan Hamilton wrote: > nit: There are quite a few arguments in each of these 3 methods which makes it a > bit non-obvious to tell the difference. Could you add short comments before each > which describes how they're different? > > Or even better, consider renaming them to be a bit more descriptive? > > CreateMainJob > CreateAlternativeJob > CreateAlternativeProxyJob > > (Though maybe that'd be better in cleanup/followup) Done. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:503: ProxyInfo proxy_info, On 2017/05/09 20:38:48, tbansal1 wrote: > nit: pass as const ref? here and below. Done. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:33: // Returns parameters associated with the proxy resolution. On 2017/05/09 20:38:48, tbansal1 wrote: > nit: linebreak after namespace { Done. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:205: int HttpStreamFactoryImpl::JobController::ReconsiderProxyAfterError(Job* job, On 2017/05/09 20:38:48, tbansal1 wrote: > Consider passing |job| as const ref. Acknowledged. I will keep it as a raw ptr because the job was passed in through OnStreamFailed() which takes a raw Job*. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:212: On 2017/05/09 21:16:48, tbansal1 wrote: > May be add: > DCHECK(!job->alternative_proxy_server().is_valid()); Done. Good idea. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:233: alternative_job_.reset(); On 2017/05/09 21:16:48, tbansal1 wrote: > IIUC, this is a functional change? Now, if the main job fails to connect to the > first proxy, we kill the alternative job too? > > I think it's okay to do this. Just wanted to confirm. No there is no functional change. This method is only invoked in OnStreamFailed() if there isn't a spare job. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:337: OnAlternativeProxyJobFailed(status); On 2017/05/09 20:59:30, Zhongyi Shi wrote: > an alternative_job_ will be set if an alternative service or an alternative > proxy server is available. > > Calling both methods here seems a little confusing. How about rename the > OnAlternativeJobFailed to OnAlternativeServiceJobFailed and > if (alternative_job_->alternative_proxy_server().is_valid()) { > OnAlternativeProxyJobFailed(status); > } else { > OnAlternativeServiceJobFailed(status); > } Done. Good idea! https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:782: NetLogEventType::HTTP_STREAM_JOB_PROXY_SERVER_RESOLVED, On 2017/05/09 20:59:30, Zhongyi Shi wrote: > optional nit: this will add the log entry to JOB_CONTROLLER, might consider to > rename as HTTP_STREAM_JOB_CONTROLLER_PROXY_SERVER_RESOLVED. Acknowledged. Good idea. I will make sure to do that in a follow-up :) https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:801: GetAlternativeServiceFor(request_info_, delegate_, stream_type_); On 2017/05/09 20:59:30, Zhongyi Shi wrote: > nit: this alt svc is only used in non-preconnect case, shall we move this to > non-preconnect specific code block so that it's more clear? Alt svc is passed to the preconnect case as well. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:801: GetAlternativeServiceFor(request_info_, delegate_, stream_type_); On 2017/05/09 20:38:48, tbansal1 wrote: > Well, now since the proxy resolution is done ahead of job creation, it is > possible to do a further optimization. If |proxy_info_| points > to a QUIC proxy, and the URL is an https url, then there is no need to create > the alt job? I don't quite understand. Are you referring to the main job? If we know proxy_info_ points to a QUIC proxy, are you suggesting that we only create alt proxy job and not main job? https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:823: main_job_->Preconnect(num_streams_); On 2017/05/09 20:38:48, tbansal1 wrote: > may be return OK here, and remove the else below to reduce the nesting. Done. Good idea. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:828: enable_ip_based_pooling_, net_log_.net_log())); On 2017/05/09 20:59:30, Zhongyi Shi wrote: > optional nit: you might consider move the creation of main job to line #877, > which will make it more clear that the main job starts after alt job starts. > NetLogs will also log creation of alt job first, and then main job. Done. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:850: } On 2017/05/09 20:59:30, Zhongyi Shi wrote: > Could you add some comments here explaining how we set the alternative job? > Basically we set a alt job if alt svc is found for the origin. Otherwise we > check whether a alt proxy server could be started. That being said, alt svc is > preferred over alt proxy server, right? Done. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:850: } On 2017/05/09 21:16:48, tbansal1 wrote: > On 2017/05/09 20:59:30, Zhongyi Shi wrote: > > Could you add some comments here explaining how we set the alternative job? > > Basically we set a alt job if alt svc is found for the origin. Otherwise we > > check whether a alt proxy server could be started. That being said, alt svc is > > preferred over alt proxy server, right? > Right now, the decision is mutually exclusive. The alt svc can be set only for > HTTPS requests. alt proxy server can only be set for HTTP requests. > Acknowledged. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:866: can_start_alternative_proxy_job_ = false; On 2017/05/09 20:59:30, Zhongyi Shi wrote: > Since we explicitly checks whether |alternative_job_| has been set before query > ShouldCreateAlternativeProxyServerJob(), shall we get rid of > |can_start_alternative_proxy_job_|? we can't get rid of |can_start_alternative_proxy_job_| because we want to make sure that alt proxy server is started once per JobController. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:866: can_start_alternative_proxy_job_ = false; On 2017/05/09 21:16:48, tbansal1 wrote: > On 2017/05/09 20:59:30, Zhongyi Shi wrote: > > Since we explicitly checks whether |alternative_job_| has been set before > query > > ShouldCreateAlternativeProxyServerJob(), shall we get rid of > > |can_start_alternative_proxy_job_|? > > I think one of the purpose of |can_start_alternative_proxy_job_| was to make > sure that alt proxy server job is started at most once per JobController (or URL > request). I think we can revisit later if we want to keep that optimization. Acknowledged. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:872: ptr_factory_.GetWeakPtr())); On 2017/05/09 20:59:30, Zhongyi Shi wrote: > Ummm, this might be a tbansal@ question: > we used to post a task to start the alternative proxy server job because we > create the alt proxy job until the main job starts and resolves the proxy. With > your change, I think in this case we can simply start the alt proxy job > immediately? Done. Good point, Cherie! https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:872: ptr_factory_.GetWeakPtr())); On 2017/05/09 21:16:48, tbansal1 wrote: > On 2017/05/09 20:59:30, Zhongyi Shi wrote: > > Ummm, this might be a tbansal@ question: > > we used to post a task to start the alternative proxy server job because we > > create the alt proxy job until the main job starts and resolves the proxy. > With > > your change, I think in this case we can simply start the alt proxy job > > immediately? > > Yup, I think this PostTask can be removed. With that, we can also get rid of the > weak ptr factory. Done. I can't get rid of weak ptr factory, because it's used to resume main job. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.h (right): https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:68: // Called when we encounter a network error that could be resolved by trying On 2017/05/09 19:22:07, Ryan Hamilton wrote: > nit: avoid first person in comments. Done. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:70: // then this method sets next_state_ appropriately and returns either OK or On 2017/05/09 20:38:48, tbansal1 wrote: > s/next_state_/|next_state_|/ Done. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:74: int ReconsiderProxyAfterError(Job* job, int error); On 2017/05/09 20:38:48, tbansal1 wrote: > Can this be a private method? Done. Good point! https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:196: // STATE_CREATE_JOBS_COMPLETE, On 2017/05/09 19:22:06, Ryan Hamilton wrote: > Should this be removed? Done. Ah, Good catch! https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.h:346: const NetLogWithSource net_log_; On 2017/05/09 20:38:48, tbansal1 wrote: > nit: |net_log_| should be the last entry (before WeakPtr factory). Done. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:159: client_maker_(QUIC_VERSION_37, On 2017/05/09 20:59:30, Zhongyi Shi wrote: > On 2017/05/09 19:22:07, Ryan Hamilton wrote: > > maybe we can get this from the default QUIC version in > > HttpNetworkSession::Params? > > +1. Done. https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:159: client_maker_(QUIC_VERSION_37, On 2017/05/09 19:22:07, Ryan Hamilton wrote: > maybe we can get this from the default QUIC version in > HttpNetworkSession::Params? Done.
The CQ bit was checked by xunjieli@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_...)
The CQ bit was checked by xunjieli@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/2814633003/diff/510001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2814633003/diff/510001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:792: enable_ip_based_pooling_, net_log_.net_log())); Cherie, looks like I need to keep the original creation order (main before alt). There is some subtle interaction in main job resumption logic that will break if I move this later (See PS#14 dry run). Didn't dig deeper. Let's address this in a follow-up? We probably can simplify the resumption logic as well given that we start the alt job right away (it doesn't need to go through proxy resolution). WDYT?
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...)
Looks like I have three Alt Proxy tests failing because I removed the extra PostTask. I am working on updating the tests.
The CQ bit was checked by xunjieli@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...
On 2017/05/10 14:44:26, xunjieli wrote: > Looks like I have three Alt Proxy tests failing because I removed the extra > PostTask. I am working on updating the tests. Done. Tests are all passing. This is ready for another round of review. Thank you!
On 2017/05/10 18:13:20, xunjieli wrote: > On 2017/05/10 14:44:26, xunjieli wrote: > > Looks like I have three Alt Proxy tests failing because I removed the extra > > PostTask. I am working on updating the tests. > > Done. Tests are all passing. This is ready for another round of review. Thank > you! LGTM. Thank you, this is amazing!
I think you are almost there, thanks for the patience! This is one of the most complicated areas on the stack, thanks Helen for taking efforts to simplify and optimize the code! https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2814633003/diff/420002/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:801: GetAlternativeServiceFor(request_info_, delegate_, stream_type_); On 2017/05/10 00:26:24, xunjieli wrote: > On 2017/05/09 20:59:30, Zhongyi Shi wrote: > > nit: this alt svc is only used in non-preconnect case, shall we move this to > > non-preconnect specific code block so that it's more clear? > > Alt svc is passed to the preconnect case as well. Acknowledged. *facepalm* I missed the preconnect case. https://codereview.chromium.org/2814633003/diff/530001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/2814633003/diff/530001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:258: // waits after it finishes proxy resolution. The alternative job never Could you also update the comments since the proxy resolution is moved out of Job? https://codereview.chromium.org/2814633003/diff/530001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:511: virtual HttpStreamFactoryImpl::Job* CreateAltJob( optional nit: how about CreateAltSvcJob? https://codereview.chromium.org/2814633003/diff/530001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2814633003/diff/530001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:688: DCHECK(!alternative_job_); Could you add some comments here? I think this is specifically handling the case where we fail before CREATE_JOBS, i.e., in proxy resolution, job controller need to notify the request of the failure. https://codereview.chromium.org/2814633003/diff/530001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:775: proxy_info_.RemoveProxiesWithoutScheme(supported_proxies); optional nit: this is trying to clean up proxies from the list, which should happen in successful DoResolveProxyComplete. It might be better to move this up to DoResolveProxyComplete(). https://codereview.chromium.org/2814633003/diff/530001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:776: Also, we used to check the proxy_info list is non-empty in HttpStreamFactoryImpl::Job::DoResolveProxy(). if (proxy_info_.is_empty()) { // No proxies/direct to choose from. This happens when we don't support // any of the proxies in the returned list. result = ERR_NO_SUPPORTED_PROXIES; } In stead of delaying the check in HttpStreamFactoryImpl::Job::DoInitConnectionImpl() as this patch does, how about return the error in HttpStreamFactoryImpl::JobController::DoResolveProxyComplete() and mark the request fail immediately? Does this logic make sense? https://codereview.chromium.org/2814633003/diff/530001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:782: main_job_.reset(job_factory_->CreateAltJob( Hrm, that suggests s/CreateAltJob/CreateAltSvcJob, WDYT? https://codereview.chromium.org/2814633003/diff/530001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:830: alternative_job_->Start(request_->stream_type()); Woohoo! This is much cleaner! https://codereview.chromium.org/2814633003/diff/530001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2814633003/diff/530001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:309: .Times(1); Woohoo! Nice! https://codereview.chromium.org/2814633003/diff/530001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:353: } These two tests all looks good to me. How about one more test that removes all the proxies in the list so that we have a test coverage for: if (proxy_info_.is_empty()) { // No proxies/direct to choose from. This happens when we don't support // any of the proxies in the returned list. return ERR_NO_SUPPORTED_PROXIES; } Or do you already have one which covers this case? The reconsider proxy after error tests looks complicated, and I am trying to get to the bottom of that.
lgtm % nit comment. Thanks a lot for cleaning this up. https://codereview.chromium.org/2814633003/diff/530001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/2814633003/diff/530001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:503: ProxyInfo proxy_info, pass as const ref. Same for other ctors below.
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...)
The CQ bit was checked by xunjieli@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...
Thanks everyone! PTAL. https://codereview.chromium.org/2814633003/diff/530001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/2814633003/diff/530001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:258: // waits after it finishes proxy resolution. The alternative job never On 2017/05/10 20:28:15, Zhongyi Shi wrote: > Could you also update the comments since the proxy resolution is moved out of > Job? Done. Good point! https://codereview.chromium.org/2814633003/diff/530001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:503: ProxyInfo proxy_info, On 2017/05/10 21:53:20, tbansal1 wrote: > pass as const ref. Same for other ctors below. Done. Good point! https://codereview.chromium.org/2814633003/diff/530001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:511: virtual HttpStreamFactoryImpl::Job* CreateAltJob( On 2017/05/10 20:28:15, Zhongyi Shi wrote: > optional nit: how about CreateAltSvcJob? Done. https://codereview.chromium.org/2814633003/diff/530001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2814633003/diff/530001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:688: DCHECK(!alternative_job_); On 2017/05/10 20:28:16, Zhongyi Shi wrote: > Could you add some comments here? > > I think this is specifically handling the case where we fail before CREATE_JOBS, > i.e., in proxy resolution, job controller need to notify the request of the > failure. Done. https://codereview.chromium.org/2814633003/diff/530001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:775: proxy_info_.RemoveProxiesWithoutScheme(supported_proxies); On 2017/05/10 20:28:16, Zhongyi Shi wrote: > optional nit: this is trying to clean up proxies from the list, which should > happen in successful DoResolveProxyComplete. It might be better to move this up > to DoResolveProxyComplete(). Done. https://codereview.chromium.org/2814633003/diff/530001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:782: main_job_.reset(job_factory_->CreateAltJob( On 2017/05/10 20:28:16, Zhongyi Shi wrote: > Hrm, that suggests s/CreateAltJob/CreateAltSvcJob, WDYT? Done. https://codereview.chromium.org/2814633003/diff/530001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2814633003/diff/530001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:353: } On 2017/05/10 20:28:16, Zhongyi Shi wrote: > These two tests all looks good to me. How about one more test that removes all > the proxies in the list so that we have a test coverage for: > > if (proxy_info_.is_empty()) { > // No proxies/direct to choose from. This happens when we don't support > // any of the proxies in the returned list. > return ERR_NO_SUPPORTED_PROXIES; > } > > Or do you already have one which covers this case? The reconsider proxy after > error tests looks complicated, and I am trying to get to the bottom of that. Done. Good idea! The code path isn't tested.
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_...)
The CQ bit was checked by xunjieli@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 xunjieli@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by xunjieli@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 xunjieli@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...
LGTM! Thanks a lot for your patience!
Thanks everyone for the reviews. Ryan: do you want to take another look?
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...)
On 2017/05/11 15:25:33, xunjieli wrote: > Thanks everyone for the reviews. > Ryan: do you want to take another look? Nope, LGTM. (but I thought I removed myself as a reviewer)
The CQ bit was checked by xunjieli@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 xunjieli@chromium.org to run a CQ dry run
Patchset #9 (id:400001) 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 #21 (id:650001) has been deleted
Patchset #26 (id:770001) has been deleted
Patchset #26 (id:780001) has been deleted
Patchset #22 (id:690001) has been deleted
Patchset #25 (id:800001) has been deleted
Patchset #5 (id:320001) has been deleted
There's some issue with Network Predictor which passes empty urls to HttpStreamFactoryImpl::PreconnectStreams() on Android. I filed crbug.com/721981. I will fix that one first and then land this.
Patchset #21 (id:710001) has been deleted
The CQ bit was checked by xunjieli@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: This issue passed the CQ dry run.
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bnc@chromium.org, tbansal@chromium.org, rch@chromium.org, zhongyi@chromium.org Link to the patchset: https://codereview.chromium.org/2814633003/#ps870013 (title: "rebased")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 870013, "attempt_start_ts": 1494876733169060,
"parent_rev": "69d6a2deaa51e566d61bc5c77bce43b9ce9bb0d7", "commit_rev":
"c9a993789f4732a105d5ede4f9f36e0f7e0ce9b9"}
Message was sent while issue was closed.
Description was changed from ========== Extract Proxy Resolution out of HttpStreamFactoryImpl::Job This is to extract proxy resolution out of HttpStreamFactoryImpl::Job to simplify the logic in the Job class. A side benefit is that we will have fewer proxy resolutions that we need to do. See linked doc for more details. Design doc: https://docs.google.com/document/d/1xKeUygQRvYXeg55Jt_f0LYHAZsJ98O10uQPLmtmNG... BUG=475060 ========== to ========== Extract Proxy Resolution out of HttpStreamFactoryImpl::Job This is to extract proxy resolution out of HttpStreamFactoryImpl::Job to simplify the logic in the Job class. A side benefit is that we will have fewer proxy resolutions that we need to do. See linked doc for more details. Design doc: https://docs.google.com/document/d/1xKeUygQRvYXeg55Jt_f0LYHAZsJ98O10uQPLmtmNG... BUG=475060 Review-Url: https://codereview.chromium.org/2814633003 Cr-Commit-Position: refs/heads/master@{#471899} Committed: https://chromium.googlesource.com/chromium/src/+/c9a993789f4732a105d5ede4f9f3... ==========
Message was sent while issue was closed.
Committed patchset #27 (id:870013) as https://chromium.googlesource.com/chromium/src/+/c9a993789f4732a105d5ede4f9f3... |
