Basic HTTP pipelining support

Basic HTTP pipelining support.

This must be enabled in about:flags. It is naive and assumes all servers correctly implement pipelining. Proxies are not supported.

Immediate future work:
- Integration tests.
- Additional NetLog logging.
- Refactor HttpPipelinedConnectionImpl.

Long term:
- Detect broken transparent proxies.
- Detect and/or mitigate broken servers.
- Buffer HttpPipelinedStream.
- Optimize number of pipelines and their depth.
- Support proxies.

BUG=8991
TEST=net_unittests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=106364

[willchan no longer on Chromium, 2011-07-06 07:01:53]

Hi! I'm glad to work has started on this. It actually looks remarkably like how
I had imagined it would when I designed the HTTP stream framework way back.
There are obviously going to be a ton of edge cases, which I haven't had a
chance to look into yet. But let me give you some high level comments to help
you structure things.

* Is there a reason you renamed HttpStreamFactoryImpl::Job to HttpConnectionJob?
It seems unnecessary and even incorrect. Here's the general concept.
HttpNetworkTransaction wants to do a request/response pair, a networked HTTP
transaction. It needs some object to call SendRequest() on and get an HTTP
response. For lack of a better term, we call that a HttpStream. So, the
HttpStreamFactory accepts requests (HttpStreamRequests) and fulfills them with
Jobs (HttpStreamFactoryImpl::Job). HttpConnectionJob makes this unclear. It's
also too specific, because the connection may be on top of SPDY or whatever
else. Also note that keeping the same class nesting that I used previously will
obviate your need for friending.

* It looks like at the request time, you determine whether or not you want to
make this pipelined. I argue that this is the wrong time. I think you should
have kept the HttpStreamFactoryImpl::Job mostly as is wrt that. When the
connection is complete, you can inspect it to see if a SPDY connection was
created. If so, we create a SpdyHttpStream. If not, then it's a HTTP stream of
some sort. At this point, if the host is a viable HTTP pipelining host according
to our heuristics, then I'd contribute the connection to the the map of
pipelining connections and create a HttpPipelinedStream from it. If you
structure the code in this way, you shouldn't need your compile time option, it
should be easy at the point where you see that it's not a SPDY connection, to
check a command line flag to see whether or not you want to attempt using the
connection for pipelining or not.

* I didn't look super carefully, but I'm not sure if you've enabled late binding
for pipelining. I'll need to read it again to see. That's pretty important.

That should be enough for now to get you moving closer. There are a lot more
comments I'll have wrt testing and NetLog and what not, but try this for now :)

[James Simonsen, 2011-07-14 04:29:14]

Hi Will, Thanks for your feedback. I definitely need guidance on this, since I'm
new to the networking code.

On 2011/07/06 07:01:53, willchan wrote:
> Hi! I'm glad to work has started on this. It actually looks remarkably like
how
> I had imagined it would when I designed the HTTP stream framework way back.
> There are obviously going to be a ton of edge cases, which I haven't had a
> chance to look into yet. But let me give you some high level comments to help
> you structure things.
> 
> * Is there a reason you renamed HttpStreamFactoryImpl::Job to
HttpConnectionJob?
> It seems unnecessary and even incorrect. Here's the general concept.
> HttpNetworkTransaction wants to do a request/response pair, a networked HTTP
> transaction. It needs some object to call SendRequest() on and get an HTTP
> response. For lack of a better term, we call that a HttpStream. So, the
> HttpStreamFactory accepts requests (HttpStreamRequests) and fulfills them with
> Jobs (HttpStreamFactoryImpl::Job). HttpConnectionJob makes this unclear. It's
> also too specific, because the connection may be on top of SPDY or whatever
> else. Also note that keeping the same class nesting that I used previously
will
> obviate your need for friending.

When a Request comes in, we're not always sure what the fastest way to service
it is. If we have a good pipeline with capacity, then we should assign it to
that. If we don't have a pipeline ready, we would want to kick off a new
HttpConnectionJob. However, before that job completes, a pipeline might open up.
In that case, we'd prefer to use the existing warm pipeline. On the flip side, a
pipeline we previously thought would work might be closed and then we'd need to
open a new connection to replace it.

This doesn't really fit into the HttpStreamFactoryImpl::Job state machine, since
it's influenced by other connections. Instead, I viewed HttpConnectionJob as
more of a get-me-a-fully-negotiated-connection job. Then HttpPipelinedHost
manages the decisions of when to open new connections, abandon jobs, and create
HttpPipelinedStreams.

One idea would be to create an AttemptToReuseExistingPipelineJob as an alternate
job for the request. It'd basically be a proxy to the HttpPipelinedHost, which
would still manage the list of pipelines. If a pipeline were available, then the
new job would complete before the normal HttpStreamFactoryImpl::Job. If not,
it'd just go away when it was orphaned. This probably fits best with the
existing code.

Another idea would be to give the Factory all of these smarts. Maybe changing
its name to something like HostConnectionManager. It would be responsible for
kicking off connection jobs and then creating the best Stream for each Request
based on the established connections. That's already kind of what my
HttpPipelinedHost does, but we'd expand it to handle all the connection types
(SPDY, pipelining, and basic HTTP). I noticed a TODO in
HttpStreamFactoryImpl::Job about moving some of the SPDY stuff out. I think this
would fit well with that.

Any other ideas? What do you like best?

> * It looks like at the request time, you determine whether or not you want to
> make this pipelined. I argue that this is the wrong time. I think you should
> have kept the HttpStreamFactoryImpl::Job mostly as is wrt that. When the
> connection is complete, you can inspect it to see if a SPDY connection was
> created. If so, we create a SpdyHttpStream. If not, then it's a HTTP stream of
> some sort. At this point, if the host is a viable HTTP pipelining host
according
> to our heuristics, then I'd contribute the connection to the the map of
> pipelining connections and create a HttpPipelinedStream from it. If you
> structure the code in this way, you shouldn't need your compile time option,
it
> should be easy at the point where you see that it's not a SPDY connection, to
> check a command line flag to see whether or not you want to attempt using the
> connection for pipelining or not.

This mostly depends on what we do about the above point. If we decide up front
whether or not a new connection is needed, then the above works out well.

> * I didn't look super carefully, but I'm not sure if you've enabled late
binding
> for pipelining. I'll need to read it again to see. That's pretty important.

I'm not sure exactly what you mean here. I think you're asking if we can upgrade
a connection to pipelining late. If so, the answer is no, it's the other way
around. We try pipelining first and downgrade if it fails. Note that the
downgrading code isn't in place yet, we just naively try to pipeline again
starting with the next request.

> That should be enough for now to get you moving closer. There are a lot more
> comments I'll have wrt testing and NetLog and what not, but try this for now
:)

Yep. I definitely want your feedback on those when we get to that point.

[willchan no longer on Chromium, 2011-07-14 16:38:55]

On 2011/07/14 04:29:14, James Simonsen wrote:
> Hi Will, Thanks for your feedback. I definitely need guidance on this, since
I'm
> new to the networking code.
> 
> On 2011/07/06 07:01:53, willchan wrote:
> > Hi! I'm glad to work has started on this. It actually looks remarkably like
> how
> > I had imagined it would when I designed the HTTP stream framework way back.
> > There are obviously going to be a ton of edge cases, which I haven't had a
> > chance to look into yet. But let me give you some high level comments to
help
> > you structure things.
> > 
> > * Is there a reason you renamed HttpStreamFactoryImpl::Job to
> HttpConnectionJob?
> > It seems unnecessary and even incorrect. Here's the general concept.
> > HttpNetworkTransaction wants to do a request/response pair, a networked HTTP
> > transaction. It needs some object to call SendRequest() on and get an HTTP
> > response. For lack of a better term, we call that a HttpStream. So, the
> > HttpStreamFactory accepts requests (HttpStreamRequests) and fulfills them
with
> > Jobs (HttpStreamFactoryImpl::Job). HttpConnectionJob makes this unclear.
It's
> > also too specific, because the connection may be on top of SPDY or whatever
> > else. Also note that keeping the same class nesting that I used previously
> will
> > obviate your need for friending.
> 
> When a Request comes in, we're not always sure what the fastest way to service
> it is. If we have a good pipeline with capacity, then we should assign it to
> that. If we don't have a pipeline ready, we would want to kick off a new
> HttpConnectionJob. However, before that job completes, a pipeline might open
up.
> In that case, we'd prefer to use the existing warm pipeline. On the flip side,
a
> pipeline we previously thought would work might be closed and then we'd need
to
> open a new connection to replace it.
> 
> This doesn't really fit into the HttpStreamFactoryImpl::Job state machine,
since
> it's influenced by other connections. Instead, I viewed HttpConnectionJob as
> more of a get-me-a-fully-negotiated-connection job. Then HttpPipelinedHost
> manages the decisions of when to open new connections, abandon jobs, and
create
> HttpPipelinedStreams.

I'm not sure I understand this point. We need a Job to first resolve the proxy
to figure out which of our connection pools to examine (socket pools or SPDY
connection pools, and in the new pipelined world, some pipeline connection
manager too). HttpStreamFactoryImpl::Job is already influenced by other
connections. Since it is built on top of our connection pools, it's very
fundamentally affected by other connections.

I think you have the mental conception that Job implies that it is always
establishing a new connection or something. It actually just figures out which
connection pool to use and defers the actual connection logic to that connection
pool.

> 
> One idea would be to create an AttemptToReuseExistingPipelineJob as an
alternate
> job for the request. It'd basically be a proxy to the HttpPipelinedHost, which
> would still manage the list of pipelines. If a pipeline were available, then
the
> new job would complete before the normal HttpStreamFactoryImpl::Job. If not,
> it'd just go away when it was orphaned. This probably fits best with the
> existing code.
> 
> Another idea would be to give the Factory all of these smarts. Maybe changing
> its name to something like HostConnectionManager. It would be responsible for
> kicking off connection jobs and then creating the best Stream for each Request
> based on the established connections. That's already kind of what my
> HttpPipelinedHost does, but we'd expand it to handle all the connection types
> (SPDY, pipelining, and basic HTTP). I noticed a TODO in
> HttpStreamFactoryImpl::Job about moving some of the SPDY stuff out. I think
this
> would fit well with that.

Job actually does all of this currently, except it doesn't handle the pipelined
connections. It has all the smarts that you refer to.

> 
> Any other ideas? What do you like best?
> 
> > * It looks like at the request time, you determine whether or not you want
to
> > make this pipelined. I argue that this is the wrong time. I think you should
> > have kept the HttpStreamFactoryImpl::Job mostly as is wrt that. When the
> > connection is complete, you can inspect it to see if a SPDY connection was
> > created. If so, we create a SpdyHttpStream. If not, then it's a HTTP stream
of
> > some sort. At this point, if the host is a viable HTTP pipelining host
> according
> > to our heuristics, then I'd contribute the connection to the the map of
> > pipelining connections and create a HttpPipelinedStream from it. If you
> > structure the code in this way, you shouldn't need your compile time option,
> it
> > should be easy at the point where you see that it's not a SPDY connection,
to
> > check a command line flag to see whether or not you want to attempt using
the
> > connection for pipelining or not.
> 
> This mostly depends on what we do about the above point. If we decide up front
> whether or not a new connection is needed, then the above works out well.

We don't want to do a tight binding of a request to a connection in the process
of being established. We want to late bind requests to jobs, where a job can
either use an existing SpdySession, reuse an HTTP keep alive connection,
establish a new connection from scratch, or use a new connection that was
already being established by another request that got canceled or started up by
the preconnect subsystem, or an available slot in a pipelined connection.

> 
> > * I didn't look super carefully, but I'm not sure if you've enabled late
> binding
> > for pipelining. I'll need to read it again to see. That's pretty important.
> 
> I'm not sure exactly what you mean here. I think you're asking if we can
upgrade
> a connection to pipelining late. If so, the answer is no, it's the other way
> around. We try pipelining first and downgrade if it fails. Note that the
> downgrading code isn't in place yet, we just naively try to pipeline again
> starting with the next request.
> 
> > That should be enough for now to get you moving closer. There are a lot more
> > comments I'll have wrt testing and NetLog and what not, but try this for now
> :)
> 
> Yep. I definitely want your feedback on those when we get to that point.

[James Simonsen, 2011-07-15 05:23:10]

Thanks for your help, I think I understand everything better.

On 2011/07/14 16:38:55, willchan wrote:
> We don't want to do a tight binding of a request to a connection in the
process
> of being established. We want to late bind requests to jobs, where a job can
> either use an existing SpdySession, reuse an HTTP keep alive connection,
> establish a new connection from scratch, or use a new connection that was
> already being established by another request that got canceled or started up
by
> the preconnect subsystem, or an available slot in a pipelined connection.

Yeah, I see how all of the SPDY stuff works now. I needed to follow all of the
code paths to see how it short-circuits out of opening duplicate SPDY sessions.
Pipelining has the same requirement. I was concerned about dividing that
responsibility between the job and the factory, so I was trying to find a way to
do it all in one place.

I see now that pipelining can work very similarly to SPDY. I'll hack this up on
Friday. Thanks again for your help.

[willchan no longer on Chromium, 2011-07-15 06:03:18]

On Fri, Jul 15, 2011 at 8:23 AM,  <simonjam@chromium.org> wrote:
> Thanks for your help, I think I understand everything better.

Great!

