Issue 2522703002: Allow at most one preconnect to HTTP2 proxy servers

tbansal1

Description was changed from ========== Skip preconnect for HTTP requests when QUIC is in use ...

4 years, 1 month ago (2016-11-21 22:09:33 UTC) #1

tbansal1

Description was changed from ========== Skip preconnect for HTTP requests when QUIC is in use ...

4 years, 1 month ago (2016-11-21 22:13:24 UTC) #2

tbansal1

tbansal@chromium.org changed reviewers: + zhongyi@chromium.org

4 years, 1 month ago (2016-11-21 22:13:58 UTC) #3

Ryan Hamilton

rch@chromium.org changed reviewers: + rch@chromium.org

4 years, 1 month ago (2016-11-21 23:39:22 UTC) #6

Ryan Hamilton

FWIW, we tried disabling preconnect when QUIC is in use and didn't find it to ...

4 years, 1 month ago (2016-11-21 23:39:22 UTC) #7

tbansal1

On 2016/11/21 23:39:22, Ryan Hamilton wrote: > FWIW, we tried disabling preconnect when QUIC is ...

4 years, 1 month ago (2016-11-21 23:49:25 UTC) #8

tbansal1

On 2016/11/21 23:49:25, tbansal1 wrote: > On 2016/11/21 23:39:22, Ryan Hamilton wrote: > > FWIW, ...

4 years, 1 month ago (2016-11-21 23:53:02 UTC) #9

Ryan Hamilton

On 2016/11/21 23:53:02, tbansal1 wrote: > On 2016/11/21 23:49:25, tbansal1 wrote: > > On 2016/11/21 ...

4 years, 1 month ago (2016-11-22 18:57:31 UTC) #10

Ryan Hamilton

On 2016/11/21 23:49:25, tbansal1 wrote: > On 2016/11/21 23:39:22, Ryan Hamilton wrote: > > FWIW, ...

4 years, 1 month ago (2016-11-22 18:58:27 UTC) #11

tbansal1

On 2016/11/22 18:57:31, Ryan Hamilton wrote: > On 2016/11/21 23:53:02, tbansal1 wrote: > > On ...

4 years, 1 month ago (2016-11-22 19:00:10 UTC) #12

tbansal1

On 2016/11/22 18:58:27, Ryan Hamilton wrote: > On 2016/11/21 23:49:25, tbansal1 wrote: > > On ...

4 years, 1 month ago (2016-11-22 19:12:10 UTC) #13

On 2016/11/22 18:58:27, Ryan Hamilton wrote:
> On 2016/11/21 23:49:25, tbansal1 wrote:
> > On 2016/11/21 23:39:22, Ryan Hamilton wrote:
> > > FWIW, we tried disabling preconnect when QUIC is in use and didn't find it
> to
> > > make a difference in any metric. Do you expect this to improve
performance?
> > 
> > Currently, Chrome opens up to 32 TLS/TCP connections to data saver when QUIC
> is
> > in use:
> > 1. User navigates to http://example.com
> > 2. net predictor triggers the preconnects based on the subresources inside
> > http://example.com.
> > The number of subresources may be very high.
> > 3. Chrome opens up to 32 TLS/TCP connections to data saver proxy. This is
> > because
> > connections to proxies have a limit of 32 connections (compared to 6 for
> regular
> > web servers).
> > 4. All these 32 connections end up not being used, and compete with the
> regular
> > page
> > load which is happening on QUIC.
> > 
> > I expect to see a benefit, at least reduction in data usage. Hopefully, some
> > performance benefit too. The only case where the data saver preconnects
might
> > have a
> > benefit is when the main frame was HTTPS, while the subresources are HTTP.
> 
> I'm surprised chrome is opening 32 sockets since the proxy supports H2 which
> typically restricts the network stack into opening only a single connection. I
> wonder if it would make sense to dig deeper into this issue.

I did dig deeper. Here is the timeline of what happens:
t=0: Start page load
t=1: net predictor fires a bunch of preconnects
t-2: All these preconnects use a different socket because the SPDY connection
has 
not been established so far
t=3: SPDY connection is established.
t=4: From this point on, no extra connection will be established.

I think the problem is that the single-socket check is effective only
after the SPDY connection has been established, and that point multiple
connections have already been established. This bug is not really
specific to using QUIC though, but I am focusing on that for now.

Ryan Hamilton

On 2016/11/22 19:12:10, tbansal1 wrote: > On 2016/11/22 18:58:27, Ryan Hamilton wrote: > > On ...

4 years, 1 month ago (2016-11-22 19:33:42 UTC) #14

On 2016/11/22 19:12:10, tbansal1 wrote:
> On 2016/11/22 18:58:27, Ryan Hamilton wrote:
> > On 2016/11/21 23:49:25, tbansal1 wrote:
> > > On 2016/11/21 23:39:22, Ryan Hamilton wrote:
> > > > FWIW, we tried disabling preconnect when QUIC is in use and didn't find
it
> > to
> > > > make a difference in any metric. Do you expect this to improve
> performance?
> > > 
> > > Currently, Chrome opens up to 32 TLS/TCP connections to data saver when
QUIC
> > is
> > > in use:
> > > 1. User navigates to http://example.com
> > > 2. net predictor triggers the preconnects based on the subresources inside
> > > http://example.com.
> > > The number of subresources may be very high.
> > > 3. Chrome opens up to 32 TLS/TCP connections to data saver proxy. This is
> > > because
> > > connections to proxies have a limit of 32 connections (compared to 6 for
> > regular
> > > web servers).
> > > 4. All these 32 connections end up not being used, and compete with the
> > regular
> > > page
> > > load which is happening on QUIC.
> > > 
> > > I expect to see a benefit, at least reduction in data usage. Hopefully,
some
> > > performance benefit too. The only case where the data saver preconnects
> might
> > > have a
> > > benefit is when the main frame was HTTPS, while the subresources are HTTP.
> > 
> > I'm surprised chrome is opening 32 sockets since the proxy supports H2 which
> > typically restricts the network stack into opening only a single connection.
I
> > wonder if it would make sense to dig deeper into this issue.
> 
> I did dig deeper. Here is the timeline of what happens:
> t=0: Start page load
> t=1: net predictor fires a bunch of preconnects
> t-2: All these preconnects use a different socket because the SPDY connection
> has 
> not been established so far
> t=3: SPDY connection is established.
> t=4: From this point on, no extra connection will be established.
> 
> I think the problem is that the single-socket check is effective only
> after the SPDY connection has been established, and that point multiple
> connections have already been established. This bug is not really
> specific to using QUIC though, but I am focusing on that for now.

I agree that this is what happens, but there's a disconnect. The preconnect code
tries to open a single connection when the server supports request priorities
(SPDY or QUIC):

https://cs.chromium.org/chromium/src/net/http/http_stream_factory_impl_job.cc...

I think the proxy configuration is defeating this logic. Since the data
deduction proxy speaks both SPDY and QUIC, the intent of this code is that only
a single connection be opened. I think we should look into what it would take to
fix that code to do the right thing when there is a proxy.

tbansal1

On 2016/11/22 19:33:42, Ryan Hamilton wrote: > On 2016/11/22 19:12:10, tbansal1 wrote: > > On ...

4 years, 1 month ago (2016-11-22 19:45:49 UTC) #15

On 2016/11/22 19:33:42, Ryan Hamilton wrote:
> On 2016/11/22 19:12:10, tbansal1 wrote:
> > On 2016/11/22 18:58:27, Ryan Hamilton wrote:
> > > On 2016/11/21 23:49:25, tbansal1 wrote:
> > > > On 2016/11/21 23:39:22, Ryan Hamilton wrote:
> > > > > FWIW, we tried disabling preconnect when QUIC is in use and didn't
find
> it
> > > to
> > > > > make a difference in any metric. Do you expect this to improve
> > performance?
> > > > 
> > > > Currently, Chrome opens up to 32 TLS/TCP connections to data saver when
> QUIC
> > > is
> > > > in use:
> > > > 1. User navigates to http://example.com
> > > > 2. net predictor triggers the preconnects based on the subresources
inside
> > > > http://example.com.
> > > > The number of subresources may be very high.
> > > > 3. Chrome opens up to 32 TLS/TCP connections to data saver proxy. This
is
> > > > because
> > > > connections to proxies have a limit of 32 connections (compared to 6 for
> > > regular
> > > > web servers).
> > > > 4. All these 32 connections end up not being used, and compete with the
> > > regular
> > > > page
> > > > load which is happening on QUIC.
> > > > 
> > > > I expect to see a benefit, at least reduction in data usage. Hopefully,
> some
> > > > performance benefit too. The only case where the data saver preconnects
> > might
> > > > have a
> > > > benefit is when the main frame was HTTPS, while the subresources are
HTTP.
> > > 
> > > I'm surprised chrome is opening 32 sockets since the proxy supports H2
which
> > > typically restricts the network stack into opening only a single
connection.
> I
> > > wonder if it would make sense to dig deeper into this issue.
> > 
> > I did dig deeper. Here is the timeline of what happens:
> > t=0: Start page load
> > t=1: net predictor fires a bunch of preconnects
> > t-2: All these preconnects use a different socket because the SPDY
connection
> > has 
> > not been established so far
> > t=3: SPDY connection is established.
> > t=4: From this point on, no extra connection will be established.
> > 
> > I think the problem is that the single-socket check is effective only
> > after the SPDY connection has been established, and that point multiple
> > connections have already been established. This bug is not really
> > specific to using QUIC though, but I am focusing on that for now.
> 
> I agree that this is what happens, but there's a disconnect. The preconnect
code
> tries to open a single connection when the server supports request priorities
> (SPDY or QUIC):
> 
>
https://cs.chromium.org/chromium/src/net/http/http_stream_factory_impl_job.cc...
> 
> I think the proxy configuration is defeating this logic. Since the data
> deduction proxy speaks both SPDY and QUIC, the intent of this code is that
only
> a single connection be opened. I think we should look into what it would take
to
> fix that code to do the right thing when there is a proxy.

