Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(58)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 2595413002: Race preconnects to HTTP2 proxies that support alternate proxies

Created:
4 years ago by tbansal1
Modified:
3 years, 9 months ago
Reviewers:
CC:
chromium-reviews, cbentzel+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Race preconnects to HTTP2 proxies that support alternate proxies If the proxy supports an alternative proxy server, then an alternative job is started after proxy resolution. The job controller is destroyed after both the jobs finish. The new behavior is guarded behind a field trial param. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=671291

Patch Set 1 : ps #

Total comments: 11

Patch Set 2 : Rebased, rch comments #

Total comments: 7
Unified diffs Side-by-side diffs Delta from patch set Stats (+344 lines, -24 lines) Patch
M components/network_session_configurator/network_session_configurator.cc View 1 1 chunk +3 lines, -0 lines 0 comments Download
M net/http/http_network_session.h View 1 1 chunk +3 lines, -0 lines 1 comment Download
M net/http/http_network_session.cc View 1 1 chunk +2 lines, -1 line 0 comments Download
M net/http/http_stream_factory_impl_job.h View 1 1 chunk +3 lines, -0 lines 0 comments Download
M net/http/http_stream_factory_impl_job.cc View 1 3 chunks +7 lines, -2 lines 0 comments Download
M net/http/http_stream_factory_impl_job_controller.cc View 1 6 chunks +56 lines, -18 lines 6 comments Download
M net/http/http_stream_factory_impl_unittest.cc View 1 7 chunks +110 lines, -3 lines 0 comments Download
M net/quic/chromium/quic_network_transaction_unittest.cc View 1 3 chunks +152 lines, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 1 chunk +8 lines, -0 lines 0 comments Download

Messages

Total messages: 72 (67 generated)
tbansal1
rch: ptal at PS# 2. Thanks.
3 years, 11 months ago (2016-12-29 18:46:14 UTC) #60
tbansal1
rch: ping. thanks.
3 years, 11 months ago (2017-01-12 20:03:40 UTC) #63
Ryan Hamilton
Sorry for the delay :( https://codereview.chromium.org/2595413002/diff/280001/net/http/http_network_session.h File net/http/http_network_session.h (right): https://codereview.chromium.org/2595413002/diff/280001/net/http/http_network_session.h#newcode212 net/http/http_network_session.h:212: bool race_preconnects_to_proxies; The comment ...
3 years, 11 months ago (2017-01-23 18:05:54 UTC) #64
tbansal1
rch: ptal. thanks. https://codereview.chromium.org/2595413002/diff/280001/net/http/http_network_session.h File net/http/http_network_session.h (right): https://codereview.chromium.org/2595413002/diff/280001/net/http/http_network_session.h#newcode212 net/http/http_network_session.h:212: bool race_preconnects_to_proxies; On 2017/01/23 18:05:53, Ryan ...
3 years, 11 months ago (2017-01-23 21:48:23 UTC) #67
Ryan Hamilton
3 years, 11 months ago (2017-01-24 19:36:59 UTC) #71 
https://codereview.chromium.org/2595413002/diff/280001/net/http/http_network_...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/2595413002/diff/280001/net/http/http_network_...
net/http/http_network_session.h:212: bool race_preconnects_to_proxies;
On 2017/01/23 21:48:22, tbansal1 wrote:
> On 2017/01/23 18:05:53, Ryan Hamilton wrote:
> > The comment says this is only for HTTP2 proxies, but the member name does
not
> > mention http2. 
> Done.
> > I suspect (and hope) that this also applies to QUIC proxies...
> Did you mean a QUIC proxy explicitly specified as quic://myproxy...
> Then, answer is no.

Hm. Seems like it should work for QUIC proxies too, shouldn't it?

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

https://codereview.chromium.org/2595413002/diff/280001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller.cc:401:
DCHECK(alternative_proxy_server.is_quic());
On 2017/01/23 21:48:22, tbansal1 wrote:
> On 2017/01/23 18:05:53, Ryan Hamilton wrote:
> > Why is this guaranteed to be true?
> 
> Removed. (Currently it is true because QUIC is the only alternative service).

We also support HTTP/2 based Alt-Svc.

https://codereview.chromium.org/2595413002/diff/280001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller.cc:405:
job->RestrictNumStreamsToOne();
On 2017/01/23 21:48:22, tbansal1 wrote:
> On 2017/01/23 18:05:53, Ryan Hamilton wrote:
> > It seems like this is a bit orthogonal to racing alt-proxies, isn't it?
> 
> The preconnections for non-proxy http2 jobs restrict the number of preconnect
> streams to 1.
>
https://cs.chromium.org/chromium/src/net/http/http_stream_factory_impl_job.cc...
> 
> This CL enables preconnects for proxy http2 jobs. So, we should also restrict
> the preconnect streams to 1 for proxy http2/quic jobs.

Ah, I see. You're saying that "restrict to 1 stream" is inherently part of
preconnect, and since this CL implements a new preconnect mechanism, we need to
do the same thing. Did I get that right?

> I can do it in a separate CL too, but then I would need to plumb |num_streams|
> from the Job to
> HttpStreamFactoryImpl::JobController::StartAlternativeProxyServerJob() method
> below.

https://codereview.chromium.org/2595413002/diff/300001/net/http/http_network_...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/2595413002/diff/300001/net/http/http_network_...
net/http/http_network_session.h:308: void DisableQuic();
nit: it's best to upload Rebase patch sets independently from functional changes
so that it's easier for reviewers to compare two patch sets and see just what
changed. Not a big deal of course, but it's handy.

https://codereview.chromium.org/2595413002/diff/300001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job_controller.cc (left):

https://codereview.chromium.org/2595413002/diff/300001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller.cc:1043: return false;
you could probably add a DCHECK here checking the param which enable preconnects
to alt proxies.

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

https://codereview.chromium.org/2595413002/diff/300001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller.cc:402:
DCHECK_EQ(main_job_.get(), job);
The previous line suggest that this job might not be the main job if
is_preconnect_ is true. If that's the case, isn't this DCHECK invalid? Or
alternatively, should is_preconnect_ be removed from the first DCHECK? (Or more
likely than either, am I confused?)

https://codereview.chromium.org/2595413002/diff/300001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller.cc:409: }
So this restricts the number of streams on the main job, right? But if we're
doing a preconnect for the main job, wouldn't we have started it by calling
Preconnect() which sets num_streams to 1?

https://codereview.chromium.org/2595413002/diff/300001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller.cc:426: main_job_is_blocked_ =
true;
I believe this all means that the main job will now pause and give the alt-job a
head start. (Which is desirable). Can you make sure we have a test of this case?
(Where we don't send any traffic to the HTTP/2 proxy if the QUIC proxy can do
0-RTT.)

https://codereview.chromium.org/2595413002/diff/300001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller.cc:508:
"Net.QuicAlternativeProxy.PreconnectRace.FirstJobFinished", 1, 2);
I'm not sure "FirstJobFinished" adds any value to the name of the histograms.
I'd probably remove it.

Also, you might consider making the histogram's value be true if the main job
finished first, and false otherwise (or use an enum). This will help us
understand what's going on better.

https://codereview.chromium.org/2595413002/diff/300001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller.cc:1154: }
nit: I'd structure this slightly differently:

if (is_preconnect) {
 ... Preconnect();
 return;
}

if (!request_)
  return;

...

This avoid a bit of nesting which is always nice.

Powered by Google App Engine
This is Rietveld 408576698