>
> On 2011/07/14 16:38:55, willchan wrote:
>>
>> We don't want to do a tight binding of a request to a connection in the
>
> process
>>
>> of being established. We want to late bind requests to jobs, where a job
>> can
>> either use an existing SpdySession, reuse an HTTP keep alive connection,
>> establish a new connection from scratch, or use a new connection that was
>> already being established by another request that got canceled or started
>> up
>
> by
>>
>> the preconnect subsystem, or an available slot in a pipelined connection.
>
> Yeah, I see how all of the SPDY stuff works now. I needed to follow all of
> the
> code paths to see how it short-circuits out of opening duplicate SPDY
> sessions.
> Pipelining has the same requirement. I was concerned about dividing that
> responsibility between the job and the factory, so I was trying to find a
> way to
> do it all in one place.
>
> I see now that pipelining can work very similarly to SPDY. I'll hack this up
> on
> Friday. Thanks again for your help.
>
> http://codereview.chromium.org/7289006/
>

[James Simonsen, 2011-07-17 01:39:30]

Alright, I've updated it to follow the general pattern that SPDY uses. Would you
mind taking another look?

[willchan no longer on Chromium, 2011-07-18 13:03:21]

On 2011/07/17 01:39:30, James Simonsen wrote:
> Alright, I've updated it to follow the general pattern that SPDY uses. Would
you
> mind taking another look?

http_stream_factory_impl_job.* still looks to be deleted, but your code seems to
reference it still. Is this a gcl/git use error?

[James Simonsen, 2011-07-18 19:25:23]

On 2011/07/18 13:03:21, willchan wrote:
> On 2011/07/17 01:39:30, James Simonsen wrote:
> > Alright, I've updated it to follow the general pattern that SPDY uses. Would
> you
> > mind taking another look?
> 
> http_stream_factory_impl_job.* still looks to be deleted, but your code seems
to
> reference it still. Is this a gcl/git use error?

It looks like a bug in the UI. If you actually click on "View" to see the diffs,
it shows the correct set of changes to those files.

[willchan no longer on Chromium, 2011-07-19 13:29:38]

I've started looking at this, but this will require a much more detailed look.
This version is much closer to what I had in mind. Thanks for the cleanup! I've
added the suggestion for tying it into about:flags now. That will help people
experiment with it.

Next steps I will be looking into, which you can look into preemptively:
* How to tie in NetLog
* How to unit test, probably will require some design finessing to have mockable
interfaces and cut down on dependencies.
* Detailed review for bugs

After that, I think it will be ok to land this changelist so we can begin dev
experimentation and benchmarking and what not, while we continue working on
handling all the fugly corner cases wrt pipelining (cancellation, fallback,
flushing the pipelines, etc).

http://codereview.chromium.org/7289006/diff/6001/net/http/http_pipelined_host...
File net/http/http_pipelined_host_pool.h (right):

http://codereview.chromium.org/7289006/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_pool.h:25: HttpPipelinedHostPool(HttpStreamFactory*
factory);
explicit

http://codereview.chromium.org/7289006/diff/6001/net/http/http_stream_factory...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/7289006/diff/6001/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.cc:778: !existing_available_pipeline_)
Heh, this is used to debug bug 80095. You probably shouldn't use it. You
probably want a DCHECK(connection_->socket() || existing_spdy_session_ ||
existing_available_pipeline_); Put that on the next line, then later on when I
remove the HACK function, your assertion will be the valid one.