So, there are 2 problems:
1. Preconnect function that you referenced above looks at the HTTP server
properties
of the URL, not the proxy (because proxy has not been resolved yet).
2. That Preconnect function is called multiple times. e.g., this is what
I saw when loading cnn.com with data saver on (all the timestamps are within 1
second):
[9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
HttpStreamFactoryImpl::Job::Preconnect() num_streams=1 url=http://www.cnn.com/
[9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
HttpStreamFactoryImpl::Job::Preconnect() num_streams=3
url=http://beacon.krxd.net/
[9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
HttpStreamFactoryImpl::Job::Preconnect() num_streams=3
url=http://c.go-mpulse.net/
[9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
HttpStreamFactoryImpl::Job::Preconnect() num_streams=4 url=http://cdn.krxd.net/
[9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
HttpStreamFactoryImpl::Job::Preconnect() num_streams=3
url=http://cdn.livefyre.com/
[9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
HttpStreamFactoryImpl::Job::Preconnect() num_streams=3 url=http://data.cnn.com/
[9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
HttpStreamFactoryImpl::Job::Preconnect() num_streams=8 url=http://i.cdn.cnn.com/
[9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
HttpStreamFactoryImpl::Job::Preconnect() num_streams=3
url=http://partner.googleadservices.com/
[9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
HttpStreamFactoryImpl::Job::Preconnect() num_streams=6 url=http://www.cnn.com/
[9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
HttpStreamFactoryImpl::Job::Preconnect() num_streams=10
url=http://www.i.cdn.cnn.com/
[9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
HttpStreamFactoryImpl::Job::Preconnect() num_streams=14
url=https://logx.optimizely.com/

So, a better fix may be to let the preconnect job run, resolve the proxy, and
then stop 
(right before connection is initialized) the job if the resolved proxy supports
http server properties. wdyt?

Ryan Hamilton

On 2016/11/22 19:45:49, tbansal1 wrote: > So, there are 2 problems: > 1. Preconnect function ...

4 years, 1 month ago (2016-11-22 20:02:26 UTC) #16

On 2016/11/22 19:45:49, tbansal1 wrote:
> So, there are 2 problems:
> 1. Preconnect function that you referenced above looks at the HTTP server
> properties
> of the URL, not the proxy (because proxy has not been resolved yet).

*nod*

> 2. That Preconnect function is called multiple times. e.g., this is what
> I saw when loading http://cnn.com with data saver on (all the timestamps are
within 1
> second):
> [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> HttpStreamFactoryImpl::Job::Preconnect() num_streams=1 url=http://www.cnn.com/
> [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> HttpStreamFactoryImpl::Job::Preconnect() num_streams=3
> url=http://beacon.krxd.net/
> [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> HttpStreamFactoryImpl::Job::Preconnect() num_streams=3
> url=http://c.go-mpulse.net/
> [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> HttpStreamFactoryImpl::Job::Preconnect() num_streams=4
url=http://cdn.krxd.net/
> [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> HttpStreamFactoryImpl::Job::Preconnect() num_streams=3
> url=http://cdn.livefyre.com/
> [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> HttpStreamFactoryImpl::Job::Preconnect() num_streams=3
url=http://data.cnn.com/
> [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> HttpStreamFactoryImpl::Job::Preconnect() num_streams=8
url=http://i.cdn.cnn.com/
> [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> HttpStreamFactoryImpl::Job::Preconnect() num_streams=3
> url=http://partner.googleadservices.com/
> [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> HttpStreamFactoryImpl::Job::Preconnect() num_streams=6 url=http://www.cnn.com/
> [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> HttpStreamFactoryImpl::Job::Preconnect() num_streams=10
> url=http://www.i.cdn.cnn.com/
> [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> HttpStreamFactoryImpl::Job::Preconnect() num_streams=14
> url=https://logx.optimizely.com/

Right, even if num_streams were 1 there, that would still not help!

> So, a better fix may be to let the preconnect job run, resolve the proxy, and
> then stop 
> (right before connection is initialized) the job if the resolved proxy
supports
> http server properties. wdyt?

Yes, though I'd say we want to stop if:
1) the job is a preconnect
2) it's going through a proxy that supports request prioritization (QUIC/SPDY)
3) there's already a connection available.

Though it might be tricky to implement #3 easily so if instead we wanted a
general purpose "AllowPreconnect()" method on the proxy delegate, that might be
more straightforward. What do you think?

tbansal1

On 2016/11/22 20:02:26, Ryan Hamilton wrote: > On 2016/11/22 19:45:49, tbansal1 wrote: > > So, ...

4 years, 1 month ago (2016-11-22 20:06:09 UTC) #17

On 2016/11/22 20:02:26, Ryan Hamilton wrote:
> On 2016/11/22 19:45:49, tbansal1 wrote:
> > So, there are 2 problems:
> > 1. Preconnect function that you referenced above looks at the HTTP server
> > properties
> > of the URL, not the proxy (because proxy has not been resolved yet).
> 
> *nod*
> 
> > 2. That Preconnect function is called multiple times. e.g., this is what
> > I saw when loading http://cnn.com with data saver on (all the timestamps are
> within 1
> > second):
> > [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> > HttpStreamFactoryImpl::Job::Preconnect() num_streams=1
url=http://www.cnn.com/
> > [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> > HttpStreamFactoryImpl::Job::Preconnect() num_streams=3
> > url=http://beacon.krxd.net/
> > [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> > HttpStreamFactoryImpl::Job::Preconnect() num_streams=3
> > url=http://c.go-mpulse.net/
> > [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> > HttpStreamFactoryImpl::Job::Preconnect() num_streams=4
> url=http://cdn.krxd.net/
> > [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> > HttpStreamFactoryImpl::Job::Preconnect() num_streams=3
> > url=http://cdn.livefyre.com/
> > [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> > HttpStreamFactoryImpl::Job::Preconnect() num_streams=3
> url=http://data.cnn.com/
> > [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> > HttpStreamFactoryImpl::Job::Preconnect() num_streams=8
> url=http://i.cdn.cnn.com/
> > [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> > HttpStreamFactoryImpl::Job::Preconnect() num_streams=3
> > url=http://partner.googleadservices.com/
> > [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> > HttpStreamFactoryImpl::Job::Preconnect() num_streams=6
url=http://www.cnn.com/
> > [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> > HttpStreamFactoryImpl::Job::Preconnect() num_streams=10
> > url=http://www.i.cdn.cnn.com/
> > [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> > HttpStreamFactoryImpl::Job::Preconnect() num_streams=14
> > url=https://logx.optimizely.com/
> 
> Right, even if num_streams were 1 there, that would still not help!
> 
> > So, a better fix may be to let the preconnect job run, resolve the proxy,
and
> > then stop 
> > (right before connection is initialized) the job if the resolved proxy
> supports
> > http server properties. wdyt?
> 
> Yes, though I'd say we want to stop if:
> 1) the job is a preconnect
> 2) it's going through a proxy that supports request prioritization (QUIC/SPDY)
> 3) there's already a connection available.
> 
> Though it might be tricky to implement #3 easily so if instead we wanted a
> general purpose "AllowPreconnect()" method on the proxy delegate, that might
be
> more straightforward. What do you think?

Yup, that's almost what this CL does. Although, I use the method 
name ShouldSkipPreconnect() instead of AllowPreconnect().

Also, this CL does not implement #3 that you suggested. It simply stops
all preconnects provided #1 and #2 hold.
Reason being that waiting for the connection
to be available might be too late. In my cnn.com example,
I see that net/predictor fires off 32 connections
before the first SPDY/QUIC connection is established.
So, I do not believe that #3 should be a condition for
stopping preconnects.

Ryan Hamilton

On 2016/11/22 20:06:09, tbansal1 wrote: > On 2016/11/22 20:02:26, Ryan Hamilton wrote: > > On ...

4 years, 1 month ago (2016-11-22 20:10:49 UTC) #18

On 2016/11/22 20:06:09, tbansal1 wrote:
> On 2016/11/22 20:02:26, Ryan Hamilton wrote:
> > On 2016/11/22 19:45:49, tbansal1 wrote:
> > > So, there are 2 problems:
> > > 1. Preconnect function that you referenced above looks at the HTTP server
> > > properties
> > > of the URL, not the proxy (because proxy has not been resolved yet).
> > 
> > *nod*
> > 
> > > 2. That Preconnect function is called multiple times. e.g., this is what
> > > I saw when loading http://cnn.com with data saver on (all the timestamps
are
> > within 1
> > > second):
> > > [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> > > HttpStreamFactoryImpl::Job::Preconnect() num_streams=1
> url=http://www.cnn.com/
> > > [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> > > HttpStreamFactoryImpl::Job::Preconnect() num_streams=3
> > > url=http://beacon.krxd.net/
> > > [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> > > HttpStreamFactoryImpl::Job::Preconnect() num_streams=3
> > > url=http://c.go-mpulse.net/
> > > [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> > > HttpStreamFactoryImpl::Job::Preconnect() num_streams=4
> > url=http://cdn.krxd.net/
> > > [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> > > HttpStreamFactoryImpl::Job::Preconnect() num_streams=3
> > > url=http://cdn.livefyre.com/
> > > [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> > > HttpStreamFactoryImpl::Job::Preconnect() num_streams=3
> > url=http://data.cnn.com/
> > > [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> > > HttpStreamFactoryImpl::Job::Preconnect() num_streams=8
> > url=http://i.cdn.cnn.com/
> > > [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> > > HttpStreamFactoryImpl::Job::Preconnect() num_streams=3
> > > url=http://partner.googleadservices.com/
> > > [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> > > HttpStreamFactoryImpl::Job::Preconnect() num_streams=6
> url=http://www.cnn.com/
> > > [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> > > HttpStreamFactoryImpl::Job::Preconnect() num_streams=10
> > > url=http://www.i.cdn.cnn.com/
> > > [9805:9860:1122/113730:WARNING:http_stream_factory_impl_job.cc(265)] In
> > > HttpStreamFactoryImpl::Job::Preconnect() num_streams=14
> > > url=https://logx.optimizely.com/
> > 
> > Right, even if num_streams were 1 there, that would still not help!
> > 
> > > So, a better fix may be to let the preconnect job run, resolve the proxy,
> and
> > > then stop 
> > > (right before connection is initialized) the job if the resolved proxy
> > supports
> > > http server properties. wdyt?
> > 
> > Yes, though I'd say we want to stop if:
> > 1) the job is a preconnect
> > 2) it's going through a proxy that supports request prioritization
(QUIC/SPDY)
> > 3) there's already a connection available.
> > 
> > Though it might be tricky to implement #3 easily so if instead we wanted a
> > general purpose "AllowPreconnect()" method on the proxy delegate, that might
> be
> > more straightforward. What do you think?
> 
> Yup, that's almost what this CL does. Although, I use the method 
> name ShouldSkipPreconnect() instead of AllowPreconnect().
> 
> Also, this CL does not implement #3 that you suggested. It simply stops
> all preconnects provided #1 and #2 hold.
> Reason being that waiting for the connection
> to be available might be too late. In my http://cnn.com example,
> I see that net/predictor fires off 32 connections
> before the first SPDY/QUIC connection is established.
> So, I do not believe that #3 should be a condition for
> stopping preconnects.

Sorry, I was a bit too specific in #3. I should have said, a connection or job,
exists. So we only run one preconnect job at a time to a particular proxy. When
that job succeeds, a connection will be created. Any subsequent jobs that come
in after that can proceed (but only 1 at a time, of course) but they will
immediately find the existing connection.

tbansal1

ptal. Using ProxyDelegate to allow preconnect for the first request, and disabling it for subsequent ...

4 years, 1 month ago (2016-11-22 21:06:12 UTC) #20

tbansal1

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

4 years, 1 month ago (2016-11-22 21:18:07 UTC) #21

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2522703002/60001

4 years, 1 month ago (2016-11-22 21:18:35 UTC) #22

Ryan Hamilton

https://codereview.chromium.org/2522703002/diff/60001/net/http/http_stream_factory_impl_job_controller.cc File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2522703002/diff/60001/net/http/http_stream_factory_impl_job_controller.cc#newcode129 net/http/http_stream_factory_impl_job_controller.cc:129: } Instead of checking the proxy delegate, I was ...

4 years, 1 month ago (2016-11-22 22:02:58 UTC) #23

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-22 22:39:28 UTC) #24

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 1 month ago (2016-11-22 22:39:29 UTC) #25

tbansal1

rch: ptal. Thanks. https://codereview.chromium.org/2522703002/diff/60001/net/http/http_stream_factory_impl_job_controller.cc File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2522703002/diff/60001/net/http/http_stream_factory_impl_job_controller.cc#newcode129 net/http/http_stream_factory_impl_job_controller.cc:129: } On 2016/11/22 22:02:58, Ryan Hamilton ...

4 years, 1 month ago (2016-11-23 02:06:03 UTC) #26

Ryan Hamilton

Nice progress! This is looking good. https://codereview.chromium.org/2522703002/diff/80001/net/http/http_stream_factory_impl_job.cc File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/2522703002/diff/80001/net/http/http_stream_factory_impl_job.cc#newcode766 net/http/http_stream_factory_impl_job.cc:766: return ERR_IO_PENDING; Instead ...

4 years, 1 month ago (2016-11-23 04:26:27 UTC) #27

tbansal1

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

4 years ago (2016-11-24 00:09:53 UTC) #28

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2522703002/100001

4 years ago (2016-11-24 00:10:43 UTC) #29

tbansal1

rch: ptal. I did some cleanup, added tests. I have also guarded the code behind ...

4 years ago (2016-11-24 00:10:51 UTC) #30

tbansal1

Description was changed from ========== Skip preconnect for HTTP requests when QUIC is in use ...

4 years ago (2016-11-24 00:48:59 UTC) #31

tbansal1

Description was changed from ========== Allow at most one preconnect to HTTP2/QUIC proxy servers If ...

4 years ago (2016-11-24 00:49:16 UTC) #32

commit-bot: I haz the power

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

4 years ago (2016-11-24 02:12:45 UTC) #33

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) ...

4 years ago (2016-11-24 02:12:46 UTC) #34

Ryan Hamilton

Looking great! https://codereview.chromium.org/2522703002/diff/100001/net/http/http_stream_factory.h File net/http/http_stream_factory.h (right): https://codereview.chromium.org/2522703002/diff/100001/net/http/http_stream_factory.h#newcode245 net/http/http_stream_factory.h:245: // Returns true if a preconnection to ...

4 years ago (2016-11-29 00:15:24 UTC) #35

tbansal1

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

4 years ago (2016-11-29 06:46:45 UTC) #40

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2522703002/190001

4 years ago (2016-11-29 06:47:07 UTC) #41

tbansal1

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

4 years ago (2016-11-29 07:12:22 UTC) #44

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2522703002/230001

4 years ago (2016-11-29 07:12:48 UTC) #45

commit-bot: I haz the power

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

4 years ago (2016-11-29 08:02:08 UTC) #46

tbansal1

rch: ptal. Thanks. https://codereview.chromium.org/2522703002/diff/100001/net/http/http_stream_factory.h File net/http/http_stream_factory.h (right): https://codereview.chromium.org/2522703002/diff/100001/net/http/http_stream_factory.h#newcode245 net/http/http_stream_factory.h:245: // Returns true if a preconnection ...

4 years ago (2016-11-29 08:25:43 UTC) #48

Ryan Hamilton

https://codereview.chromium.org/2522703002/diff/100001/net/http/http_stream_factory_impl_job.h File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/2522703002/diff/100001/net/http/http_stream_factory_impl_job.h#newcode107 net/http/http_stream_factory_impl_job.h:107: const HttpRequestInfo& request_info) = 0; On 2016/11/29 08:25:43, tbansal1 ...

4 years ago (2016-11-29 20:47:57 UTC) #49

tbansal1

rch: ptal. I have moved the logic to the factory. https://codereview.chromium.org/2522703002/diff/230001/net/http/http_stream_factory_impl_job.cc File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/2522703002/diff/230001/net/http/http_stream_factory_impl_job.cc#newcode814 ...

4 years ago (2016-12-01 01:43:06 UTC) #52

rch: ptal. I have moved the logic to the factory.

https://codereview.chromium.org/2522703002/diff/230001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/2522703002/diff/230001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:814: return OK;
On 2016/11/29 20:47:57, Ryan Hamilton wrote:
> Since this job is finished, I wonder if it would make sense to explicitly set
> next_state_ = STATE_NONE here before returning.

I can do that. But, there is existing similar code which is not setting
|next_state_| to NONE. See lines 905-906 below. If we want to do this, we should
be consistent and do it at both places.

https://codereview.chromium.org/2522703002/diff/230001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job.h (right):

https://codereview.chromium.org/2522703002/diff/230001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.h:108: const ProxyInfo& proxy_info) = 0;
On 2016/11/29 20:47:57, Ryan Hamilton wrote:
> Since the controller already knows if it's a preconnect or not, lets remove
the
> restriction that this method only be called for preconnect jobs. In that case,
> perhaps just name this method ShouldConnect() (and move it down next to
> ShouldWait()).
> 
> I'm inclined to do this because right now we have logic for not preconnecting
in
> 3 places: job, controller, and factory. I'd be nice to reduce this.

Done. Now all the logic is in the factory.

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

https://codereview.chromium.org/2522703002/diff/230001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller.cc:399: // exists.
On 2016/11/29 20:47:57, Ryan Hamilton wrote:
> I don't follow why this is true. Is it the case that we don't do alternative
> jobs when preconnecting or we do have alternate jobs when preconnecting but we
> don't want to skip in that case?

I had that conditions because we don't do alternative jobs when preconnecting.
But, you are right, we do not really need to enforce that check here. I have
removed it.

https://codereview.chromium.org/2522703002/diff/230001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller.cc:414:
factory_->PreconnectingViaProxy(proxy_info.proxy_server());
On 2016/11/29 20:47:57, Ryan Hamilton wrote:
> It looks like we can easily combine these two method into one. I would also
> probably move the ProxyServerSupportsPriorities() logic up into that method. 
> You could probably move allow_only_one_preconnect_to_proxy_servers_ up to the
> factory too.
> 
> In that case, the only logic not up there would be checking is_preconnect_.
> Perhaps it would make sense to expose JobController::is_preconnect_ via a
public
> getter, and then moving all of the logic up to the factory.

Done.

https://codereview.chromium.org/2522703002/diff/230001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller.cc:1135:
!proxy_info.proxy_server().is_quic()) {
On 2016/11/29 20:47:57, Ryan Hamilton wrote:
> QUIC always supports priorities so I think you can just return true here.

Done.

https://codereview.chromium.org/2522703002/diff/230001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job_controller.h (right):

https://codereview.chromium.org/2522703002/diff/230001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller.h:121: bool
OnInitConnectionForPreconnectJob(const ProxyInfo& proxy_info) override;
On 2016/11/29 20:47:57, Ryan Hamilton wrote:
> nit: newline after to be consistent.

Done.

Ryan Hamilton

https://codereview.chromium.org/2522703002/diff/290001/net/http/http_stream_factory_impl.h File net/http/http_stream_factory_impl.h (right): https://codereview.chromium.org/2522703002/diff/290001/net/http/http_stream_factory_impl.h#newcode136 net/http/http_stream_factory_impl.h:136: bool OnInitConnection(JobController* controller, const ProxyInfo& proxy_info); Can |controller| be ...

4 years ago (2016-12-01 02:03:36 UTC) #54

tbansal1

rch: ptal thanks. https://codereview.chromium.org/2522703002/diff/290001/net/http/http_stream_factory_impl.h File net/http/http_stream_factory_impl.h (right): https://codereview.chromium.org/2522703002/diff/290001/net/http/http_stream_factory_impl.h#newcode136 net/http/http_stream_factory_impl.h:136: bool OnInitConnection(JobController* controller, const ProxyInfo& proxy_info); ...

4 years ago (2016-12-02 01:27:31 UTC) #56

rch: ptal thanks.

https://codereview.chromium.org/2522703002/diff/290001/net/http/http_stream_f...
File net/http/http_stream_factory_impl.h (right):

https://codereview.chromium.org/2522703002/diff/290001/net/http/http_stream_f...
net/http/http_stream_factory_impl.h:136: bool OnInitConnection(JobController*
controller, const ProxyInfo& proxy_info);
On 2016/12/01 02:03:36, Ryan Hamilton wrote:
> Can |controller| be a const ref instead of a *?

Done.

https://codereview.chromium.org/2522703002/diff/290001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/2522703002/diff/290001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:810: // Since connection initialization
can be skipped, we're done.
On 2016/12/01 02:03:36, Ryan Hamilton wrote:
> nit: avoid using the first person in comments.

Fixed.

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

https://codereview.chromium.org/2522703002/diff/290001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller.cc:172: void
HttpStreamFactoryImpl::JobController::OnStreamReady(
On 2016/12/01 02:03:36, Ryan Hamilton wrote:
> Just to confirm, is this method called for preconnects? If not, I wonder if we
> might want to hook into OnInitConnectionComplete()?

This is called for all jobs (not just preconnects). I had tried
OnInitConnectionComplete() before, but that would be too early. We need to call
this after the proxy server has been added to the list of spdy sessions here:
https://cs.chromium.org/chromium/src/net/spdy/spdy_session_pool.cc?rcl=148058...

Tracing back this call, this insertion happens while the spdy stream is being
created which IIUC happens after connection initialization is finished. So,
OnInitConnectionComplete() is too early.

Calling it after the session has been added to the list of spdy sessions ensures
that next preconnect will return early here:
https://cs.chromium.org/chromium/src/net/http/http_stream_factory_impl_job.cc...

https://codereview.chromium.org/2522703002/diff/290001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller.cc:181:
factory_->OnStreamReadyViaProxy(job->proxy_info());
On 2016/12/01 02:03:36, Ryan Hamilton wrote:
> Heh. Sorry! Since this method is called for all jobs, not just for those with
> proxies, we can probably remove ViaProxy from this method. Sorry!

Done.

https://codereview.chromium.org/2522703002/diff/290001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job_controller.h (right):

https://codereview.chromium.org/2522703002/diff/290001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller.h:120: //
HttpStreamFactoryImpl::Job::Delegate implementation:
On 2016/12/01 02:03:36, Ryan Hamilton wrote:
> I think you don't need this comment or the one on line 123 since we already
have
> the comment on line 70

Done.

https://codereview.chromium.org/2522703002/diff/290001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_unittest.cc (right):

https://codereview.chromium.org/2522703002/diff/290001/net/http/http_stream_f...
net/http/http_stream_factory_impl_unittest.cc:1238: for (const auto& test :
tests) {
On 2016/12/01 02:03:36, Ryan Hamilton wrote:
> nit: this is a bit hard to grok. How about:
> 
> for (bool set_http_server_properties : {true, false}) {
> }
> 
> (Does that work?)

Done.

https://codereview.chromium.org/2522703002/diff/290001/net/http/http_stream_f...
net/http/http_stream_factory_impl_unittest.cc:1261: }
On 2016/12/01 02:03:36, Ryan Hamilton wrote:
> Since the code no longer checks http server properties for QUIC proxies, do we
> need to do this?

Currently yes. To skip this, I would need to set the proxy to a QUIC proxy
instead of HTTPS on line 1242-1243 above. I tried doing that, but then I am not
sure how to verify the number of streams created from the
MockClientSocketPoolManager.

https://codereview.chromium.org/2522703002/diff/290001/net/http/http_stream_f...
net/http/http_stream_factory_impl_unittest.cc:1313: // Second preconnect job
should not succeed.
On 2016/12/01 02:03:36, Ryan Hamilton wrote:
> I'm not quite sure how to read the expectations here. This is trying to test
> that a second concurrent preconnect is a no-op, is that right? 
Correct.
> How do we know
> this preconnect is concurrent?
Because the first preconnect never finishes. That's guaranteed because we are
using CapturePreconnect*SocketPool (same as other Preconnect tests in this
file). This socket pool never calls the callback to Job or higher layers. So,
HttpStreamFactoryImpl::Job::OnPreconnectsComplete() is guaranteed to be never
called, and so for all purposes first preconnect never finishes.

> 
> It would also be good to have a test where we preconnect, and then eventually
> close the preconnected socket. Then attempt *another* preconnect. This second
> preconnection should create a new socket.
IIUC, this is slightly incorrect (see my other comment in controller.cc). A
proxy server should be removed from |preconnecting_proxy_servers_| in
factory_impl.cc after the stream has been established (NOT after the first
preconnect has completed). This is because once the stream is established, then
any subsequent preconnects will be cancelled here:
https://cs.chromium.org/chromium/src/net/http/http_stream_factory_impl_job.cc...

If we remove proxy server from |preconnecting_proxy_servers_|  after the first
successful preconnect, then subsequent preconnects will result in unnecessary
creation of more sockets.

The test you are asking for is almost the other test that I added below except
that it creates a new request which causes successful stream creation which
causes proxy server to be evicted from |preconnecting_proxy_servers_|.

tbansal1

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

4 years ago (2016-12-02 01:27:33 UTC) #57

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2522703002/350001

4 years ago (2016-12-02 01:28:09 UTC) #58

Ryan Hamilton

Almost there, I think. https://codereview.chromium.org/2522703002/diff/290001/net/http/http_stream_factory_impl_job_controller.cc File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2522703002/diff/290001/net/http/http_stream_factory_impl_job_controller.cc#newcode172 net/http/http_stream_factory_impl_job_controller.cc:172: void HttpStreamFactoryImpl::JobController::OnStreamReady( On 2016/12/02 01:27:30, ...

4 years ago (2016-12-02 01:47:30 UTC) #59

Almost there, I think.

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

https://codereview.chromium.org/2522703002/diff/290001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller.cc:172: void
HttpStreamFactoryImpl::JobController::OnStreamReady(
On 2016/12/02 01:27:30, tbansal1 wrote:
> On 2016/12/01 02:03:36, Ryan Hamilton wrote:
> > Just to confirm, is this method called for preconnects? If not, I wonder if
we
> > might want to hook into OnInitConnectionComplete()?
> 
> This is called for all jobs (not just preconnects). I had tried
> OnInitConnectionComplete() before, but that would be too early. We need to
call
> this after the proxy server has been added to the list of spdy sessions here:
>
https://cs.chromium.org/chromium/src/net/spdy/spdy_session_pool.cc?rcl=148058...
> 
> Tracing back this call, this insertion happens while the spdy stream is being
> created which IIUC happens after connection initialization is finished. So,
> OnInitConnectionComplete() is too early.
> 
> Calling it after the session has been added to the list of spdy sessions
ensures
> that next preconnect will return early here:
>
https://cs.chromium.org/chromium/src/net/http/http_stream_factory_impl_job.cc...

Ah! Thanks for walking me through this. You're right, of course.

https://codereview.chromium.org/2522703002/diff/290001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_unittest.cc (right):

https://codereview.chromium.org/2522703002/diff/290001/net/http/http_stream_f...
net/http/http_stream_factory_impl_unittest.cc:1261: }
On 2016/12/02 01:27:30, tbansal1 wrote:
> On 2016/12/01 02:03:36, Ryan Hamilton wrote:
> > Since the code no longer checks http server properties for QUIC proxies, do
we
> > need to do this?
> 
> Currently yes. To skip this, I would need to set the proxy to a QUIC proxy
> instead of HTTPS on line 1242-1243 above. I tried doing that, but then I am
not
> sure how to verify the number of streams created from the
> MockClientSocketPoolManager.

Oh, I thought it was a QUIC proxy. Not sure how I convinced myself of that. Oh,
'cause of the alt-svc setting. Hm. If we're connecting to an HTTPS proxy, then
we won't honor the alt-svc for that proxy (as you know 'cause of all the work
you had to do to plumb down alternative proxy code). In any case, I suspect you
want to call SetSupportsSpdy here instead.

https://codereview.chromium.org/2522703002/diff/290001/net/http/http_stream_f...
net/http/http_stream_factory_impl_unittest.cc:1313: // Second preconnect job
should not succeed.
On 2016/12/02 01:27:30, tbansal1 wrote:
> On 2016/12/01 02:03:36, Ryan Hamilton wrote:
> > I'm not quite sure how to read the expectations here. This is trying to test
> > that a second concurrent preconnect is a no-op, is that right? 
> Correct.
> > How do we know
> > this preconnect is concurrent?
> Because the first preconnect never finishes. That's guaranteed because we are
> using CapturePreconnect*SocketPool (same as other Preconnect tests in this
> file). This socket pool never calls the callback to Job or higher layers. So,
> HttpStreamFactoryImpl::Job::OnPreconnectsComplete() is guaranteed to be never
> called, and so for all purposes first preconnect never finishes.
> 
> > 
> > It would also be good to have a test where we preconnect, and then
eventually
> > close the preconnected socket. Then attempt *another* preconnect. This
second
> > preconnection should create a new socket.
> IIUC, this is slightly incorrect (see my other comment in controller.cc). A
> proxy server should be removed from |preconnecting_proxy_servers_| in
> factory_impl.cc after the stream has been established (NOT after the first
> preconnect has completed). This is because once the stream is established,
then
> any subsequent preconnects will be cancelled here:
>
https://cs.chromium.org/chromium/src/net/http/http_stream_factory_impl_job.cc...
> 
> If we remove proxy server from |preconnecting_proxy_servers_|  after the first
> successful preconnect, then subsequent preconnects will result in unnecessary
> creation of more sockets.
> 
> The test you are asking for is almost the other test that I added below except
> that it creates a new request which causes successful stream creation which
> causes proxy server to be evicted from |preconnecting_proxy_servers_|.
> 

I'm confused about "when a stream is established". In the case of Preconnect I
did not think an actual HttpStream was created?

https://codereview.chromium.org/2522703002/diff/350001/net/http/http_stream_f...
File net/http/http_stream_factory_impl.cc (right):

https://codereview.chromium.org/2522703002/diff/350001/net/http/http_stream_f...
net/http/http_stream_factory_impl.cc:296: }
nit: do we actually need these two lines?

tbansal1

I will handle other comments later, but wanted to know your thoughts on this before. ...

4 years ago (2016-12-02 02:06:43 UTC) #60

I will handle other comments later, but wanted to know your thoughts on this
before.

https://codereview.chromium.org/2522703002/diff/290001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_unittest.cc (right):

https://codereview.chromium.org/2522703002/diff/290001/net/http/http_stream_f...
net/http/http_stream_factory_impl_unittest.cc:1313: // Second preconnect job
should not succeed.
On 2016/12/02 01:47:30, Ryan Hamilton wrote:
> On 2016/12/02 01:27:30, tbansal1 wrote:
> > On 2016/12/01 02:03:36, Ryan Hamilton wrote:
> > > I'm not quite sure how to read the expectations here. This is trying to
test
> > > that a second concurrent preconnect is a no-op, is that right? 
> > Correct.
> > > How do we know
> > > this preconnect is concurrent?
> > Because the first preconnect never finishes. That's guaranteed because we
are
> > using CapturePreconnect*SocketPool (same as other Preconnect tests in this
> > file). This socket pool never calls the callback to Job or higher layers.
So,
> > HttpStreamFactoryImpl::Job::OnPreconnectsComplete() is guaranteed to be
never
> > called, and so for all purposes first preconnect never finishes.
> > 
> > > 
> > > It would also be good to have a test where we preconnect, and then
> eventually
> > > close the preconnected socket. Then attempt *another* preconnect. This
> second
> > > preconnection should create a new socket.
> > IIUC, this is slightly incorrect (see my other comment in controller.cc). A
> > proxy server should be removed from |preconnecting_proxy_servers_| in
> > factory_impl.cc after the stream has been established (NOT after the first
> > preconnect has completed). This is because once the stream is established,
> then
> > any subsequent preconnects will be cancelled here:
> >
>
https://cs.chromium.org/chromium/src/net/http/http_stream_factory_impl_job.cc...
> > 
> > If we remove proxy server from |preconnecting_proxy_servers_|  after the
first
> > successful preconnect, then subsequent preconnects will result in
unnecessary
> > creation of more sockets.
> > 
> > The test you are asking for is almost the other test that I added below
except
> > that it creates a new request which causes successful stream creation which
> > causes proxy server to be evicted from |preconnecting_proxy_servers_|.
> > 
> 
> I'm confused about "when a stream is established". In the case of Preconnect I
> did not think an actual HttpStream was created?

Right, Preconnects do not create streams. They only result in connection
establishment.

Once the stream is established (potentially by a MAIN or ALTERNATIVE job), after
that the preconnect jobs are no-op because there would be an existing
SpdySession in the spdy_session_pool. This is true with/without this CL.

Before the stream is established, and without this CL, all preconnects would
succeed. This can potentially create multiple sockets which we are trying to
avoid.

Before the stream is established, and with this CL, the first preconnect would
succeed. Afterwards, all preconnects would be blocked until at least one stream
is established (this would require a MAIN or ALTERNATIVE job to be created).

This is confusing because there is no good place where I can remove the proxy
server from the set of preconnecting proxy servers. Currently, I am removing it
at the time of stream establishment because that's when the spdy_session_pool
logic kicks in and prevents further preconnects from opening connections. But I
feel that's kind of unintuitive. May be it would be less confusing if I rename
|preconnecting_proxy_servers_| in factory_impl.h to something else. Suggestions?

commit-bot: I haz the power

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

4 years ago (2016-12-02 02:30:29 UTC) #61

tbansal1

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

4 years ago (2016-12-02 06:54:51 UTC) #64

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2522703002/390001

4 years ago (2016-12-02 06:55:12 UTC) #65

commit-bot: I haz the power

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

4 years ago (2016-12-02 07:55:42 UTC) #66

tbansal1

rch: ptal. Thanks. https://codereview.chromium.org/2522703002/diff/290001/net/http/http_stream_factory_impl_unittest.cc File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/2522703002/diff/290001/net/http/http_stream_factory_impl_unittest.cc#newcode1261 net/http/http_stream_factory_impl_unittest.cc:1261: } On 2016/12/02 01:47:30, Ryan Hamilton ...

4 years ago (2016-12-06 01:10:49 UTC) #70

tbansal1

Description was changed from ========== Allow at most one preconnect to HTTP2/QUIC proxy servers If ...

4 years ago (2016-12-06 01:11:42 UTC) #71

tbansal1

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

4 years ago (2016-12-06 01:11:54 UTC) #72

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2522703002/430001

4 years ago (2016-12-06 01:12:23 UTC) #73

commit-bot: I haz the power

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

4 years ago (2016-12-06 02:03:30 UTC) #74

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_cronet_tester on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cronet_tester/builds/754)

4 years ago (2016-12-06 02:03:31 UTC) #75

Ryan Hamilton

https://codereview.chromium.org/2522703002/diff/430001/net/http/http_stream_factory_impl_unittest.cc File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/2522703002/diff/430001/net/http/http_stream_factory_impl_unittest.cc#newcode1229 net/http/http_stream_factory_impl_unittest.cc:1229: for (int num_streams = 1; num_streams < 3; ++num_streams) ...

4 years ago (2016-12-06 20:09:40 UTC) #76

tbansal1

rch: ptal. thanks. https://codereview.chromium.org/2522703002/diff/430001/net/http/http_stream_factory_impl_unittest.cc File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/2522703002/diff/430001/net/http/http_stream_factory_impl_unittest.cc#newcode1229 net/http/http_stream_factory_impl_unittest.cc:1229: for (int num_streams = 1; num_streams ...

4 years ago (2016-12-07 00:59:34 UTC) #77

rch: ptal. thanks.

https://codereview.chromium.org/2522703002/diff/430001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_unittest.cc (right):

https://codereview.chromium.org/2522703002/diff/430001/net/http/http_stream_f...
net/http/http_stream_factory_impl_unittest.cc:1229: for (int num_streams = 1;
num_streams < 3; ++num_streams) {
On 2016/12/06 20:09:40, Ryan Hamilton wrote:
> nit: You could also do:
> 
> for (int stream {1, 2}.
> 
> Or perhaps:
> 
> for (bool is_first_request : { true, false }) {
> 

Skipping this because I do not think this is too confusing :). This also matches
the existing patterns in this file:
https://cs.chromium.org/search/?q=%22for+(int+num_streams+%3D+1%22&sq=package...

Please let me know if you feel strongly about this.

https://codereview.chromium.org/2522703002/diff/430001/net/http/http_stream_f...
net/http/http_stream_factory_impl_unittest.cc:1239:
http_server_properties.SetSupportsSpdy(spdy_server, true);
On 2016/12/06 20:09:40, Ryan Hamilton wrote:
> Just to sanity check, have you verified that SetSupportsSpdy is called for
SPDY
> proxies? 
Yes.
> I would expect so, but it'd be a good idea to verify.  We should
> probably have a test in SpdyNetworkTransactionTest
Done. Added test HttpStreamFactoryTest.RequestSpdyHttpStreamHttpURL

https://codereview.chromium.org/2522703002/diff/430001/net/http/http_stream_f...
net/http/http_stream_factory_impl_unittest.cc:1278:
peer.SetClientSocketPoolManager(std::move(mock_pool_manager));
On 2016/12/06 20:09:40, Ryan Hamilton wrote:
> It seems like a bunch of this setup could be performed outside of the
> per-request loop, couldn't it? 
We are creating a new socket pool for every request.
> If we're creating a different HttpNetworkSession
> for each request, doesn't that mean a different HttpStreamFactory? 
No, session stays the same for different requests. Only the pool changes.
> If so, I'm
> not sure I understand how this test passes, since I would think that each
> preconnect job would appear to be the first preconnect job to the
> HttpStreamFactory.  Am I being dense?
See above. Only the pool changes for every request, not the session.

Creating a new socket pool makes it easier to write the expectations since the
test looks at "last_num_streams()" for different pools. If the same socket pool
is reused across requests, then last_num_streams() may reflect streams from the
last request, or from the current request, which is confusing.

Ryan Hamilton

lgtm https://codereview.chromium.org/2522703002/diff/430001/net/http/http_stream_factory_impl_unittest.cc File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/2522703002/diff/430001/net/http/http_stream_factory_impl_unittest.cc#newcode1278 net/http/http_stream_factory_impl_unittest.cc:1278: peer.SetClientSocketPoolManager(std::move(mock_pool_manager)); On 2016/12/07 00:59:34, tbansal1 wrote: > On ...

4 years ago (2016-12-07 20:07:10 UTC) #78

rkaplow

https://codereview.chromium.org/2522703002/diff/450001/net/http/http_stream_factory_impl.cc File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/2522703002/diff/450001/net/http/http_stream_factory_impl.cc#newcode271 net/http/http_stream_factory_impl.cc:271: UMA_HISTOGRAM_COUNTS_100("Net.PreconnectSkippedToProxyServers", 1); this means you will only be emitting ...

4 years ago (2016-12-07 21:24:42 UTC) #83

tbansal1

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

4 years ago (2016-12-07 23:15:42 UTC) #84

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2522703002/470001

4 years ago (2016-12-07 23:16:17 UTC) #85

tbansal1

rkaplow: ptal. thanks. https://codereview.chromium.org/2522703002/diff/450001/net/http/http_stream_factory_impl.cc File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/2522703002/diff/450001/net/http/http_stream_factory_impl.cc#newcode271 net/http/http_stream_factory_impl.cc:271: UMA_HISTOGRAM_COUNTS_100("Net.PreconnectSkippedToProxyServers", 1); On 2016/12/07 21:24:42, rkaplow ...

4 years ago (2016-12-07 23:16:30 UTC) #86

commit-bot: I haz the power

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

4 years ago (2016-12-08 00:45:43 UTC) #87

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/81575)

4 years ago (2016-12-08 00:45:45 UTC) #88

tbansal1

The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org Link to the ...

4 years ago (2016-12-12 17:40:53 UTC) #91

commit-bot: I haz the power

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

4 years ago (2016-12-12 17:41:14 UTC) #92

commit-bot: I haz the power

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

4 years ago (2016-12-12 20:44:42 UTC) #93

commit-bot: I haz the power

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_android_rel_ng/builds/197334)

4 years ago (2016-12-12 20:44:44 UTC) #94

commit-bot: I haz the power

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

4 years ago (2016-12-13 00:03:18 UTC) #96

commit-bot: I haz the power

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

4 years ago (2016-12-13 03:33:39 UTC) #97

commit-bot: I haz the power

Failed to apply patch for net/http/http_stream_factory_impl_unittest.cc: While running git apply --index -p1; error: patch failed: ...

4 years ago (2016-12-13 03:33:42 UTC) #98

Failed to apply patch for net/http/http_stream_factory_impl_unittest.cc:
While running git apply --index -p1;
  error: patch failed: net/http/http_stream_factory_impl_unittest.cc:35
  error: net/http/http_stream_factory_impl_unittest.cc: patch does not apply

Patch:       net/http/http_stream_factory_impl_unittest.cc
Index: net/http/http_stream_factory_impl_unittest.cc
diff --git a/net/http/http_stream_factory_impl_unittest.cc
b/net/http/http_stream_factory_impl_unittest.cc
index
9ea8df492aa43761889b4caeea35f66c7f7f098f..f7227ce30fb08b907ebfd90b9850169dc21dad61
100644
--- a/net/http/http_stream_factory_impl_unittest.cc
+++ b/net/http/http_stream_factory_impl_unittest.cc
@@ -14,7 +14,9 @@
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
+#include "base/metrics/field_trial.h"
 #include "base/run_loop.h"
+#include "base/test/histogram_tester.h"
 #include "net/base/port_util.h"
 #include "net/base/test_completion_callback.h"
 #include "net/base/test_proxy_delegate.h"
@@ -35,6 +37,7 @@
 #include "net/http/transport_security_state.h"
 #include "net/log/net_log_with_source.h"
 #include "net/proxy/proxy_info.h"
+#include "net/proxy/proxy_server.h"
 #include "net/proxy/proxy_service.h"
 #include "net/quic/core/quic_http_utils.h"
 #include "net/quic/core/quic_server_id.h"
@@ -1215,6 +1218,101 @@ TEST_F(HttpStreamFactoryTest,
QuicDisablePreConnectIfZeroRtt) {
   }
 }
 
+// Verify that the proxy delegate can disable preconnect jobs to only the proxy
+// servers that support request priorities.
+TEST_F(HttpStreamFactoryTest, ProxyDelegateDisablesPreconnect) {
+  base::FieldTrialList field_trial_list(nullptr);
+  base::FieldTrialList::CreateFieldTrial(
+      "NetAllowOnlyOnePreconnectToProxyServers", "Enabled");
+
+  for (bool set_http_server_properties : {false, true}) {
+    for (int num_streams = 1; num_streams < 3; ++num_streams) {
+      base::HistogramTester histogram_tester;
+      GURL url = GURL("http://www.google.com");
+      std::unique_ptr<ProxyService> proxy_service =
+          ProxyService::CreateFixedFromPacResult("HTTPS myproxy.org:443");
+
+      // Set up the proxy server as a server that supports request priorities.
+      HttpServerPropertiesImpl http_server_properties;
+      if (set_http_server_properties) {
+        url::SchemeHostPort spdy_server("https", "myproxy.org", 443);
+        http_server_properties.SetSupportsSpdy(spdy_server, true);
+      }
+
+      SpdySessionDependencies session_deps;
+      HttpNetworkSession::Params params =
+          SpdySessionDependencies::CreateSessionParams(&session_deps);
+      params.enable_quic = true;
+      params.proxy_service = proxy_service.get();
+      params.http_server_properties = &http_server_properties;
+
+      std::unique_ptr<HttpNetworkSession> session(
+          new HttpNetworkSession(params));
+
+      HttpNetworkSessionPeer peer(session.get());
+      HostPortPair proxy_host("myproxy.org", 443);
+
+      for (int preconnect_request = 0; preconnect_request < 2;
+           ++preconnect_request) {
+        CapturePreconnectsHttpProxySocketPool* http_proxy_pool =
+            new CapturePreconnectsHttpProxySocketPool(
+                session_deps.host_resolver.get(),
+                session_deps.cert_verifier.get(),
+                session_deps.transport_security_state.get(),
+                session_deps.cert_transparency_verifier.get(),
+                session_deps.ct_policy_enforcer.get());
+        CapturePreconnectsSSLSocketPool* ssl_conn_pool =
+            new CapturePreconnectsSSLSocketPool(
+                session_deps.host_resolver.get(),
+                session_deps.cert_verifier.get(),
+                session_deps.transport_security_state.get(),
+                session_deps.cert_transparency_verifier.get(),
+                session_deps.ct_policy_enforcer.get());
+
+        std::unique_ptr<MockClientSocketPoolManager> mock_pool_manager(
+            new MockClientSocketPoolManager);
+        mock_pool_manager->SetSocketPoolForHTTPProxy(
+            proxy_host, base::WrapUnique(http_proxy_pool));
+        mock_pool_manager->SetSocketPoolForSSLWithProxy(
+            proxy_host, base::WrapUnique(ssl_conn_pool));
+        peer.SetClientSocketPoolManager(std::move(mock_pool_manager));
+
+        HttpRequestInfo request;
+        request.method = "GET";
+        request.url = url;
+        request.load_flags = 0;
+
+        if (preconnect_request == 0) {
+          // First preconnect job should succeed.
+          session->http_stream_factory()->PreconnectStreams(num_streams,
+                                                            request);
+          EXPECT_EQ(-1, ssl_conn_pool->last_num_streams());
+          EXPECT_EQ(num_streams, http_proxy_pool->last_num_streams());
+        } else {
+          // Second preconnect job should not succeed.
+          session->http_stream_factory()->PreconnectStreams(num_streams,
+                                                            request);
+          EXPECT_EQ(-1, ssl_conn_pool->last_num_streams());
+          if (set_http_server_properties) {
+            EXPECT_EQ(-1, http_proxy_pool->last_num_streams());
+          } else {
+            EXPECT_EQ(num_streams, http_proxy_pool->last_num_streams());
+          }
+        }
+      }
+      // Preconnects should be skipped only to proxy servers that support
+      // request priorities.
+      if (set_http_server_properties) {
+        histogram_tester.ExpectUniqueSample(
+            "Net.PreconnectSkippedToProxyServers", 1, 1);
+      } else {
+       
histogram_tester.ExpectTotalCount("Net.PreconnectSkippedToProxyServers",
+                                          0);
+      }
+    }
+  }
+}
+
 namespace {
 
 TEST_F(HttpStreamFactoryTest, PrivacyModeDisablesChannelId) {
@@ -1509,6 +1607,91 @@ TEST_F(HttpStreamFactoryTest, RequestHttpStreamOverProxy)
{
   EXPECT_FALSE(waiter.used_proxy_info().is_direct());
 }
 
+// Verifies that once a stream has been created to a proxy server (that
supports
+// request priorities) the next preconnect job can again open new sockets.
+TEST_F(HttpStreamFactoryTest, RequestHttpStreamOverProxyWithPreconnects) {
+  base::FieldTrialList field_trial_list(nullptr);
+  base::FieldTrialList::CreateFieldTrial(
+      "NetAllowOnlyOnePreconnectToProxyServers", "Enabled");
+
+  SpdySessionDependencies session_deps(
+      ProxyService::CreateFixed("https://myproxy.org:443"));
+
+  // Set up the proxy server as a server that supports request priorities.
+  std::unique_ptr<HttpServerPropertiesImpl> http_server_properties(
+      new HttpServerPropertiesImpl());
+  url::SchemeHostPort spdy_server("https", "myproxy.org", 443);
+  http_server_properties->SetSupportsSpdy(spdy_server, true);
+  session_deps.http_server_properties = std::move(http_server_properties);
+
+  StaticSocketDataProvider socket_data;
+  socket_data.set_connect_data(MockConnect(ASYNC, OK));
+  session_deps.socket_factory->AddSocketDataProvider(&socket_data);
+
+  SSLSocketDataProvider ssl_data(ASYNC, OK);
+  session_deps.socket_factory->AddSSLSocketDataProvider(&ssl_data);
+
+  std::unique_ptr<HttpNetworkSession> session(
+      SpdySessionDependencies::SpdyCreateSession(&session_deps));
+
+  // Now preconnect a request. Only the first preconnect should succeed.
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("http://www.google.com");
+  request_info.load_flags = 0;
+
+  base::HistogramTester histogram_tester;
+  const int num_preconnects = 5;
+
+  // First preconnect would be successful. The next |num_preconnects-1| would
be
+  // skipped.
+  for (int preconnect_request = 1; preconnect_request <= num_preconnects;
+       ++preconnect_request) {
+    session->http_stream_factory()->PreconnectStreams(1, request_info);
+    base::RunLoop().RunUntilIdle();
+    while (GetSocketPoolGroupCount(session->GetSocketPoolForHTTPProxy(
+               HttpNetworkSession::NORMAL_SOCKET_POOL,
+               HostPortPair("myproxy.org", 443))) == 0) {
+      base::RunLoop().RunUntilIdle();
+    }
+  }
+
+  histogram_tester.ExpectUniqueSample("Net.PreconnectSkippedToProxyServers", 1,
+                                      num_preconnects - 1);
+
+  // Start a request. This should cause subsequent preconnect to proxy server
to
+  // succeed.
+  SSLConfig ssl_config;
+  StreamRequestWaiter waiter;
+  std::unique_ptr<HttpStreamRequest> request(
+      session->http_stream_factory()->RequestStream(
+          request_info, DEFAULT_PRIORITY, ssl_config, ssl_config, &waiter,
+          NetLogWithSource()));
+  waiter.WaitForStream();
+  EXPECT_TRUE(waiter.stream_done());
+  ASSERT_TRUE(nullptr != waiter.stream());
+  EXPECT_TRUE(nullptr == waiter.websocket_stream());
+
+  EXPECT_EQ(1, GetSocketPoolGroupCount(session->GetSocketPoolForHTTPProxy(
+                   HttpNetworkSession::NORMAL_SOCKET_POOL,
+                   HostPortPair("myproxy.org", 443))));
+  EXPECT_FALSE(waiter.used_proxy_info().is_direct());
+
+  for (int preconnect_request = 1; preconnect_request <= num_preconnects;
+       ++preconnect_request) {
+    session->http_stream_factory()->PreconnectStreams(1, request_info);
+    base::RunLoop().RunUntilIdle();
+    EXPECT_EQ(1, GetSocketPoolGroupCount(session->GetSocketPoolForHTTPProxy(
+                     HttpNetworkSession::NORMAL_SOCKET_POOL,
+                     HostPortPair("myproxy.org", 443))));
+  }
+
+  // First preconnect would be successful since the stream to the proxy server
+  // has suceeded. The next |num_preconnects-1| would be skipped.
+  histogram_tester.ExpectUniqueSample("Net.PreconnectSkippedToProxyServers", 1,
+                                      2 * (num_preconnects - 1));
+}
+
 TEST_F(HttpStreamFactoryTest, RequestWebSocketBasicHandshakeStream) {
   SpdySessionDependencies session_deps(ProxyService::CreateDirect());
 
@@ -1643,7 +1826,7 @@ TEST_F(HttpStreamFactoryTest,
RequestWebSocketBasicHandsha…
(message too large)

commit-bot: I haz the power

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

4 years ago (2016-12-13 03:47:40 UTC) #100

commit-bot: I haz the power

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

4 years ago (2016-12-13 03:53:29 UTC) #101

commit-bot: I haz the power

Failed to apply patch for net/http/http_stream_factory_impl_unittest.cc: While running git apply --index -p1; error: patch failed: ...

4 years ago (2016-12-13 03:53:31 UTC) #102

Failed to apply patch for net/http/http_stream_factory_impl_unittest.cc:
While running git apply --index -p1;
  error: patch failed: net/http/http_stream_factory_impl_unittest.cc:35
  error: net/http/http_stream_factory_impl_unittest.cc: patch does not apply

Patch:       net/http/http_stream_factory_impl_unittest.cc
Index: net/http/http_stream_factory_impl_unittest.cc
diff --git a/net/http/http_stream_factory_impl_unittest.cc
b/net/http/http_stream_factory_impl_unittest.cc
index
9ea8df492aa43761889b4caeea35f66c7f7f098f..f7227ce30fb08b907ebfd90b9850169dc21dad61
100644
--- a/net/http/http_stream_factory_impl_unittest.cc
+++ b/net/http/http_stream_factory_impl_unittest.cc
@@ -14,7 +14,9 @@
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
+#include "base/metrics/field_trial.h"
 #include "base/run_loop.h"
+#include "base/test/histogram_tester.h"
 #include "net/base/port_util.h"
 #include "net/base/test_completion_callback.h"
 #include "net/base/test_proxy_delegate.h"
@@ -35,6 +37,7 @@
 #include "net/http/transport_security_state.h"
 #include "net/log/net_log_with_source.h"
 #include "net/proxy/proxy_info.h"
+#include "net/proxy/proxy_server.h"
 #include "net/proxy/proxy_service.h"
 #include "net/quic/core/quic_http_utils.h"
 #include "net/quic/core/quic_server_id.h"
@@ -1215,6 +1218,101 @@ TEST_F(HttpStreamFactoryTest,
QuicDisablePreConnectIfZeroRtt) {
   }
 }
 
+// Verify that the proxy delegate can disable preconnect jobs to only the proxy
+// servers that support request priorities.
+TEST_F(HttpStreamFactoryTest, ProxyDelegateDisablesPreconnect) {
+  base::FieldTrialList field_trial_list(nullptr);
+  base::FieldTrialList::CreateFieldTrial(
+      "NetAllowOnlyOnePreconnectToProxyServers", "Enabled");
+
+  for (bool set_http_server_properties : {false, true}) {
+    for (int num_streams = 1; num_streams < 3; ++num_streams) {
+      base::HistogramTester histogram_tester;
+      GURL url = GURL("http://www.google.com");
+      std::unique_ptr<ProxyService> proxy_service =
+          ProxyService::CreateFixedFromPacResult("HTTPS myproxy.org:443");
+
+      // Set up the proxy server as a server that supports request priorities.
+      HttpServerPropertiesImpl http_server_properties;
+      if (set_http_server_properties) {
+        url::SchemeHostPort spdy_server("https", "myproxy.org", 443);
+        http_server_properties.SetSupportsSpdy(spdy_server, true);
+      }
+
+      SpdySessionDependencies session_deps;
+      HttpNetworkSession::Params params =
+          SpdySessionDependencies::CreateSessionParams(&session_deps);
+      params.enable_quic = true;
+      params.proxy_service = proxy_service.get();
+      params.http_server_properties = &http_server_properties;
+
+      std::unique_ptr<HttpNetworkSession> session(
+          new HttpNetworkSession(params));
+
+      HttpNetworkSessionPeer peer(session.get());
+      HostPortPair proxy_host("myproxy.org", 443);
+
+      for (int preconnect_request = 0; preconnect_request < 2;
+           ++preconnect_request) {
+        CapturePreconnectsHttpProxySocketPool* http_proxy_pool =
+            new CapturePreconnectsHttpProxySocketPool(
+                session_deps.host_resolver.get(),
+                session_deps.cert_verifier.get(),
+                session_deps.transport_security_state.get(),
+                session_deps.cert_transparency_verifier.get(),
+                session_deps.ct_policy_enforcer.get());
+        CapturePreconnectsSSLSocketPool* ssl_conn_pool =
+            new CapturePreconnectsSSLSocketPool(
+                session_deps.host_resolver.get(),
+                session_deps.cert_verifier.get(),
+                session_deps.transport_security_state.get(),
+                session_deps.cert_transparency_verifier.get(),
+                session_deps.ct_policy_enforcer.get());
+
+        std::unique_ptr<MockClientSocketPoolManager> mock_pool_manager(
+            new MockClientSocketPoolManager);
+        mock_pool_manager->SetSocketPoolForHTTPProxy(
+            proxy_host, base::WrapUnique(http_proxy_pool));
+        mock_pool_manager->SetSocketPoolForSSLWithProxy(
+            proxy_host, base::WrapUnique(ssl_conn_pool));
+        peer.SetClientSocketPoolManager(std::move(mock_pool_manager));
+
+        HttpRequestInfo request;
+        request.method = "GET";
+        request.url = url;
+        request.load_flags = 0;
+
+        if (preconnect_request == 0) {
+          // First preconnect job should succeed.
+          session->http_stream_factory()->PreconnectStreams(num_streams,
+                                                            request);
+          EXPECT_EQ(-1, ssl_conn_pool->last_num_streams());
+          EXPECT_EQ(num_streams, http_proxy_pool->last_num_streams());
+        } else {
+          // Second preconnect job should not succeed.
+          session->http_stream_factory()->PreconnectStreams(num_streams,
+                                                            request);
+          EXPECT_EQ(-1, ssl_conn_pool->last_num_streams());
+          if (set_http_server_properties) {
+            EXPECT_EQ(-1, http_proxy_pool->last_num_streams());
+          } else {
+            EXPECT_EQ(num_streams, http_proxy_pool->last_num_streams());
+          }
+        }
+      }
+      // Preconnects should be skipped only to proxy servers that support
+      // request priorities.
+      if (set_http_server_properties) {
+        histogram_tester.ExpectUniqueSample(
+            "Net.PreconnectSkippedToProxyServers", 1, 1);
+      } else {
+       
histogram_tester.ExpectTotalCount("Net.PreconnectSkippedToProxyServers",
+                                          0);
+      }
+    }
+  }
+}
+
 namespace {
 
 TEST_F(HttpStreamFactoryTest, PrivacyModeDisablesChannelId) {
@@ -1509,6 +1607,91 @@ TEST_F(HttpStreamFactoryTest, RequestHttpStreamOverProxy)
{
   EXPECT_FALSE(waiter.used_proxy_info().is_direct());
 }
 
+// Verifies that once a stream has been created to a proxy server (that
supports
+// request priorities) the next preconnect job can again open new sockets.
+TEST_F(HttpStreamFactoryTest, RequestHttpStreamOverProxyWithPreconnects) {
+  base::FieldTrialList field_trial_list(nullptr);
+  base::FieldTrialList::CreateFieldTrial(
+      "NetAllowOnlyOnePreconnectToProxyServers", "Enabled");
+
+  SpdySessionDependencies session_deps(
+      ProxyService::CreateFixed("https://myproxy.org:443"));
+
+  // Set up the proxy server as a server that supports request priorities.
+  std::unique_ptr<HttpServerPropertiesImpl> http_server_properties(
+      new HttpServerPropertiesImpl());
+  url::SchemeHostPort spdy_server("https", "myproxy.org", 443);
+  http_server_properties->SetSupportsSpdy(spdy_server, true);
+  session_deps.http_server_properties = std::move(http_server_properties);
+
+  StaticSocketDataProvider socket_data;
+  socket_data.set_connect_data(MockConnect(ASYNC, OK));
+  session_deps.socket_factory->AddSocketDataProvider(&socket_data);
+
+  SSLSocketDataProvider ssl_data(ASYNC, OK);
+  session_deps.socket_factory->AddSSLSocketDataProvider(&ssl_data);
+
+  std::unique_ptr<HttpNetworkSession> session(
+      SpdySessionDependencies::SpdyCreateSession(&session_deps));
+
+  // Now preconnect a request. Only the first preconnect should succeed.
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("http://www.google.com");
+  request_info.load_flags = 0;
+
+  base::HistogramTester histogram_tester;
+  const int num_preconnects = 5;
+
+  // First preconnect would be successful. The next |num_preconnects-1| would
be
+  // skipped.
+  for (int preconnect_request = 1; preconnect_request <= num_preconnects;
+       ++preconnect_request) {
+    session->http_stream_factory()->PreconnectStreams(1, request_info);
+    base::RunLoop().RunUntilIdle();
+    while (GetSocketPoolGroupCount(session->GetSocketPoolForHTTPProxy(
+               HttpNetworkSession::NORMAL_SOCKET_POOL,
+               HostPortPair("myproxy.org", 443))) == 0) {
+      base::RunLoop().RunUntilIdle();
+    }
+  }
+
+  histogram_tester.ExpectUniqueSample("Net.PreconnectSkippedToProxyServers", 1,
+                                      num_preconnects - 1);
+
+  // Start a request. This should cause subsequent preconnect to proxy server
to
+  // succeed.
+  SSLConfig ssl_config;
+  StreamRequestWaiter waiter;
+  std::unique_ptr<HttpStreamRequest> request(
+      session->http_stream_factory()->RequestStream(
+          request_info, DEFAULT_PRIORITY, ssl_config, ssl_config, &waiter,
+          NetLogWithSource()));
+  waiter.WaitForStream();
+  EXPECT_TRUE(waiter.stream_done());
+  ASSERT_TRUE(nullptr != waiter.stream());
+  EXPECT_TRUE(nullptr == waiter.websocket_stream());
+
+  EXPECT_EQ(1, GetSocketPoolGroupCount(session->GetSocketPoolForHTTPProxy(
+                   HttpNetworkSession::NORMAL_SOCKET_POOL,
+                   HostPortPair("myproxy.org", 443))));
+  EXPECT_FALSE(waiter.used_proxy_info().is_direct());
+
+  for (int preconnect_request = 1; preconnect_request <= num_preconnects;
+       ++preconnect_request) {
+    session->http_stream_factory()->PreconnectStreams(1, request_info);
+    base::RunLoop().RunUntilIdle();
+    EXPECT_EQ(1, GetSocketPoolGroupCount(session->GetSocketPoolForHTTPProxy(
+                     HttpNetworkSession::NORMAL_SOCKET_POOL,
+                     HostPortPair("myproxy.org", 443))));
+  }
+
+  // First preconnect would be successful since the stream to the proxy server
+  // has suceeded. The next |num_preconnects-1| would be skipped.
+  histogram_tester.ExpectUniqueSample("Net.PreconnectSkippedToProxyServers", 1,
+                                      2 * (num_preconnects - 1));
+}
+
 TEST_F(HttpStreamFactoryTest, RequestWebSocketBasicHandshakeStream) {
   SpdySessionDependencies session_deps(ProxyService::CreateDirect());
 
@@ -1643,7 +1826,7 @@ TEST_F(HttpStreamFactoryTest,
RequestWebSocketBasicHandsha…
(message too large)

tbansal1

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

4 years ago (2016-12-13 04:03:25 UTC) #103

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2522703002/490001

4 years ago (2016-12-13 04:03:46 UTC) #104

commit-bot: I haz the power

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

4 years ago (2016-12-13 05:32:24 UTC) #105

tbansal1

The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org, rch@chromium.org Link to ...

4 years ago (2016-12-13 15:53:49 UTC) #108

commit-bot: I haz the power

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

4 years ago (2016-12-13 15:54:18 UTC) #109

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 490001, "attempt_start_ts": 1481644428922670, "parent_rev": "a9557022621bff52be1e64d57bc8a2cfa515a2fd", "commit_rev": "2ef515de44342bc650fd30e565667634a2cb8420"}

4 years ago (2016-12-13 16:00:07 UTC) #110

commit-bot: I haz the power

Description was changed from ========== Allow at most one preconnect to HTTP2 proxy servers If ...

4 years ago (2016-12-13 16:00:33 UTC) #111

commit-bot: I haz the power

Description was changed from ========== Allow at most one preconnect to HTTP2 proxy servers If ...

4 years ago (2016-12-13 16:03:23 UTC) #113

commit-bot: I haz the power

4 years ago (2016-12-13 16:03:24 UTC) #114

Message was sent while issue was closed.

Patchset 11 (id:??) landed as
https://crrev.com/0844a896c0334b3541315e79d1048789ea14cc71
Cr-Commit-Position: refs/heads/master@{#438179}

Issue 2522703002: Allow at most one preconnect to HTTP2 proxy servers (Closed)

Description

Patch Set 1 : ps #

Patch Set 2 : Addressed comments #

Patch Set 3 : Using http stream factory (still need to add tests) #

Patch Set 4 : Cleanup, added tests #

Patch Set 5 : rch comments #

Patch Set 6 : rch comments, moved all the logic to factory #

Patch Set 7 : rebased, rch comments #

Patch Set 8 : rch comments, add DCHECKs #

Patch Set 9 : rch comments #

Patch Set 10 : ps #

Patch Set 11 : rebased #

Messages