Ensure pipelined requests are sent in the same order they're queued.

Ensure pipelined requests are sent in the same order they're queued. Also, ensure HttpPipeliningCompatibilityClient closes the stream when it's done with a request.

BUG=110794
TEST=net_unittests and unit_tests


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

[James Simonsen, 2012-03-01 22:23:13]

The next couple patches are much smaller than the last one.

This is something I noticed while testing against the actual server. We request
the server stats before we've sent all of the other test requests.

[mmenke, 2012-03-02 18:55:55]

Changes all look fine to me, but why are the HttpPipeliningCompatibilityClient
changes needed?

https://chromiumcodereview.appspot.com/9567025/diff/1/net/http/http_pipelined...
File net/http/http_pipelined_network_transaction_unittest.cc (right):

https://chromiumcodereview.appspot.com/9567025/diff/1/net/http/http_pipelined...
net/http/http_pipelined_network_transaction_unittest.cc:867: MockWrite(ASYNC, 0,
"GET /one.html HTTP/1.1\r\n"
nit:  Think this is a little easier to read if the "GET..." is on the next line.

[James Simonsen, 2012-03-02 20:17:35]

On 2012/03/02 18:55:55, Matt Menke wrote:
> Changes all look fine to me, but why are the HttpPipeliningCompatibilityClient
> changes needed?

I updated the description to mention it.

Basically, there's a code path through HttpNetworkTransaction (I don't remember
which exactly) where it doesn't Close() the HttpStream until the transaction is
destructed. That was blocking the rest of the pipelined requests from finishing.
I think I only tickled it because I'd screwed up some of the server code. But,
we ought to be safe. Besides, this is the proper way to use URLRequest.

This is similar to a point Will brought up earlier: we should buffer in
HttpPipelinedStream so that one stream can't hold up the rest of the pipeline.
Now, we should make sure it calls Close() too. I've made a note for myself.

https://chromiumcodereview.appspot.com/9567025/diff/1/net/http/http_pipelined...
> File net/http/http_pipelined_network_transaction_unittest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9567025/diff/1/net/http/http_pipelined...
> net/http/http_pipelined_network_transaction_unittest.cc:867: MockWrite(ASYNC,
0,
> "GET /one.html HTTP/1.1\r\n"
> nit:  Think this is a little easier to read if the "GET..." is on the next
line.

Done.

[mmenke, 2012-03-02 20:23:52]

On 2012/03/02 20:17:35, James Simonsen wrote:
> On 2012/03/02 18:55:55, Matt Menke wrote:
> > Changes all look fine to me, but why are the
HttpPipeliningCompatibilityClient
> > changes needed?
> 
> I updated the description to mention it.
> 
> Basically, there's a code path through HttpNetworkTransaction (I don't
remember
> which exactly) where it doesn't Close() the HttpStream until the transaction
is
> destructed. That was blocking the rest of the pipelined requests from
finishing.
> I think I only tickled it because I'd screwed up some of the server code. But,
> we ought to be safe. Besides, this is the proper way to use URLRequest.
> 
> This is similar to a point Will brought up earlier: we should buffer in
> HttpPipelinedStream so that one stream can't hold up the rest of the pipeline.
> Now, we should make sure it calls Close() too. I've made a note for myself.

Makes sense.  LGTM.

[commit-bot: I haz the power, 2012-03-02 22:01:39]

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

[commit-bot: I haz the power, 2012-03-03 00:42:34]

Change committed as 124793