http://codereview.chromium.org/7289006/diff/6001/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.cc:799:
GetOrCreatePipelinedHost(origin_)->CreateNewPipeline(
I need to look at this a bit more. This is pretty key code. Currently this code
seems to make the assumption that for all hosts, we can pipeline requests to
them. Obviously, we'll be tying in heuristics here, and also need to check to
see if we already tried to pipeline to this host, and if it failed, we shouldn't
retry. Lots of logic should be going in around this location.

http://codereview.chromium.org/7289006/diff/6001/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.cc:1108: bool
HttpStreamFactoryImpl::Job::IsRequestEligibleForPipelining() const {
So, we probably want to expose pipelining via about:flags. That will let us set
a command line flag for this feature. You should correlate that command line
flag to a global variable here. When the command line flag is set to true, then
a global variable in this code should be set to true too. Note that command line
flags only exist in chrome/. For an example of how to do this, please refer to
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/browse....
That doesn't show how to tie it into about:flags, but you can probably read
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/about_...
and figure it out pretty easily (I've never done it myself, but it looks easy).

http://codereview.chromium.org/7289006/diff/6001/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.cc:1112: return request_info_.method ==
"GET";
Allow other idempotent methods like HEAD?

[James Simonsen, 2011-07-19 22:54:23]

I'll do unit tests next, but I'm a little distracted right now, so it may be
next week.

http://codereview.chromium.org/7289006/diff/6001/net/http/http_pipelined_host...
File net/http/http_pipelined_host_pool.h (right):

http://codereview.chromium.org/7289006/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_pool.h:25: HttpPipelinedHostPool(HttpStreamFactory*
factory);
On 2011/07/19 13:29:38, willchan wrote:
> explicit

Done.

http://codereview.chromium.org/7289006/diff/6001/net/http/http_stream_factory...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/7289006/diff/6001/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.cc:778: !existing_available_pipeline_)
On 2011/07/19 13:29:38, willchan wrote:
> Heh, this is used to debug bug 80095. You probably shouldn't use it. You
> probably want a DCHECK(connection_->socket() || existing_spdy_session_ ||
> existing_available_pipeline_); Put that on the next line, then later on when I
> remove the HACK function, your assertion will be the valid one.

Done, but I still need to avoid falling into the HACK when I use an existing
pipeline, so I still need to modify the if().

http://codereview.chromium.org/7289006/diff/6001/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.cc:799:
GetOrCreatePipelinedHost(origin_)->CreateNewPipeline(
On 2011/07/19 13:29:38, willchan wrote:
> I need to look at this a bit more. This is pretty key code. Currently this
code
> seems to make the assumption that for all hosts, we can pipeline requests to
> them. Obviously, we'll be tying in heuristics here, and also need to check to
> see if we already tried to pipeline to this host, and if it failed, we
shouldn't
> retry. Lots of logic should be going in around this location.

Yeah, I'm still not sure exactly what we all need. For instance, will Google
keep a list of good/bad sites or will each client build their own? Anyway, I
don't intend to figure this out until we have some actual data.

http://codereview.chromium.org/7289006/diff/6001/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.cc:1108: bool
HttpStreamFactoryImpl::Job::IsRequestEligibleForPipelining() const {
On 2011/07/19 13:29:38, willchan wrote:
> So, we probably want to expose pipelining via about:flags. That will let us
set
> a command line flag for this feature. You should correlate that command line
> flag to a global variable here. When the command line flag is set to true,
then
> a global variable in this code should be set to true too. Note that command
line
> flags only exist in chrome/. For an example of how to do this, please refer to
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/browse....
> That doesn't show how to tie it into about:flags, but you can probably read
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/about_...
> and figure it out pretty easily (I've never done it myself, but it looks
easy).

Done. Yep, easy.

http://codereview.chromium.org/7289006/diff/6001/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.cc:1112: return request_info_.method ==
"GET";
On 2011/07/19 13:29:38, willchan wrote:
> Allow other idempotent methods like HEAD?

Done. HEAD seems the only other safe and useful one right now.

[mmenke, 2011-08-03 21:09:39]

Chris suggested that I help review this CL, with Will taking lead.  A couple
preliminary comments.

http://codereview.chromium.org/7289006/diff/12001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection.cc (right):

http://codereview.chromium.org/7289006/diff/12001/net/http/http_pipelined_con...
net/http/http_pipelined_connection.cc:230: if
(stream_state_map_[evicted_send.pipeline_id] == STREAM_CLOSED) {
This case will result in an infinite loop without a 
deferred_request_queue_.pop().

http://codereview.chromium.org/7289006/diff/12001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection.h (right):

http://codereview.chromium.org/7289006/diff/12001/net/http/http_pipelined_con...
net/http/http_pipelined_connection.h:5: // This class manages all of the state
for a single pipelined connection. It
nit:  All this should go just before the class, rather than up here.

http://codereview.chromium.org/7289006/diff/12001/net/http/http_pipelined_con...
net/http/http_pipelined_connection.h:60: // True if this pipeline has bound one
request is ready for additional
nit:  "has bound one request and...".

http://codereview.chromium.org/7289006/diff/12001/net/http/http_pipelined_con...
net/http/http_pipelined_connection.h:157: int DoSendNextRequest(int result);
It'd great to have comments on what these functions are called in response to,
and what they actually do.

http://codereview.chromium.org/7289006/diff/12001/net/http/http_pipelined_hos...
File net/http/http_pipelined_host_pool.cc (right):

http://codereview.chromium.org/7289006/diff/12001/net/http/http_pipelined_hos...
net/http/http_pipelined_host_pool.cc:15: : factory_(factory) {
nit:  Should be four spaces.

http://codereview.chromium.org/7289006/diff/12001/net/http/http_pipelined_str...
File net/http/http_pipelined_stream.h (right):

http://codereview.chromium.org/7289006/diff/12001/net/http/http_pipelined_str...
net/http/http_pipelined_stream.h:5: // HttpPipelinedStream is the pipelined
implementation of HttpStream. It has
nit:  Class docs should go above class HttpPipelinedStream.

http://codereview.chromium.org/7289006/diff/12001/net/http/http_pipelined_str...
net/http/http_pipelined_stream.h:42: CompletionCallback* callback);
nit:  Should use OVERRIDE with overridden functions.

http://codereview.chromium.org/7289006/diff/12001/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/7289006/diff/12001/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:589: } else if
(IsRequestEligibleForPipelining()) {
Looks to me like you're implicitly not using pipelining for SSL connections
here, due to the above |using_ssl_| check.  If that's deliberate, might want to
just use HttpBasicStream for SSL connections below.

http://codereview.chromium.org/7289006/diff/12001/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_request.cc (right):

http://codereview.chromium.org/7289006/diff/12001/net/http/http_stream_factor...
net/http/http_stream_factory_impl_request.cc:98: // SpdySession (one that was
not created by a job in our |jobs_| set).
nit:  Update comment.

[James Simonsen, 2011-08-05 01:39:00]

Thanks for the additional feedback!

I've also added unit tests and fixed any associated bugs.

http://codereview.chromium.org/7289006/diff/12001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection.cc (right):

http://codereview.chromium.org/7289006/diff/12001/net/http/http_pipelined_con...
net/http/http_pipelined_connection.cc:230: if
(stream_state_map_[evicted_send.pipeline_id] == STREAM_CLOSED) {
On 2011/08/03 21:09:39, Matt Menke wrote:
> This case will result in an infinite loop without a 
> deferred_request_queue_.pop().

Done. Added a unit test too.

http://codereview.chromium.org/7289006/diff/12001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection.h (right):

http://codereview.chromium.org/7289006/diff/12001/net/http/http_pipelined_con...
net/http/http_pipelined_connection.h:5: // This class manages all of the state
for a single pipelined connection. It
On 2011/08/03 21:09:39, Matt Menke wrote:
> nit:  All this should go just before the class, rather than up here.

Done.

http://codereview.chromium.org/7289006/diff/12001/net/http/http_pipelined_con...
net/http/http_pipelined_connection.h:60: // True if this pipeline has bound one
request is ready for additional
On 2011/08/03 21:09:39, Matt Menke wrote:
> nit:  "has bound one request and...".

Done.

http://codereview.chromium.org/7289006/diff/12001/net/http/http_pipelined_con...
net/http/http_pipelined_connection.h:157: int DoSendNextRequest(int result);
On 2011/08/03 21:09:39, Matt Menke wrote:
> It'd great to have comments on what these functions are called in response to,
> and what they actually do.

Done.

http://codereview.chromium.org/7289006/diff/12001/net/http/http_pipelined_hos...
File net/http/http_pipelined_host_pool.cc (right):

http://codereview.chromium.org/7289006/diff/12001/net/http/http_pipelined_hos...
net/http/http_pipelined_host_pool.cc:15: : factory_(factory) {
On 2011/08/03 21:09:39, Matt Menke wrote:
> nit:  Should be four spaces.

Done.

http://codereview.chromium.org/7289006/diff/12001/net/http/http_pipelined_str...
File net/http/http_pipelined_stream.h (right):

http://codereview.chromium.org/7289006/diff/12001/net/http/http_pipelined_str...
net/http/http_pipelined_stream.h:5: // HttpPipelinedStream is the pipelined
implementation of HttpStream. It has
On 2011/08/03 21:09:39, Matt Menke wrote:
> nit:  Class docs should go above class HttpPipelinedStream.

Done.

http://codereview.chromium.org/7289006/diff/12001/net/http/http_pipelined_str...
net/http/http_pipelined_stream.h:42: CompletionCallback* callback);
On 2011/08/03 21:09:39, Matt Menke wrote:
> nit:  Should use OVERRIDE with overridden functions.

Done.

http://codereview.chromium.org/7289006/diff/12001/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/7289006/diff/12001/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:589: } else if
(IsRequestEligibleForPipelining()) {
On 2011/08/03 21:09:39, Matt Menke wrote:
> Looks to me like you're implicitly not using pipelining for SSL connections
> here, due to the above |using_ssl_| check.  If that's deliberate, might want
to
> just use HttpBasicStream for SSL connections below.

Done.

http://codereview.chromium.org/7289006/diff/12001/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_request.cc (right):

http://codereview.chromium.org/7289006/diff/12001/net/http/http_stream_factor...
net/http/http_stream_factory_impl_request.cc:98: // SpdySession (one that was
not created by a job in our |jobs_| set).
On 2011/08/03 21:09:39, Matt Menke wrote:
> nit:  Update comment.

Done.

[mmenke, 2011-08-23 19:05:25]

Sorry I'm so late getting back to you.

There are huge swaths of the network stack that I'm completely unfamiliar with,
and I've just been putting off doing the reading necessary to get a better
understanding of how this CL fits into things.  I'll be much faster with the
next round.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection.h (right):

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection.h:57: virtual bool active() const= 0;
nit:  space

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection_impl.cc (right):

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:10: #include
"net/http/http_pipelined_host.h"
nit:  No longer needed.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:101:
DCHECK_EQ(stream_state_map_[pipeline_id], STREAM_CLOSED);
A BOUND_STREAM can currently end up here on ERR_PIPELINE_EVICTION (See the start
of SendRequest).  Same with STREAM_SENT.

Should probably have some tests delete streams right after they error out.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:195: if (result == ERR_IO_PENDING ||
rv == ERR_IO_PENDING) {
nit:  Add a comment that |result| == ERR_IO_PENDING implies that we're being
called in a task posted from DoSendComplete(), rather than directly from
SendRequest.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:322: rv));
Is there any guarantee that a stream won't be deleted from here until
FireUserCallback is called?  I'm not aware of one.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection_impl.h (right):

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.h:27: class HttpPipelinedHost;
nit:  No longer needed.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.h:59: int depth() const OVERRIDE {
return stream_state_map_.size(); }
virtual should always be used with OVERRIDE.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.h:161: // DoSendComplete().
Thanks for adding all these comments.  Makes it much easier to follow the flow
of the code.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.h:170: // error or one of our sockets
informs us the connection is no longer
nit:  "One of our streams"

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.h:182: // Determines if the next
response in the pipeline is ready to be read. A
nit:  Remove "A" at end of line.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection_impl_unittest.cc (right):

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl_unittest.cc:7: #include <string>
nit:  Add blank link.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl_unittest.cc:274:
TEST_F(HttpPipelinedConnectionImplTest, SendOrderSwapped) {
How is this different from the previous test?

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl_unittest.cc:434:
TEST_F(HttpPipelinedConnectionImplTest, PipelineNotUsed) {
nit:  Might want to put this test as well as the next 3 up top, as that's
generally simple null-ish tests are put.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl_unittest.cc:435: MockWrite writes[] =
{};
MSVC doesn't like 0-length arrays like this.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl_unittest.cc:616: EXPECT_THAT(actual,
StrEq(expected));
nit:  Instead of the above lines, could just use:

ExpectResponse("ok.html", closed_stream, false);

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl_unittest.cc:675:
TEST_F(HttpPipelinedConnectionImplTest, PendingResponseAbandoned) {
Out of general paranoia, should also have tests where we abandon a stream while
it's writing, both when it's the first stream and when it's the second.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_hos...
File net/http/http_pipelined_host.cc (right):

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_hos...
net/http/http_pipelined_host.cc:16: HttpPipelinedConnection* CreateNewPipeline(
nit:  Indent "public:" 1 space, next line 2 spaces.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_host.h
File net/http/http_pipelined_host.h (right):

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_hos...
net/http/http_pipelined_host.h:7: // assigns requests to the least loaded
pipelined connection.
nit:  Move above HttpPipelinedHost.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_hos...
net/http/http_pipelined_host.h:29: class Owner {
"Delegate" is the preferred term for this kind of thing in the Chrome code base.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_hos...
File net/http/http_pipelined_host_pool.h (right):

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_hos...
net/http/http_pipelined_host_pool.h:39: void OnHostIdle(HttpPipelinedHost* host)
OVERRIDE;
virtual

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_hos...
net/http/http_pipelined_host_pool.h:41: void
OnHostHasAdditionalCapacity(HttpPipelinedHost* host) OVERRIDE;
virtual

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_hos...
File net/http/http_pipelined_host_unittest.cc (right):

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_hos...
net/http/http_pipelined_host_unittest.cc:66: bool active() const OVERRIDE {
return active_; }
nit:  Use virtual as well as OVERRIDE.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_hos...
net/http/http_pipelined_host_unittest.cc:134: .Times(2);
Maybe add:

  EXPECT_CALL(owner_, OnHostIdle(&host_))
      .Times(0);

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_hos...
net/http/http_pipelined_host_unittest.cc:177: 
Maybe add something along the lines of:

TEST_F(HttpPipelinedHostTest, ClearPipeline) {
  MockPipeline* pipeline = AddTestPipeline(0, true, true);

  EXPECT_NE(NULL, host_.FindAvailablePipeline());

  ClearTestPipeline(pipeline);

  EXPECT_EQ(NULL, host_.FindAvailablePipeline());
}

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_str...
File net/http/http_pipelined_stream.h (right):

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_str...
net/http/http_pipelined_stream.h:21: class HttpRequestHeaders;
nit:  Alphabetize.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_request.h (right):

http://codereview.chromium.org/7289006/diff/21001/net/http/http_stream_factor...
net/http/http_stream_factory_impl_request.h:53: // If this Request has a
http_pipelining_key, remove this session from the
nit:  |http_pipelining_key_|

[mmenke, 2011-08-23 19:15:25]

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection_impl.cc (right):

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:211: if (result < OK) {
On ERR_SOCKET_NOT_CONNECTED, we might want to switch to ERR_PIPELINE_EVICTION,
if this wasn't the first sent request, so we'll retry.

[mmenke, 2011-08-23 20:35:12]

Found a crasher while playing with the code.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection_impl.cc (right):

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:444: DCHECK(pipeline_id ==
request_order_.front());
On destruction, an HttpNetworkTransactionuses a HttpResponseBodyDrainer to send
any remaining received data along to the current HttpStream.  When an error
occurs, we'll have evicted all our streams before the HttpNetworkTransaction
gets deleted and flushes all its data, so we'll crash here.  See
HttpNetworkTransaction::~HttpNetworkTransaction.

[James Simonsen, 2011-08-26 22:19:07]

Thanks for the detailed review! I think you really dug up a lot of good edge
cases that would've been painful to debug later.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection.h (right):

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection.h:57: virtual bool active() const= 0;
On 2011/08/23 19:05:25, Matt Menke wrote:
> nit:  space

Done.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection_impl.cc (right):

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:10: #include
"net/http/http_pipelined_host.h"
On 2011/08/23 19:05:25, Matt Menke wrote:
> nit:  No longer needed.

Done.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:101:
DCHECK_EQ(stream_state_map_[pipeline_id], STREAM_CLOSED);
On 2011/08/23 19:05:25, Matt Menke wrote:
> A BOUND_STREAM can currently end up here on ERR_PIPELINE_EVICTION (See the
start
> of SendRequest).  Same with STREAM_SENT.
> 
> Should probably have some tests delete streams right after they error out.

Good catch!

Hmm. I'd actually prefer that Close() always be called. So, I added a call to
that in http_network_transaction.cc.

I may need to revisit this when we handle proxies. I've added a TODO there about
that.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:195: if (result == ERR_IO_PENDING ||
rv == ERR_IO_PENDING) {
On 2011/08/23 19:05:25, Matt Menke wrote:
> nit:  Add a comment that |result| == ERR_IO_PENDING implies that we're being
> called in a task posted from DoSendComplete(), rather than directly from
> SendRequest.

Done.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:211: if (result < OK) {
On 2011/08/23 19:15:25, Matt Menke wrote:
> On ERR_SOCKET_NOT_CONNECTED, we might want to switch to ERR_PIPELINE_EVICTION,
> if this wasn't the first sent request, so we'll retry.

Yeah, good idea. Done. And tested.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:322: rv));
On 2011/08/23 19:05:25, Matt Menke wrote:
> Is there any guarantee that a stream won't be deleted from here until
> FireUserCallback is called?  I'm not aware of one.

Another good catch! Fixed and added tests.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:444: DCHECK(pipeline_id ==
request_order_.front());
On 2011/08/23 20:35:12, Matt Menke wrote:
> On destruction, an HttpNetworkTransactionuses a HttpResponseBodyDrainer to
send
> any remaining received data along to the current HttpStream.  When an error
> occurs, we'll have evicted all our streams before the HttpNetworkTransaction
> gets deleted and flushes all its data, so we'll crash here.  See
> HttpNetworkTransaction::~HttpNetworkTransaction.

Nice. I liked Will's comment and moved it to the stream classes. There's room
for improvement in pipelining, so I added a TODO to revisit this.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection_impl.h (right):

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.h:27: class HttpPipelinedHost;
On 2011/08/23 19:05:25, Matt Menke wrote:
> nit:  No longer needed.

Done.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.h:59: int depth() const OVERRIDE {
return stream_state_map_.size(); }
On 2011/08/23 19:05:25, Matt Menke wrote:
> virtual should always be used with OVERRIDE.

Done.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.h:170: // error or one of our sockets
informs us the connection is no longer
On 2011/08/23 19:05:25, Matt Menke wrote:
> nit:  "One of our streams"

Done.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.h:182: // Determines if the next
response in the pipeline is ready to be read. A
On 2011/08/23 19:05:25, Matt Menke wrote:
> nit:  Remove "A" at end of line.

Done.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection_impl_unittest.cc (right):

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl_unittest.cc:7: #include <string>
On 2011/08/23 19:05:25, Matt Menke wrote:
> nit:  Add blank link.

Done.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl_unittest.cc:274:
TEST_F(HttpPipelinedConnectionImplTest, SendOrderSwapped) {
On 2011/08/23 19:05:25, Matt Menke wrote:
> How is this different from the previous test?

Hmm. It was different at one point, but I oversimplified the code so it no
longer is.

Arguably, the previous test is pretty unlikely to ever break, so I'm just going
to toss it.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl_unittest.cc:434:
TEST_F(HttpPipelinedConnectionImplTest, PipelineNotUsed) {
On 2011/08/23 19:05:25, Matt Menke wrote:
> nit:  Might want to put this test as well as the next 3 up top, as that's
> generally simple null-ish tests are put.

Done.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl_unittest.cc:435: MockWrite writes[] =
{};
On 2011/08/23 19:05:25, Matt Menke wrote:
> MSVC doesn't like 0-length arrays like this.

Lame. Fixed.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl_unittest.cc:616: EXPECT_THAT(actual,
StrEq(expected));
On 2011/08/23 19:05:25, Matt Menke wrote:
> nit:  Instead of the above lines, could just use:
> 
> ExpectResponse("ok.html", closed_stream, false);

Good eye. Fixed.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl_unittest.cc:675:
TEST_F(HttpPipelinedConnectionImplTest, PendingResponseAbandoned) {
On 2011/08/23 19:05:25, Matt Menke wrote:
> Out of general paranoia, should also have tests where we abandon a stream
while
> it's writing, both when it's the first stream and when it's the second.

Nice. Caught a bug too.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_hos...
File net/http/http_pipelined_host.cc (right):

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_hos...
net/http/http_pipelined_host.cc:16: HttpPipelinedConnection* CreateNewPipeline(
On 2011/08/23 19:05:25, Matt Menke wrote:
> nit:  Indent "public:" 1 space, next line 2 spaces.

Done.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_host.h
File net/http/http_pipelined_host.h (right):

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_hos...
net/http/http_pipelined_host.h:7: // assigns requests to the least loaded
pipelined connection.
On 2011/08/23 19:05:25, Matt Menke wrote:
> nit:  Move above HttpPipelinedHost.

Done.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_hos...
net/http/http_pipelined_host.h:29: class Owner {
On 2011/08/23 19:05:25, Matt Menke wrote:
> "Delegate" is the preferred term for this kind of thing in the Chrome code
base.

Done.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_hos...
File net/http/http_pipelined_host_pool.h (right):

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_hos...
net/http/http_pipelined_host_pool.h:39: void OnHostIdle(HttpPipelinedHost* host)
OVERRIDE;
On 2011/08/23 19:05:25, Matt Menke wrote:
> virtual

Done.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_hos...
net/http/http_pipelined_host_pool.h:41: void
OnHostHasAdditionalCapacity(HttpPipelinedHost* host) OVERRIDE;
On 2011/08/23 19:05:25, Matt Menke wrote:
> virtual

Done.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_hos...
File net/http/http_pipelined_host_unittest.cc (right):

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_hos...
net/http/http_pipelined_host_unittest.cc:66: bool active() const OVERRIDE {
return active_; }
On 2011/08/23 19:05:25, Matt Menke wrote:
> nit:  Use virtual as well as OVERRIDE.

Done.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_hos...
net/http/http_pipelined_host_unittest.cc:134: .Times(2);
On 2011/08/23 19:05:25, Matt Menke wrote:
> Maybe add:
> 
>   EXPECT_CALL(owner_, OnHostIdle(&host_))
>       .Times(0);

Done.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_hos...
net/http/http_pipelined_host_unittest.cc:177: 
On 2011/08/23 19:05:25, Matt Menke wrote:
> Maybe add something along the lines of:
> 
> TEST_F(HttpPipelinedHostTest, ClearPipeline) {
>   MockPipeline* pipeline = AddTestPipeline(0, true, true);
> 
>   EXPECT_NE(NULL, host_.FindAvailablePipeline());
> 
>   ClearTestPipeline(pipeline);
> 
>   EXPECT_EQ(NULL, host_.FindAvailablePipeline());
> }

Done.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_str...
File net/http/http_pipelined_stream.h (right):

http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_str...
net/http/http_pipelined_stream.h:21: class HttpRequestHeaders;
On 2011/08/23 19:05:25, Matt Menke wrote:
> nit:  Alphabetize.

Done.

http://codereview.chromium.org/7289006/diff/21001/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_request.h (right):

http://codereview.chromium.org/7289006/diff/21001/net/http/http_stream_factor...
net/http/http_stream_factory_impl_request.h:53: // If this Request has a
http_pipelining_key, remove this session from the
On 2011/08/23 19:05:25, Matt Menke wrote:
> nit:  |http_pipelining_key_|

Done.

[willchan no longer on Chromium, 2011-09-01 21:11:38]

Just got back from vacation, I hope to get around to this today. If I don't,
feel free to ping me.

[mmenke, 2011-09-01 21:15:39]

A couple more comments.

http://codereview.chromium.org/7289006/diff/38001/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/7289006/diff/38001/net/http/http_network_trans...
net/http/http_network_transaction.cc:479:
RestartIgnoringLastError(user_callback_);
You want to return here.  Otherwise, you'll pass the error back up to whatever's
waiting to hear back from the request, assuming user_callback_ is non-null.

http://codereview.chromium.org/7289006/diff/38001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection_impl.cc (right):

http://codereview.chromium.org/7289006/diff/38001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:101: // added.
Seems to me this class is no longer implementing the HttpStream interface, since
it has an additional requirement other HttpStreams do not.  I think we should
come to a decision if we're going to force Close() to be called on all
HttpStreams or not before landing this CL.

Can change our minds later, but think the interface should behave consistently
across different classes that implement it.

http://codereview.chromium.org/7289006/diff/38001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:106:
DCHECK(!ContainsKey(callback_map_, pipeline_id));
nit:  Could move this to the top of the function, outside the if.

http://codereview.chromium.org/7289006/diff/38001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:111: }
nit:  Maybe add out of general paranoia:

DCHECK(!ContainsKey(parser_map_, pipeline_id));.

http://codereview.chromium.org/7289006/diff/38001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:200: // SendRequest() earlier and now
need to invoke its callback.
nit:  "now need" -> "will need".   "Now need" has a bit of "need to do it now"
implication.

http://codereview.chromium.org/7289006/diff/38001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:219: callback->Run(result);
Looks to me like this will always result in cancellation on error.  The caller
will get the original error code, then delete everything and give up.  Instead,
you want to change the error code to ERR_PIPELINE_EVICTION here, if necessary,
and pass that along.

Should check this in the unit tests as well, of course.

http://codereview.chromium.org/7289006/diff/38001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:232: } else {
nit:  Else not needed, because of the above return.

http://codereview.chromium.org/7289006/diff/38001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:512: return
parser_map_.find(pipeline_id)->second->CanFindEndOfResponse();
Half the time you use "parser_map_.find(pipeline_id)->second", half the time you
use "parser_map_[pipeline_id]".  I suggest you just settle on the latter, except
in cases where you check if it's present or not.

http://codereview.chromium.org/7289006/diff/38001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection_impl.h (right):

http://codereview.chromium.org/7289006/diff/38001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.h:27: class HttpPipelinedStream;
nit:  Doesn't look like you use this.

http://codereview.chromium.org/7289006/diff/38001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.h:220: // being invoked. This may be
used for either Send or Read callbacks.
nit:  Update comment.

[willchan no longer on Chromium, 2011-09-03 01:11:06]

I've only just started to take a look. I primarily looked at the
HttpStreamFactoryImpl integration points, which look great. But the meat of the
change is in the HttpPipelined* stuff, which I need to delve into. I'll read
those in detail later this weekend and get back to you.

http://codereview.chromium.org/7289006/diff/38001/net/http/http_stream.h
File net/http/http_stream.h (right):

http://codereview.chromium.org/7289006/diff/38001/net/http/http_stream.h#newc...
net/http/http_stream.h:143: // In the case of an HTTP error, flush the response
body (usually a simple
It's not only used for HTTP errors, but also redirects.

http://codereview.chromium.org/7289006/diff/38001/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/7289006/diff/38001/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:589: } else if
(IsRequestEligibleForPipelining()) {
If we're preconnecting, you may want to choose this opportunity to reduce the
number of connections we try to establish. I'm not sure what makes sense. Just
thought I'd point it out here. I'd probably leave a TODO to consider calculating
a desired number of connections to preconnect here, where the num_connections <=
num_streams_.

[James Simonsen, 2011-09-07 21:20:10]

http://codereview.chromium.org/7289006/diff/38001/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/7289006/diff/38001/net/http/http_network_trans...
net/http/http_network_transaction.cc:479:
RestartIgnoringLastError(user_callback_);
On 2011/09/01 21:15:39, Matt Menke wrote:
> You want to return here.  Otherwise, you'll pass the error back up to
whatever's
> waiting to hear back from the request, assuming user_callback_ is non-null.

Eek. Yep. Fixed. Added test cases for the transaction and fixed some other bugs
too.

http://codereview.chromium.org/7289006/diff/38001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection_impl.cc (right):

http://codereview.chromium.org/7289006/diff/38001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:101: // added.
On 2011/09/01 21:15:39, Matt Menke wrote:
> Seems to me this class is no longer implementing the HttpStream interface,
since
> it has an additional requirement other HttpStreams do not.  I think we should
> come to a decision if we're going to force Close() to be called on all
> HttpStreams or not before landing this CL.
> 
> Can change our minds later, but think the interface should behave consistently
> across different classes that implement it.

Done.

http://codereview.chromium.org/7289006/diff/38001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:106:
DCHECK(!ContainsKey(callback_map_, pipeline_id));
On 2011/09/01 21:15:39, Matt Menke wrote:
> nit:  Could move this to the top of the function, outside the if.

Done.

http://codereview.chromium.org/7289006/diff/38001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:111: }
On 2011/09/01 21:15:39, Matt Menke wrote:
> nit:  Maybe add out of general paranoia:
> 
> DCHECK(!ContainsKey(parser_map_, pipeline_id));.

Done.

http://codereview.chromium.org/7289006/diff/38001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:200: // SendRequest() earlier and now
need to invoke its callback.
On 2011/09/01 21:15:39, Matt Menke wrote:
> nit:  "now need" -> "will need".   "Now need" has a bit of "need to do it now"
> implication.

Done.

http://codereview.chromium.org/7289006/diff/38001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:219: callback->Run(result);
On 2011/09/01 21:15:39, Matt Menke wrote:
> Looks to me like this will always result in cancellation on error.  The caller
> will get the original error code, then delete everything and give up. 
Instead,
> you want to change the error code to ERR_PIPELINE_EVICTION here, if necessary,
> and pass that along.
> 
> Should check this in the unit tests as well, of course.

Done.

http://codereview.chromium.org/7289006/diff/38001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:232: } else {
On 2011/09/01 21:15:39, Matt Menke wrote:
> nit:  Else not needed, because of the above return.

Done.

http://codereview.chromium.org/7289006/diff/38001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:512: return
parser_map_.find(pipeline_id)->second->CanFindEndOfResponse();
On 2011/09/01 21:15:39, Matt Menke wrote:
> Half the time you use "parser_map_.find(pipeline_id)->second", half the time
you
> use "parser_map_[pipeline_id]".  I suggest you just settle on the latter,
except
> in cases where you check if it's present or not.

I prefer [] too, but it isn't const. So I used it wherever I could and fell back
to find() when I couldn't.

http://codereview.chromium.org/7289006/diff/38001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection_impl.h (right):

http://codereview.chromium.org/7289006/diff/38001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.h:27: class HttpPipelinedStream;
On 2011/09/01 21:15:39, Matt Menke wrote:
> nit:  Doesn't look like you use this.

Done.

http://codereview.chromium.org/7289006/diff/38001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.h:220: // being invoked. This may be
used for either Send or Read callbacks.
On 2011/09/01 21:15:39, Matt Menke wrote:
> nit:  Update comment.

Done.

http://codereview.chromium.org/7289006/diff/38001/net/http/http_stream.h
File net/http/http_stream.h (right):

http://codereview.chromium.org/7289006/diff/38001/net/http/http_stream.h#newc...
net/http/http_stream.h:143: // In the case of an HTTP error, flush the response
body (usually a simple
On 2011/09/03 01:11:06, willchan wrote:
> It's not only used for HTTP errors, but also redirects.

Done.

http://codereview.chromium.org/7289006/diff/38001/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/7289006/diff/38001/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:589: } else if
(IsRequestEligibleForPipelining()) {
On 2011/09/03 01:11:06, willchan wrote:
> If we're preconnecting, you may want to choose this opportunity to reduce the
> number of connections we try to establish. I'm not sure what makes sense. Just
> thought I'd point it out here. I'd probably leave a TODO to consider
calculating
> a desired number of connections to preconnect here, where the num_connections
<=
> num_streams_.

Done.

Yeah, that's an interesting topic. It might be nice to use fewer connections in
general with pipelining, under the assumption that fewer, warmer pipes with
immediate RTTs is better.

Has anyone played with estimating response times? If we were able to model that
somehow, it'd help a lot with pipelining, especially front-of-queue blocking.

[mmenke, 2011-09-15 19:28:15]

A couple more mostly minor comments.

http://codereview.chromium.org/7289006/diff/55001/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/7289006/diff/55001/chrome/browser/about_flags....
chrome/browser/about_flags.cc:408: "enable-http-pipelining",
You used to read this flag in browser_main.cc.  You no longer seem to be doing
so, however, so pipelining will always be disabled.

http://codereview.chromium.org/7289006/diff/55001/net/http/http_network_trans...
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/7289006/diff/55001/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:9009: TestCompletionCallback
callback;
You're duplicating a fair bit of code here.  I think you could just adapt
SimpleGetHelper to your needs, either by making a new function (Possibly used
internally by SimpleGetHelper), or by just adding another parameter.

http://codereview.chromium.org/7289006/diff/55001/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:9012:
EXPECT_TRUE(log.bound().IsLoggingAllEvents());
nit:  Not needed, since you aren't validating events from the NetLog.

http://codereview.chromium.org/7289006/diff/55001/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:9067:
EXPECT_TRUE(log.bound().IsLoggingAllEvents());
nit:  Not needed, since you aren't validating events from the NetLog.

http://codereview.chromium.org/7289006/diff/55001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection.h (right):

http://codereview.chromium.org/7289006/diff/55001/net/http/http_pipelined_con...
net/http/http_pipelined_connection.h:19: class HttpPipelinedConnection {
Need "class NET_EXPORT_PRIVATE HttpPipelinedConnection" for unit tests on shared
library builds.

http://codereview.chromium.org/7289006/diff/55001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection_impl.cc (right):

http://codereview.chromium.org/7289006/diff/55001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:221: callback->Run(result);
Maybe we should mark this as unusable before running the callback?  Otherwise,
it may try to get a new pipeline, get us, and then be evicted again...  I think
things turn out fine, but seems a little funky.

Might want to test some of the pipeline assignment logic, too.

http://codereview.chromium.org/7289006/diff/55001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection_impl.h (right):

http://codereview.chromium.org/7289006/diff/55001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.h:41: class HttpPipelinedConnectionImpl
: public HttpPipelinedConnection {
Need "class NET_EXPORT_PRIVATE HttpPipelinedConnectionImpl" for unit tests on
shared library builds.

http://codereview.chromium.org/7289006/diff/55001/net/http/http_pipelined_host.h
File net/http/http_pipelined_host.h (right):

http://codereview.chromium.org/7289006/diff/55001/net/http/http_pipelined_hos...
net/http/http_pipelined_host.h:26: class HttpPipelinedHost : public
HttpPipelinedConnection::Delegate {
Need to use "class NET_EXPORT_PRIVATE HttpPipelinedHost", so the unit tests can
access it in a shared library build.

[willchan no longer on Chromium, 2011-09-15 19:44:47]

Apologies, code yellow is slowing me down. I'll get to this eventually =/
On Sep 15, 2011 12:28 PM, <mmenke@chromium.org> wrote:
> A couple more mostly minor comments.
>
>
>
http://codereview.chromium.org/7289006/diff/55001/chrome/browser/about_flags.cc
> File chrome/browser/about_flags.cc (right):
>
>
http://codereview.chromium.org/7289006/diff/55001/chrome/browser/about_flags....
> chrome/browser/about_flags.cc:408: "enable-http-pipelining",
> You used to read this flag in browser_main.cc. You no longer seem to be
> doing so, however, so pipelining will always be disabled.
>
>
http://codereview.chromium.org/7289006/diff/55001/net/http/http_network_trans...
> File net/http/http_network_transaction_unittest.cc (right):
>
>
http://codereview.chromium.org/7289006/diff/55001/net/http/http_network_trans...
> net/http/http_network_transaction_unittest.cc:9009:
> TestCompletionCallback callback;
> You're duplicating a fair bit of code here. I think you could just
> adapt SimpleGetHelper to your needs, either by making a new function
> (Possibly used internally by SimpleGetHelper), or by just adding another
> parameter.
>
>
http://codereview.chromium.org/7289006/diff/55001/net/http/http_network_trans...
> net/http/http_network_transaction_unittest.cc:9012:
> EXPECT_TRUE(log.bound().IsLoggingAllEvents());
> nit: Not needed, since you aren't validating events from the NetLog.
>
>
http://codereview.chromium.org/7289006/diff/55001/net/http/http_network_trans...
> net/http/http_network_transaction_unittest.cc:9067:
> EXPECT_TRUE(log.bound().IsLoggingAllEvents());
> nit: Not needed, since you aren't validating events from the NetLog.
>
>
http://codereview.chromium.org/7289006/diff/55001/net/http/http_pipelined_con...
> File net/http/http_pipelined_connection.h (right):
>
>
http://codereview.chromium.org/7289006/diff/55001/net/http/http_pipelined_con...
> net/http/http_pipelined_connection.h:19: class HttpPipelinedConnection {
> Need "class NET_EXPORT_PRIVATE HttpPipelinedConnection" for unit tests
> on shared library builds.
>
>
http://codereview.chromium.org/7289006/diff/55001/net/http/http_pipelined_con...
> File net/http/http_pipelined_connection_impl.cc (right):
>
>
http://codereview.chromium.org/7289006/diff/55001/net/http/http_pipelined_con...
> net/http/http_pipelined_connection_impl.cc:221: callback->Run(result);
> Maybe we should mark this as unusable before running the callback?
> Otherwise, it may try to get a new pipeline, get us, and then be evicted
> again... I think things turn out fine, but seems a little funky.
>
> Might want to test some of the pipeline assignment logic, too.
>
>
http://codereview.chromium.org/7289006/diff/55001/net/http/http_pipelined_con...
> File net/http/http_pipelined_connection_impl.h (right):
>
>
http://codereview.chromium.org/7289006/diff/55001/net/http/http_pipelined_con...
> net/http/http_pipelined_connection_impl.h:41: class
> HttpPipelinedConnectionImpl : public HttpPipelinedConnection {
> Need "class NET_EXPORT_PRIVATE HttpPipelinedConnectionImpl" for unit
> tests on shared library builds.
>
>
http://codereview.chromium.org/7289006/diff/55001/net/http/http_pipelined_host.h
> File net/http/http_pipelined_host.h (right):
>
>
http://codereview.chromium.org/7289006/diff/55001/net/http/http_pipelined_hos...
> net/http/http_pipelined_host.h:26: class HttpPipelinedHost : public
> HttpPipelinedConnection::Delegate {
> Need to use "class NET_EXPORT_PRIVATE HttpPipelinedHost", so the unit
> tests can access it in a shared library build.
>
> http://codereview.chromium.org/7289006/

[James Simonsen, 2011-09-17 01:23:02]

http://codereview.chromium.org/7289006/diff/55001/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/7289006/diff/55001/chrome/browser/about_flags....
chrome/browser/about_flags.cc:408: "enable-http-pipelining",
On 2011/09/15 19:28:16, Matt Menke wrote:
> You used to read this flag in browser_main.cc.  You no longer seem to be doing
> so, however, so pipelining will always be disabled.

Done.

Oops. Must've lost it in a merge.

http://codereview.chromium.org/7289006/diff/55001/net/http/http_network_trans...
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/7289006/diff/55001/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:9009: TestCompletionCallback
callback;
On 2011/09/15 19:28:16, Matt Menke wrote:
> You're duplicating a fair bit of code here.  I think you could just adapt
> SimpleGetHelper to your needs, either by making a new function (Possibly used
> internally by SimpleGetHelper), or by just adding another parameter.

Done.

http://codereview.chromium.org/7289006/diff/55001/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:9012:
EXPECT_TRUE(log.bound().IsLoggingAllEvents());
On 2011/09/15 19:28:16, Matt Menke wrote:
> nit:  Not needed, since you aren't validating events from the NetLog.

Removed.

http://codereview.chromium.org/7289006/diff/55001/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:9067:
EXPECT_TRUE(log.bound().IsLoggingAllEvents());
On 2011/09/15 19:28:16, Matt Menke wrote:
> nit:  Not needed, since you aren't validating events from the NetLog.

Removed.

http://codereview.chromium.org/7289006/diff/55001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection.h (right):

http://codereview.chromium.org/7289006/diff/55001/net/http/http_pipelined_con...
net/http/http_pipelined_connection.h:19: class HttpPipelinedConnection {
On 2011/09/15 19:28:16, Matt Menke wrote:
> Need "class NET_EXPORT_PRIVATE HttpPipelinedConnection" for unit tests on
shared
> library builds.

Done.

http://codereview.chromium.org/7289006/diff/55001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection_impl.cc (right):

http://codereview.chromium.org/7289006/diff/55001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:221: callback->Run(result);
On 2011/09/15 19:28:16, Matt Menke wrote:
> Maybe we should mark this as unusable before running the callback?  Otherwise,
> it may try to get a new pipeline, get us, and then be evicted again...  I
think
> things turn out fine, but seems a little funky.

Yeah, that sounds good.

> Might want to test some of the pipeline assignment logic, too.

The assignment logic should be tested in http_pipelined_host_unittest.cc.

Do you mean to add integration tests? That seems like a good idea. The
relationship between objects and the order they do things is important. The unit
tests try to simulate that, but probably not perfectly.

I can look into some integration tests next week. I think the important pieces
are the transaction, pool, host, pipeline, and stream.

http://codereview.chromium.org/7289006/diff/55001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection_impl.h (right):

http://codereview.chromium.org/7289006/diff/55001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.h:41: class HttpPipelinedConnectionImpl
: public HttpPipelinedConnection {
On 2011/09/15 19:28:16, Matt Menke wrote:
> Need "class NET_EXPORT_PRIVATE HttpPipelinedConnectionImpl" for unit tests on
> shared library builds.

Done.

http://codereview.chromium.org/7289006/diff/55001/net/http/http_pipelined_host.h
File net/http/http_pipelined_host.h (right):

http://codereview.chromium.org/7289006/diff/55001/net/http/http_pipelined_hos...
net/http/http_pipelined_host.h:26: class HttpPipelinedHost : public
HttpPipelinedConnection::Delegate {
On 2011/09/15 19:28:16, Matt Menke wrote:
> Need to use "class NET_EXPORT_PRIVATE HttpPipelinedHost", so the unit tests
can
> access it in a shared library build.

Done.

[mmenke, 2011-09-17 01:41:22]

http://codereview.chromium.org/7289006/diff/55001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection_impl.cc (right):

http://codereview.chromium.org/7289006/diff/55001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:221: callback->Run(result);
Yea, I was just thinking that we don't test reassignment to a new pipelined
connection after eviction.  Think you're right about the components we'd need to
adequately test that.

On 2011/09/17 01:23:02, James Simonsen wrote:
> > Might want to test some of the pipeline assignment logic, too.
> 
> The assignment logic should be tested in http_pipelined_host_unittest.cc.
> 
> Do you mean to add integration tests? That seems like a good idea. The
> relationship between objects and the order they do things is important. The
unit
> tests try to simulate that, but probably not perfectly.
> 
> I can look into some integration tests next week. I think the important pieces
> are the transaction, pool, host, pipeline, and stream.

[willchan no longer on Chromium, 2011-09-26 23:49:20]

Here are some more comments finally! I have mostly read through all the
HttpStreamFactoryImpl::Job integration points. I'll try to get another iteration
out today.

http://codereview.chromium.org/7289006/diff/68001/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/7289006/diff/68001/net/http/http_network_trans...
net/http/http_network_transaction.cc:571: if (rv == ERR_PIPELINE_EVICTION) {
This is breaking our paradigm for the state machine. Can you move this to the
exact state necessary? I suspect the correct place to put this code is in
DoReadHeadersComplete().

http://codereview.chromium.org/7289006/diff/68001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection.h (right):

http://codereview.chromium.org/7289006/diff/68001/net/http/http_pipelined_con...
net/http/http_pipelined_connection.h:19: class NET_EXPORT_PRIVATE
HttpPipelinedConnection {
I forget what header this macro comes from, but please include it.

http://codereview.chromium.org/7289006/diff/68001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection_impl.h (right):

http://codereview.chromium.org/7289006/diff/68001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.h:58: virtual int depth() const OVERRIDE
{ return stream_state_map_.size(); }
I suspect this won't pass the clang bot. You might want to hit linux_clang and
see what it finds.

http://codereview.chromium.org/7289006/diff/68001/net/http/http_pipelined_hos...
File net/http/http_pipelined_host.cc (right):

http://codereview.chromium.org/7289006/diff/68001/net/http/http_pipelined_hos...
net/http/http_pipelined_host.cc:37: factory_ = new
HttpPipelinedConnectionImplFactory();
Who owns this? Where is this deleted?

http://codereview.chromium.org/7289006/diff/68001/net/http/http_pipelined_hos...
net/http/http_pipelined_host.cc:89: // WARNING: We'll probably be deleted here.
Interesting. So when pipelines go empty, we don't preserve the
HttpPipelinedConnection in its HTTP keep alive idle state? That's fine as an
initial implementation, but I would think that we should fix that in the future.
Adding a TODO somewhere to remember that would be great. I would actually prefer
you not implement it yet so we can keep this changelist manageable :)

http://codereview.chromium.org/7289006/diff/68001/net/http/http_pipelined_hos...
File net/http/http_pipelined_host_pool.h (right):

http://codereview.chromium.org/7289006/diff/68001/net/http/http_pipelined_hos...
net/http/http_pipelined_host_pool.h:48: typedef std::map<const HostPortPair,
HttpPipelinedHost*> HostMap;
types go first in the access control section, a la
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

http://codereview.chromium.org/7289006/diff/68001/net/http/http_pipelined_hos...
net/http/http_pipelined_host_pool.h:51:
DISALLOW_COPY_AND_ASSIGN(HttpPipelinedHostPool);
Include base/basictypes.h for this

http://codereview.chromium.org/7289006/diff/68001/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/7289006/diff/68001/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:789: HACKCrashHereToDebug80095();
Can you just kill this code? It's been debugged, I haven't cleaned it up yet.
Thanks.

http://codereview.chromium.org/7289006/diff/68001/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:811: HttpPipelinedConnection* pipeline
= session_->http_pipelined_host_pool()->
It's not clear why the HttpPipelinedConnection* is exposed to
HttpStreamFactoryImpl::Job. It sort of seems like, from the Job*'s perspective,
we should just have an API on the HttpPipelinedHostPool that creates a stream.
The HttpPipelinedConnection and HttpPipelinedHost are implementation details
behind HttpPipelinedHostPool.

[James Simonsen, 2011-09-29 02:39:31]

I still need to look into the integration test that Matt was talking about, but
the other comments have been addressed.

http://codereview.chromium.org/7289006/diff/68001/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/7289006/diff/68001/net/http/http_network_trans...
net/http/http_network_transaction.cc:571: if (rv == ERR_PIPELINE_EVICTION) {
On 2011/09/26 23:49:20, willchan wrote:
> This is breaking our paradigm for the state machine. Can you move this to the
> exact state necessary? I suspect the correct place to put this code is in
> DoReadHeadersComplete().

It's for the sake of logging. I want to return ERR_PIPELINE_EVICTION for the
ReadHeaders end event.

I've changed it, but now it'll log OK and then start over again. That might be
confusing.

http://codereview.chromium.org/7289006/diff/68001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection.h (right):

http://codereview.chromium.org/7289006/diff/68001/net/http/http_pipelined_con...
net/http/http_pipelined_connection.h:19: class NET_EXPORT_PRIVATE
HttpPipelinedConnection {
On 2011/09/26 23:49:20, willchan wrote:
> I forget what header this macro comes from, but please include it.

Done.

http://codereview.chromium.org/7289006/diff/68001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection_impl.h (right):

http://codereview.chromium.org/7289006/diff/68001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.h:58: virtual int depth() const OVERRIDE
{ return stream_state_map_.size(); }
On 2011/09/26 23:49:20, willchan wrote:
> I suspect this won't pass the clang bot. You might want to hit linux_clang and
> see what it finds.

Seems fine. It did find something else, which I've fixed.

http://codereview.chromium.org/7289006/diff/68001/net/http/http_pipelined_hos...
File net/http/http_pipelined_host.cc (right):

http://codereview.chromium.org/7289006/diff/68001/net/http/http_pipelined_hos...
net/http/http_pipelined_host.cc:37: factory_ = new
HttpPipelinedConnectionImplFactory();
On 2011/09/26 23:49:20, willchan wrote:
> Who owns this? Where is this deleted?

Done.

http://codereview.chromium.org/7289006/diff/68001/net/http/http_pipelined_hos...
net/http/http_pipelined_host.cc:89: // WARNING: We'll probably be deleted here.
On 2011/09/26 23:49:20, willchan wrote:
> Interesting. So when pipelines go empty, we don't preserve the
> HttpPipelinedConnection in its HTTP keep alive idle state? That's fine as an
> initial implementation, but I would think that we should fix that in the
future.
> Adding a TODO somewhere to remember that would be great. I would actually
prefer
> you not implement it yet so we can keep this changelist manageable :)

Done. Yeah, I definitely intend to track what we learn about hosts. My plan is
still to just release the connection back to the normal socket pool, but
remember that pipelining works for this host. So, the next time we need
something from that host, we'll just immediately start off with pipelining
instead of going through the soon-to-be-added heuristics.

http://codereview.chromium.org/7289006/diff/68001/net/http/http_pipelined_hos...
File net/http/http_pipelined_host_pool.h (right):

http://codereview.chromium.org/7289006/diff/68001/net/http/http_pipelined_hos...
net/http/http_pipelined_host_pool.h:48: typedef std::map<const HostPortPair,
HttpPipelinedHost*> HostMap;
On 2011/09/26 23:49:20, willchan wrote:
> types go first in the access control section, a la
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

Done.

http://codereview.chromium.org/7289006/diff/68001/net/http/http_pipelined_hos...
net/http/http_pipelined_host_pool.h:51:
DISALLOW_COPY_AND_ASSIGN(HttpPipelinedHostPool);
On 2011/09/26 23:49:20, willchan wrote:
> Include base/basictypes.h for this

Done.

http://codereview.chromium.org/7289006/diff/68001/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/7289006/diff/68001/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:789: HACKCrashHereToDebug80095();
On 2011/09/26 23:49:20, willchan wrote:
> Can you just kill this code? It's been debugged, I haven't cleaned it up yet.
> Thanks.

Done.

http://codereview.chromium.org/7289006/diff/68001/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:811: HttpPipelinedConnection* pipeline
= session_->http_pipelined_host_pool()->
On 2011/09/26 23:49:20, willchan wrote:
> It's not clear why the HttpPipelinedConnection* is exposed to
> HttpStreamFactoryImpl::Job. It sort of seems like, from the Job*'s
perspective,
> we should just have an API on the HttpPipelinedHostPool that creates a stream.
> The HttpPipelinedConnection and HttpPipelinedHost are implementation details
> behind HttpPipelinedHostPool.

Done.

[willchan no longer on Chromium, 2011-09-29 19:32:48]

http://codereview.chromium.org/7289006/diff/68001/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/7289006/diff/68001/net/http/http_network_trans...
net/http/http_network_transaction.cc:571: if (rv == ERR_PIPELINE_EVICTION) {
On 2011/09/29 02:39:31, James Simonsen wrote:
> On 2011/09/26 23:49:20, willchan wrote:
> > This is breaking our paradigm for the state machine. Can you move this to
the
> > exact state necessary? I suspect the correct place to put this code is in
> > DoReadHeadersComplete().
> 
> It's for the sake of logging. I want to return ERR_PIPELINE_EVICTION for the
> ReadHeaders end event.
> 
> I've changed it, but now it'll log OK and then start over again. That might be
> confusing.

I pointed out where I think you should do the logging.

http://codereview.chromium.org/7289006/diff/68001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection_impl.cc (right):

http://codereview.chromium.org/7289006/diff/68001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:48: DCHECK(depth() == 0);
DCHECK_EQ?

http://codereview.chromium.org/7289006/diff/79001/net/http/http_network_trans...
File net/http/http_network_transaction.cc (left):

http://codereview.chromium.org/7289006/diff/79001/net/http/http_network_trans...
net/http/http_network_transaction.cc:551:
NetLog::TYPE_HTTP_TRANSACTION_READ_HEADERS, rv);
You should keep this back where it was.

http://codereview.chromium.org/7289006/diff/79001/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/7289006/diff/79001/net/http/http_network_trans...
net/http/http_network_transaction.cc:866: next_state_ = STATE_READ_BODY;
I believe this is a bug. We should only transition to this state when Read() is
called.

http://codereview.chromium.org/7289006/diff/79001/net/http/http_network_trans...
net/http/http_network_transaction.cc:1183: case ERR_PIPELINE_EVICTION:
I think here you should log to net_log_. You should probably create a new event
for this. I think this event should also be logged below, above the call to 
ResetConnectionAndRequestForResend();

http://codereview.chromium.org/7289006/diff/79001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection_impl.cc (right):

http://codereview.chromium.org/7289006/diff/79001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:48: DCHECK(depth() == 0);
DCHECK_EQ?

[willchan no longer on Chromium, 2011-09-29 20:20:07]

HttpPipelinedConnectionImpl is complicated, I am trying to fit everything into
my head. Sending more comments.

http://codereview.chromium.org/7289006/diff/79001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection_impl.cc (right):

http://codereview.chromium.org/7289006/diff/79001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:88:
&HttpPipelinedConnectionImpl::FillPipeline));
Is this a race? What if something else comes in and uses the available pipeline
capacity before this Task gets run by the MessageLoop?

http://codereview.chromium.org/7289006/diff/79001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:92: void
HttpPipelinedConnectionImpl::FillPipeline() {
I pointed out some races earlier. Perhaps you need to check here to see if
there's still a need? Or perhaps we need to revoke some of those other methods?
I need to think about this more.

http://codereview.chromium.org/7289006/diff/79001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:116:
&HttpPipelinedConnectionImpl::FillPipeline));
I'm wondering if this is also a race.

http://codereview.chromium.org/7289006/diff/79001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:225: callback->Run(result);
After invoking this callback, do we know what the state of this connection is?
Is it possible that this is deleted? Behavior after invoking callbacks is always
a bit dicey.

http://codereview.chromium.org/7289006/diff/79001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:238:
&HttpPipelinedConnectionImpl::DoSendRequestLoop,
Why don't we just transition to the state here?

http://codereview.chromium.org/7289006/diff/79001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection_impl.h (right):

http://codereview.chromium.org/7289006/diff/79001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.h:244: StreamStateMap stream_state_map_;
I don't see why there are so many maps. Aren't they the same keys? How about we
use a single map to a struct value?

[willchan no longer on Chromium, 2011-09-29 23:36:02]

Another interesting thing I've noticed about this architecture is that it lets a
single request block all the other ones on the pipeline. If the renderer isn't
reading from the pipeline, then all the other responses behind it on the
pipeline are blocked. For the most part, this shouldn't be a problem, since for
a given pipelining host, the same renderer would be trying to read responses.
However, this isn't necessarily true for the proxy case, since theoretically we
could pipeline requests to different origin servers through the same connection
to a proxy.

Food for thought. The alternative would be to buffer the responses in memory,
which could get pretty large. We could use a fixed size to compensate for this.

Another thing I'd like to see is a lot more use of CHECKs than DCHECKs. Part of
this is because you're launching via about:flags (great!) so we can be meaner.
But the other thing is that pipelining implementation mistakes could have nasty
repercussions, including some security related ones, so I think it's safer to
crash when our invariants aren't met. It raises the bar for the quality of this
code, but I think we should do it.

http://codereview.chromium.org/7289006/diff/79001/net/http/http_stream_factory.h
File net/http/http_stream_factory.h (right):

http://codereview.chromium.org/7289006/diff/79001/net/http/http_stream_factor...
net/http/http_stream_factory.h:180: virtual void
OnHttpPipelinedHostHasAdditionalCapacity(
I consider pipelining an implementation detail. Not every HttpStreamFactory
implementation needs to support it. Therefore, I think you should move this
virtual to HttpStreamFactoryImpl.

[willchan no longer on Chromium, 2011-09-29 23:38:17]

The HttpPipelinedConnectionImpl is hard for me to grok. I'm not sure if there's
a better way to structure it that would solve this problem. I'll think on it.

I'm inclined to approve this changelist assuming there's good test coverage in
not too long. I think it needs work, but I'd rather get this in and do smaller
iterations. I may take the strategy of filing a bunch of bugs with the
implementation and letting you address those in smaller chunks.

On 2011/09/29 23:36:02, willchan wrote:
> Another interesting thing I've noticed about this architecture is that it lets
a
> single request block all the other ones on the pipeline. If the renderer isn't
> reading from the pipeline, then all the other responses behind it on the
> pipeline are blocked. For the most part, this shouldn't be a problem, since
for
> a given pipelining host, the same renderer would be trying to read responses.
> However, this isn't necessarily true for the proxy case, since theoretically
we
> could pipeline requests to different origin servers through the same
connection
> to a proxy.
> 
> Food for thought. The alternative would be to buffer the responses in memory,
> which could get pretty large. We could use a fixed size to compensate for
this.
> 
> Another thing I'd like to see is a lot more use of CHECKs than DCHECKs. Part
of
> this is because you're launching via about:flags (great!) so we can be meaner.
> But the other thing is that pipelining implementation mistakes could have
nasty
> repercussions, including some security related ones, so I think it's safer to
> crash when our invariants aren't met. It raises the bar for the quality of
this
> code, but I think we should do it.
> 
>
http://codereview.chromium.org/7289006/diff/79001/net/http/http_stream_factory.h
> File net/http/http_stream_factory.h (right):
> 
>
http://codereview.chromium.org/7289006/diff/79001/net/http/http_stream_factor...
> net/http/http_stream_factory.h:180: virtual void
> OnHttpPipelinedHostHasAdditionalCapacity(
> I consider pipelining an implementation detail. Not every HttpStreamFactory
> implementation needs to support it. Therefore, I think you should move this
> virtual to HttpStreamFactoryImpl.

[James Simonsen, 2011-10-12 01:36:58]

Done gardening. Time to get this landed...

Agreed that this HttpPipelinedConnectionImpl could use another refactoring. I
think adding more states would help, instead of trying to collapse the
synchronous and asynchronous paths onto each other.

Good point about one HttpStream holding up the rest. We could easily buffer in
HttpPipelinedStream. Sounds like something we should play with down the road.

Near term TODO list:
- Add integration tests.
- Add more NetLog events.
- Refactor HttpPipelinedConnectionImpl (add more states).

Let me know which, if any, of these should be fixed before this particular CL
goes in.

http://codereview.chromium.org/7289006/diff/79001/net/http/http_network_trans...
File net/http/http_network_transaction.cc (left):

http://codereview.chromium.org/7289006/diff/79001/net/http/http_network_trans...
net/http/http_network_transaction.cc:551:
NetLog::TYPE_HTTP_TRANSACTION_READ_HEADERS, rv);
On 2011/09/29 19:32:48, willchan wrote:
> You should keep this back where it was.

Didn't mean to delete it. Forgot to save before uploading.

http://codereview.chromium.org/7289006/diff/79001/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/7289006/diff/79001/net/http/http_network_trans...
net/http/http_network_transaction.cc:866: next_state_ = STATE_READ_BODY;
On 2011/09/29 19:32:48, willchan wrote:
> I believe this is a bug. We should only transition to this state when Read()
is
> called.

It's a tricky situation. Maybe I should just forget about it.

If we were to be evicted after Read() was called, then after restarting, we
shouldn't wait for Read() to be called again. We know Read() has been called if
we have a read_buf_len_.

Anyway, this shouldn't happen in practice, so I just removed it. I'd only seen
it while playing with the unit test.

http://codereview.chromium.org/7289006/diff/79001/net/http/http_network_trans...
net/http/http_network_transaction.cc:1183: case ERR_PIPELINE_EVICTION:
On 2011/09/29 19:32:48, willchan wrote:
> I think here you should log to net_log_. You should probably create a new
event
> for this. I think this event should also be logged below, above the call to 
> ResetConnectionAndRequestForResend();

Done.

http://codereview.chromium.org/7289006/diff/79001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection_impl.cc (right):

http://codereview.chromium.org/7289006/diff/79001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:48: DCHECK(depth() == 0);
On 2011/09/29 19:32:48, willchan wrote:
> DCHECK_EQ?

Done.

http://codereview.chromium.org/7289006/diff/79001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:88:
&HttpPipelinedConnectionImpl::FillPipeline));
On 2011/09/29 20:20:07, willchan wrote:
> Is this a race? What if something else comes in and uses the available
pipeline
> capacity before this Task gets run by the MessageLoop?

Yep, good catch! Fixed. Added/updated tests too.

http://codereview.chromium.org/7289006/diff/79001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:92: void
HttpPipelinedConnectionImpl::FillPipeline() {
On 2011/09/29 20:20:07, willchan wrote:
> I pointed out some races earlier. Perhaps you need to check here to see if
> there's still a need? Or perhaps we need to revoke some of those other
methods?
> I need to think about this more.

Done.

http://codereview.chromium.org/7289006/diff/79001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:116:
&HttpPipelinedConnectionImpl::FillPipeline));
On 2011/09/29 20:20:07, willchan wrote:
> I'm wondering if this is also a race.

Yep. Same issue. Fixed.

http://codereview.chromium.org/7289006/diff/79001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:225: callback->Run(result);
On 2011/09/29 20:20:07, willchan wrote:
> After invoking this callback, do we know what the state of this connection is?
> Is it possible that this is deleted? Behavior after invoking callbacks is
always
> a bit dicey.

Icky. Fixed. Added tests.

It worked fine when FillPipeline() was in another task. I guess that's the thing
to do here. That's what we do with read callback already.

http://codereview.chromium.org/7289006/diff/79001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:238:
&HttpPipelinedConnectionImpl::DoSendRequestLoop,
On 2011/09/29 20:20:07, willchan wrote:
> Why don't we just transition to the state here?

Yeah, seems like a good optimization.

http://codereview.chromium.org/7289006/diff/79001/net/http/http_pipelined_con...
File net/http/http_pipelined_connection_impl.h (right):

http://codereview.chromium.org/7289006/diff/79001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.h:244: StreamStateMap stream_state_map_;
On 2011/09/29 20:20:07, willchan wrote:
> I don't see why there are so many maps. Aren't they the same keys? How about
we
> use a single map to a struct value?

Done.

http://codereview.chromium.org/7289006/diff/79001/net/http/http_stream_factory.h
File net/http/http_stream_factory.h (right):

http://codereview.chromium.org/7289006/diff/79001/net/http/http_stream_factor...
net/http/http_stream_factory.h:180: virtual void
OnHttpPipelinedHostHasAdditionalCapacity(
On 2011/09/29 23:36:02, willchan wrote:
> I consider pipelining an implementation detail. Not every HttpStreamFactory
> implementation needs to support it. Therefore, I think you should move this
> virtual to HttpStreamFactoryImpl.

I tried this, but I didn't like it.

The pool gets access to the factory through the session. Changing this means the
session needs to return a FactoryImpl. That seems wrong.

An alternative would be to have the FactoryImpl own the pool, but that breaks
the symmetry with SPDY and I think the session is the right place for the pool.

My vote is to punt on this until we really do have different factories. Then we
can work it out.

[mmenke, 2011-10-12 19:41:35]

A couple more nits.  I defer to Will on deciding what should be done before
landing.

http://codereview.chromium.org/7289006/diff/101002/net/http/http_network_tran...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/7289006/diff/101002/net/http/http_network_tran...
net/http/http_network_transaction.cc:1186: make_scoped_refptr(new
NetLogIntegerParameter("net_error", error)));
nit:  I have a CL under review which lets you just do:

net_log_.AddEventWithNetErrorCode(
    NetLog::TYPE_HTTP_TRANSACTION_RESTART_AFTER_ERROR,
    error);

Hoping to land it in a day or two.

http://codereview.chromium.org/7289006/diff/101002/net/http/http_pipelined_co...
File net/http/http_pipelined_connection.h (right):

http://codereview.chromium.org/7289006/diff/101002/net/http/http_pipelined_co...
net/http/http_pipelined_connection.h:49: virtual void OnStreamDeleted(int
pipeline_id) = 0;
Don't think this belongs here.  Like all functions that use pipeline_id, it's
called directly from the stream itself, and is the only mention of pipeline IDs
in this file.

http://codereview.chromium.org/7289006/diff/101002/net/http/http_pipelined_co...
net/http/http_pipelined_connection.h:68: // The source of this piplened
connection.
nit: pipelined.

http://codereview.chromium.org/7289006/diff/101002/net/http/http_pipelined_co...
File net/http/http_pipelined_connection_impl.h (right):

http://codereview.chromium.org/7289006/diff/101002/net/http/http_pipelined_co...
net/http/http_pipelined_connection_impl.h:164: HttpStreamParser* parser;
Any reason to not use scoped_ptr for this?  Makes the ownership relation
clearer, and avoids the need for explicitly deleting it.

http://codereview.chromium.org/7289006/diff/101002/net/http/http_pipelined_ho...
File net/http/http_pipelined_host.cc (right):

http://codereview.chromium.org/7289006/diff/101002/net/http/http_pipelined_ho...
net/http/http_pipelined_host.cc:16: HttpPipelinedConnection* CreateNewPipeline(
nit:  Fix indent.

http://codereview.chromium.org/7289006/diff/101002/net/http/http_pipelined_ho...
File net/http/http_pipelined_host_pool.h (right):

http://codereview.chromium.org/7289006/diff/101002/net/http/http_pipelined_ho...
net/http/http_pipelined_host_pool.h:18: class HttpStreamFactory;
nit: Alphabetize.

[James Simonsen, 2011-10-12 21:55:51]

http://codereview.chromium.org/7289006/diff/101002/net/http/http_network_tran...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/7289006/diff/101002/net/http/http_network_tran...
net/http/http_network_transaction.cc:1186: make_scoped_refptr(new
NetLogIntegerParameter("net_error", error)));
On 2011/10/12 19:41:35, Matt Menke wrote:
> nit:  I have a CL under review which lets you just do:
> 
> net_log_.AddEventWithNetErrorCode(
>     NetLog::TYPE_HTTP_TRANSACTION_RESTART_AFTER_ERROR,
>     error);
> 
> Hoping to land it in a day or two.

Nice. Will switch when it's ready.

http://codereview.chromium.org/7289006/diff/101002/net/http/http_pipelined_co...
File net/http/http_pipelined_connection.h (right):

http://codereview.chromium.org/7289006/diff/101002/net/http/http_pipelined_co...
net/http/http_pipelined_connection.h:49: virtual void OnStreamDeleted(int
pipeline_id) = 0;
On 2011/10/12 19:41:35, Matt Menke wrote:
> Don't think this belongs here.  Like all functions that use pipeline_id, it's
> called directly from the stream itself, and is the only mention of pipeline
IDs
> in this file.

Done.

http://codereview.chromium.org/7289006/diff/101002/net/http/http_pipelined_co...
net/http/http_pipelined_connection.h:68: // The source of this piplened
connection.
On 2011/10/12 19:41:35, Matt Menke wrote:
> nit: pipelined.

Done.

http://codereview.chromium.org/7289006/diff/101002/net/http/http_pipelined_co...
File net/http/http_pipelined_connection_impl.h (right):

http://codereview.chromium.org/7289006/diff/101002/net/http/http_pipelined_co...
net/http/http_pipelined_connection_impl.h:164: HttpStreamParser* parser;
On 2011/10/12 19:41:35, Matt Menke wrote:
> Any reason to not use scoped_ptr for this?  Makes the ownership relation
> clearer, and avoids the need for explicitly deleting it.

Hadn't tried it. Had to use linked_ptr because it's in a map<>, but otherwise
it's good.

http://codereview.chromium.org/7289006/diff/101002/net/http/http_pipelined_ho...
File net/http/http_pipelined_host.cc (right):

http://codereview.chromium.org/7289006/diff/101002/net/http/http_pipelined_ho...
net/http/http_pipelined_host.cc:16: HttpPipelinedConnection* CreateNewPipeline(
On 2011/10/12 19:41:35, Matt Menke wrote:
> nit:  Fix indent.

Done.

http://codereview.chromium.org/7289006/diff/101002/net/http/http_pipelined_ho...
File net/http/http_pipelined_host_pool.h (right):

http://codereview.chromium.org/7289006/diff/101002/net/http/http_pipelined_ho...
net/http/http_pipelined_host_pool.h:18: class HttpStreamFactory;
On 2011/10/12 19:41:35, Matt Menke wrote:
> nit: Alphabetize.

Done.

[willchan no longer on Chromium, 2011-10-12 22:05:26]

I will take a look later today. James, I am out half of tomorrow and all of
Friday. I will view things later today with an eye for blockers for landing
behind about:flags, and then give approval and make recommendations for
future changelists.

Matt, thanks for the help here. James, keep Matt cc'd on future CLs, because
after he's done more of these I hope to let him completely take over the
reviews and then I'll just poke my nose in every once in awhile.

On Wed, Oct 12, 2011 at 2:55 PM, <simonjam@chromium.org> wrote:

>
> http://codereview.chromium.**org/7289006/diff/101002/net/**
>
http/http_network_transaction.**cc<http://codereview.chromium.org/7289006/diff/101002/net/http/http_network_transaction.cc>
> File net/http/http_network_**transaction.cc (right):
>
> http://codereview.chromium.**org/7289006/diff/101002/net/**
>
http/http_network_transaction.**cc#newcode1186<http://codereview.chromium.org/7289006/diff/101002/net/http/http_network_transaction.cc#newcode1186>
> net/http/http_network_**transaction.cc:1186: make_scoped_refptr(new
> NetLogIntegerParameter("net_**error", error)));
> On 2011/10/12 19:41:35, Matt Menke wrote:
>
>> nit:  I have a CL under review which lets you just do:
>>
>
>  net_log_.**AddEventWithNetErrorCode(
>>     NetLog::TYPE_HTTP_TRANSACTION_**RESTART_AFTER_ERROR,
>>     error);
>>
>
>  Hoping to land it in a day or two.
>>
>
> Nice. Will switch when it's ready.
>
>
> http://codereview.chromium.**org/7289006/diff/101002/net/**
>
http/http_pipelined_**connection.h<http://codereview.chromium.org/7289006/diff/101002/net/http/http_pipelined_connection.h>
> File net/http/http_pipelined_**connection.h (right):
>
> http://codereview.chromium.**org/7289006/diff/101002/net/**
>
http/http_pipelined_**connection.h#newcode49<http://codereview.chromium.org/7289006/diff/101002/net/http/http_pipelined_connection.h#newcode49>
> net/http/http_pipelined_**connection.h:49: virtual void
> OnStreamDeleted(int pipeline_id) = 0;
> On 2011/10/12 19:41:35, Matt Menke wrote:
>
>> Don't think this belongs here.  Like all functions that use
>>
> pipeline_id, it's
>
>> called directly from the stream itself, and is the only mention of
>>
> pipeline IDs
>
>> in this file.
>>
>
> Done.
>
>
> http://codereview.chromium.**org/7289006/diff/101002/net/**
>
http/http_pipelined_**connection.h#newcode68<http://codereview.chromium.org/7289006/diff/101002/net/http/http_pipelined_connection.h#newcode68>
> net/http/http_pipelined_**connection.h:68: // The source of this piplened
> connection.
> On 2011/10/12 19:41:35, Matt Menke wrote:
>
>> nit: pipelined.
>>
>
> Done.
>
>
> http://codereview.chromium.**org/7289006/diff/101002/net/**
>
http/http_pipelined_**connection_impl.h<http://codereview.chromium.org/7289006/diff/101002/net/http/http_pipelined_connection_impl.h>
> File net/http/http_pipelined_**connection_impl.h (right):
>
> http://codereview.chromium.**org/7289006/diff/101002/net/**
>
http/http_pipelined_**connection_impl.h#newcode164<http://codereview.chromium.org/7289006/diff/101002/net/http/http_pipelined_connection_impl.h#newcode164>
> net/http/http_pipelined_**connection_impl.h:164: HttpStreamParser* parser;
> On 2011/10/12 19:41:35, Matt Menke wrote:
>
>> Any reason to not use scoped_ptr for this?  Makes the ownership
>>
> relation
>
>> clearer, and avoids the need for explicitly deleting it.
>>
>
> Hadn't tried it. Had to use linked_ptr because it's in a map<>, but
> otherwise it's good.
>
>
> http://codereview.chromium.**org/7289006/diff/101002/net/**
>
http/http_pipelined_host.cc<http://codereview.chromium.org/7289006/diff/101002/net/http/http_pipelined_host.cc>
> File net/http/http_pipelined_host.**cc (right):
>
> http://codereview.chromium.**org/7289006/diff/101002/net/**
>
http/http_pipelined_host.cc#**newcode16<http://codereview.chromium.org/7289006/diff/101002/net/http/http_pipelined_host.cc#newcode16>
> net/http/http_pipelined_host.**cc:16: HttpPipelinedConnection*
> CreateNewPipeline(
> On 2011/10/12 19:41:35, Matt Menke wrote:
>
>> nit:  Fix indent.
>>
>
> Done.
>
>
> http://codereview.chromium.**org/7289006/diff/101002/net/**
>
http/http_pipelined_host_pool.**h<http://codereview.chromium.org/7289006/diff/101002/net/http/http_pipelined_host_pool.h>
> File net/http/http_pipelined_host_**pool.h (right):
>
> http://codereview.chromium.**org/7289006/diff/101002/net/**
>
http/http_pipelined_host_pool.**h#newcode18<http://codereview.chromium.org/7289006/diff/101002/net/http/http_pipelined_host_pool.h#newcode18>
> net/http/http_pipelined_host_**pool.h:18: class HttpStreamFactory;
> On 2011/10/12 19:41:35, Matt Menke wrote:
>
>> nit: Alphabetize.
>>
>
> Done.
>
>
http://codereview.chromium.**org/7289006/<http://codereview.chromium.org/7289...
>

[willchan no longer on Chromium, 2011-10-13 02:29:16]

lgtm

http://codereview.chromium.org/7289006/diff/68001/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/7289006/diff/68001/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:811: HttpPipelinedConnection* pipeline
= session_->http_pipelined_host_pool()->
On 2011/09/29 02:39:31, James Simonsen wrote:
> On 2011/09/26 23:49:20, willchan wrote:
> > It's not clear why the HttpPipelinedConnection* is exposed to
> > HttpStreamFactoryImpl::Job. It sort of seems like, from the Job*'s
> perspective,
> > we should just have an API on the HttpPipelinedHostPool that creates a
stream.
> > The HttpPipelinedConnection and HttpPipelinedHost are implementation details
> > behind HttpPipelinedHostPool.
> 
> Done.

I see. I feel like this is revealing a problem with the design, since I would
think that HttpStreamFactory should be able to hide the pipelining details. It
seems to me that the reason the tight coupling is getting introduced is we have
an issue with how we locate some of our HTTP state. It seems to me that the
HttpPipelinedHostPool should live in HttpStreamFactoryImpl instead of in
HttpNetworkSession. I think the same is true of SpdySessionPool. I should file a
SPDY bug to clean this up. Can you move HttpPipelinedHostPool to be a member of
HttpStreamFactoryImpl?

http://codereview.chromium.org/7289006/diff/79001/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/7289006/diff/79001/net/http/http_network_trans...
net/http/http_network_transaction.cc:866: next_state_ = STATE_READ_BODY;
On 2011/10/12 01:36:58, James Simonsen wrote:
> On 2011/09/29 19:32:48, willchan wrote:
> > I believe this is a bug. We should only transition to this state when Read()
> is
> > called.
> 
> It's a tricky situation. Maybe I should just forget about it.
> 
> If we were to be evicted after Read() was called, then after restarting, we
> shouldn't wait for Read() to be called again. We know Read() has been called
if
> we have a read_buf_len_.
> 
> Anyway, this shouldn't happen in practice, so I just removed it. I'd only seen
> it while playing with the unit test.

A new test? Because this shouldn't happen, since no one is allowed to call
Read() until the response headers have been returned. If someone does this, they
are broken.

http://codereview.chromium.org/7289006/diff/110001/net/http/http_network_tran...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/7289006/diff/110001/net/http/http_network_tran...
net/http/http_network_transaction.cc:1187: error =
RestartAfterPipelineEviction();
I think you can just use ResetConnectionAndRequestForResend(), right? And
actually, you might recreate the SSL CONNECT tunnel, so you probably should
clear out the headers. By the time you reach this point, stream_request_ has
already been reset, right?

I would really like to reuse the existing function because it reduces cognitive
overhead. HttpNetworkTransaction is complicated enough as is.

http://codereview.chromium.org/7289006/diff/110001/net/http/http_pipelined_co...
File net/http/http_pipelined_connection_impl_unittest.cc (right):

http://codereview.chromium.org/7289006/diff/110001/net/http/http_pipelined_co...
net/http/http_pipelined_connection_impl_unittest.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
I should really read this file. I'm not going to right now. I'll keep it in mind
for later.

[willchan no longer on Chromium, 2011-10-13 02:30:59]

Please update your changelist description to describe the state of the HTTP
pipelining implementation as of this changelist. And please note the areas that
need future work:
* Better structuring of HttpPipelinedConnection
* Testing (willchan is a failure for not reviewing in time)
etc

On 2011/10/13 02:29:16, willchan wrote:
> lgtm
> 
>
http://codereview.chromium.org/7289006/diff/68001/net/http/http_stream_factor...
> File net/http/http_stream_factory_impl_job.cc (right):
> 
>
http://codereview.chromium.org/7289006/diff/68001/net/http/http_stream_factor...
> net/http/http_stream_factory_impl_job.cc:811: HttpPipelinedConnection*
pipeline
> = session_->http_pipelined_host_pool()->
> On 2011/09/29 02:39:31, James Simonsen wrote:
> > On 2011/09/26 23:49:20, willchan wrote:
> > > It's not clear why the HttpPipelinedConnection* is exposed to
> > > HttpStreamFactoryImpl::Job. It sort of seems like, from the Job*'s
> > perspective,
> > > we should just have an API on the HttpPipelinedHostPool that creates a
> stream.
> > > The HttpPipelinedConnection and HttpPipelinedHost are implementation
details
> > > behind HttpPipelinedHostPool.
> > 
> > Done.
> 
> I see. I feel like this is revealing a problem with the design, since I would
> think that HttpStreamFactory should be able to hide the pipelining details. It
> seems to me that the reason the tight coupling is getting introduced is we
have
> an issue with how we locate some of our HTTP state. It seems to me that the
> HttpPipelinedHostPool should live in HttpStreamFactoryImpl instead of in
> HttpNetworkSession. I think the same is true of SpdySessionPool. I should file
a
> SPDY bug to clean this up. Can you move HttpPipelinedHostPool to be a member
of
> HttpStreamFactoryImpl?
> 
>
http://codereview.chromium.org/7289006/diff/79001/net/http/http_network_trans...
> File net/http/http_network_transaction.cc (right):
> 
>
http://codereview.chromium.org/7289006/diff/79001/net/http/http_network_trans...
> net/http/http_network_transaction.cc:866: next_state_ = STATE_READ_BODY;
> On 2011/10/12 01:36:58, James Simonsen wrote:
> > On 2011/09/29 19:32:48, willchan wrote:
> > > I believe this is a bug. We should only transition to this state when
Read()
> > is
> > > called.
> > 
> > It's a tricky situation. Maybe I should just forget about it.
> > 
> > If we were to be evicted after Read() was called, then after restarting, we
> > shouldn't wait for Read() to be called again. We know Read() has been called
> if
> > we have a read_buf_len_.
> > 
> > Anyway, this shouldn't happen in practice, so I just removed it. I'd only
seen
> > it while playing with the unit test.
> 
> A new test? Because this shouldn't happen, since no one is allowed to call
> Read() until the response headers have been returned. If someone does this,
they
> are broken.
> 
>
http://codereview.chromium.org/7289006/diff/110001/net/http/http_network_tran...
> File net/http/http_network_transaction.cc (right):
> 
>
http://codereview.chromium.org/7289006/diff/110001/net/http/http_network_tran...
> net/http/http_network_transaction.cc:1187: error =
> RestartAfterPipelineEviction();
> I think you can just use ResetConnectionAndRequestForResend(), right? And
> actually, you might recreate the SSL CONNECT tunnel, so you probably should
> clear out the headers. By the time you reach this point, stream_request_ has
> already been reset, right?
> 
> I would really like to reuse the existing function because it reduces
cognitive
> overhead. HttpNetworkTransaction is complicated enough as is.
> 
>
http://codereview.chromium.org/7289006/diff/110001/net/http/http_pipelined_co...
> File net/http/http_pipelined_connection_impl_unittest.cc (right):
> 
>
http://codereview.chromium.org/7289006/diff/110001/net/http/http_pipelined_co...
> net/http/http_pipelined_connection_impl_unittest.cc:1: // Copyright (c) 2011
The
> Chromium Authors. All rights reserved.
> I should really read this file. I'm not going to right now. I'll keep it in
mind
> for later.

[mmenke, 2011-10-13 03:02:48]

Going to give this another quick once over tomorrow before signing off.

http://codereview.chromium.org/7289006/diff/101002/net/http/http_network_tran...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/7289006/diff/101002/net/http/http_network_tran...
net/http/http_network_transaction.cc:1186: make_scoped_refptr(new
NetLogIntegerParameter("net_error", error)));
On 2011/10/12 21:55:51, James Simonsen wrote:
> On 2011/10/12 19:41:35, Matt Menke wrote:
> > nit:  I have a CL under review which lets you just do:
> > 
> > net_log_.AddEventWithNetErrorCode(
> >     NetLog::TYPE_HTTP_TRANSACTION_RESTART_AFTER_ERROR,
> >     error);
> > 
> > Hoping to land it in a day or two.
> 
> Nice. Will switch when it's ready.

No need to hold off committing on this.  Haven't heard back from one of my
reviewers yet - he may be out this week.

[James Simonsen, 2011-10-13 03:35:11]

I'll address the rest of the comments tomorrow. I just wanted to answer this
question before I left for the night...

http://codereview.chromium.org/7289006/diff/79001/net/http/http_network_trans...
> net/http/http_network_transaction.cc:866: next_state_ = STATE_READ_BODY;
> On 2011/10/12 01:36:58, James Simonsen wrote:
> > On 2011/09/29 19:32:48, willchan wrote:
> > > I believe this is a bug. We should only transition to this state when
Read()
> > is
> > > called.
> > 
> > It's a tricky situation. Maybe I should just forget about it.
> > 
> > If we were to be evicted after Read() was called, then after restarting, we
> > shouldn't wait for Read() to be called again. We know Read() has been called
> if
> > we have a read_buf_len_.
> > 
> > Anyway, this shouldn't happen in practice, so I just removed it. I'd only
seen
> > it while playing with the unit test.
> 
> A new test? Because this shouldn't happen, since no one is allowed to call
> Read() until the response headers have been returned. If someone does this,
they
> are broken.

Yeah, the test I was playing with was returning ERR_PIPELINE_EVICTION from
HttpPipelinedStream::ReadResponseBody(). This should never happen with the
existing code. Evictions should only happen during
HttpPipelinedStream::SendRequest() and
HttpPipelinedStream::ReadResponseHeaders(). In either of those cases, the
existing code is fine, because both of those occur before
HttpNetworkTransaction::Read() is called.

Down the road, I could imagine us returning ERR_PIPELINE_EVICTION from
ReadResponseBody() if an earlier transaction decides it got a garbage response,
and we shut pipelining down to prevent future errors. Right now, we don't have
the code to do that, and I'm not even sure how we would decide a response was
garbage. Apparently, Firefox has hooked the image decoders into the network
stack so that they can report errors like this.

Anyway, this is probably a ways down the road, so I'm not going to worry about
it now.

[willchan no longer on Chromium, 2011-10-13 03:36:31]

On Wed, Oct 12, 2011 at 8:35 PM, <simonjam@chromium.org> wrote:

> I'll address the rest of the comments tomorrow. I just wanted to answer
> this
> question before I left for the night...
>
>
> http://codereview.chromium.**org/7289006/diff/79001/net/**
>
http/http_network_transaction.**cc#newcode866<http://codereview.chromium.org/7289006/diff/79001/net/http/http_network_transaction.cc#newcode866>
>
>> net/http/http_network_**transaction.cc:866: next_state_ =
>> STATE_READ_BODY;
>> On 2011/10/12 01:36:58, James Simonsen wrote:
>> > On 2011/09/29 19:32:48, willchan wrote:
>> > > I believe this is a bug. We should only transition to this state when
>>
> Read()
>
>> > is
>> > > called.
>> >
>> > It's a tricky situation. Maybe I should just forget about it.
>> >
>> > If we were to be evicted after Read() was called, then after restarting,
>> we
>> > shouldn't wait for Read() to be called again. We know Read() has been
>> called
>> if
>> > we have a read_buf_len_.
>> >
>> > Anyway, this shouldn't happen in practice, so I just removed it. I'd
>> only
>>
> seen
>
>> > it while playing with the unit test.
>>
>
>  A new test? Because this shouldn't happen, since no one is allowed to call
>> Read() until the response headers have been returned. If someone does
>> this,
>>
> they
>
>> are broken.
>>
>
> Yeah, the test I was playing with was returning ERR_PIPELINE_EVICTION from
> HttpPipelinedStream::**ReadResponseBody(). This should never happen with
> the
> existing code. Evictions should only happen during
> HttpPipelinedStream::**SendRequest() and
> HttpPipelinedStream::**ReadResponseHeaders(). In either of those cases,
> the
> existing code is fine, because both of those occur before
> HttpNetworkTransaction::Read() is called.
>
> Down the road, I could imagine us returning ERR_PIPELINE_EVICTION from
> ReadResponseBody() if an earlier transaction decides it got a garbage
> response,
> and we shut pipelining down to prevent future errors. Right now, we don't
> have
> the code to do that, and I'm not even sure how we would decide a response
> was
> garbage. Apparently, Firefox has hooked the image decoders into the network
> stack so that they can report errors like this.
>
> Anyway, this is probably a ways down the road, so I'm not going to worry
> about
> it now.
>

OK


>
>
http://codereview.chromium.**org/7289006/<http://codereview.chromium.org/7289...
>

[James Simonsen, 2011-10-13 19:29:57]

http://codereview.chromium.org/7289006/diff/79001/net/http/http_stream_factory.h
File net/http/http_stream_factory.h (right):

http://codereview.chromium.org/7289006/diff/79001/net/http/http_stream_factor...
net/http/http_stream_factory.h:180: virtual void
OnHttpPipelinedHostHasAdditionalCapacity(
On 2011/09/29 23:36:02, willchan wrote:
> I consider pipelining an implementation detail. Not every HttpStreamFactory
> implementation needs to support it. Therefore, I think you should move this
> virtual to HttpStreamFactoryImpl.

Done.

http://codereview.chromium.org/7289006/diff/110001/net/http/http_network_tran...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/7289006/diff/110001/net/http/http_network_tran...
net/http/http_network_transaction.cc:1187: error =
RestartAfterPipelineEviction();
On 2011/10/13 02:29:17, willchan wrote:
> I think you can just use ResetConnectionAndRequestForResend(), right? And
> actually, you might recreate the SSL CONNECT tunnel, so you probably should
> clear out the headers. By the time you reach this point, stream_request_ has
> already been reset, right?
> 
> I would really like to reuse the existing function because it reduces
cognitive
> overhead. HttpNetworkTransaction is complicated enough as is.

Done.

[mmenke, 2011-10-14 16:21:26]

LGTM.

The M16 branch point is late this coming Monday evening/early Tuesday.  Mind
putting off landing until then?  Main reason is just to reduce the redundant
early bug reports we get.  Bug reports are great, but getting a lot of
duplicates prematurely just wastes time.

http://codereview.chromium.org/7289006/diff/124001/net/http/http_pipelined_co...
File net/http/http_pipelined_connection_impl_unittest.cc (right):

http://codereview.chromium.org/7289006/diff/124001/net/http/http_pipelined_co...
net/http/http_pipelined_connection_impl_unittest.cc:998: }
nit:  Missing blank line.

[commit-bot: I haz the power, 2011-10-19 18:40:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonjam@chromium.org/7289006/142001

[commit-bot: I haz the power, 2011-10-19 18:40:35]

Can't apply patch for file chrome/common/chrome_switches.cc.
While running patch -p1 --forward --force;
patching file chrome/common/chrome_switches.cc
Hunk #1 FAILED at 449.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/common/chrome_switches.cc.rej

[commit-bot: I haz the power, 2011-10-19 19:02:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonjam@chromium.org/7289006/146001

[commit-bot: I haz the power, 2011-10-19 20:14:33]

Change committed as 106364