Implement Drain() on HttpPipelinedStream.

Implement Drain() on HttpPipelinedStream.

BUG=102385
TEST=net_unittests


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

[mmenke, 2011-11-18 15:45:38]

I suggest you take a look at the "Important" comment before the others.

http://codereview.chromium.org/8591037/diff/1/net/http/http_pipelined_connect...
File net/http/http_pipelined_connection_impl_unittest.cc (right):

http://codereview.chromium.org/8591037/diff/1/net/http/http_pipelined_connect...
net/http/http_pipelined_connection_impl_unittest.cc:1089:
TEST_F(HttpPipelinedConnectionImplTest, RecoverFromDrainOnRedirect) {
Could you add a content-length: 0 test, too, out of general paranoia?  Looks
like we don't have one yet.

We should also have a test where draining fails - Just report an error on the
socket or something.

This is a complicated and important enough that we may want an async test as
well, and/or an integration test.  Fine to land this without one, but do think
it would be a good idea at some point.

http://codereview.chromium.org/8591037/diff/1/net/http/http_pipelined_stream.cc
File net/http/http_pipelined_stream.cc (right):

http://codereview.chromium.org/8591037/diff/1/net/http/http_pipelined_stream....
net/http/http_pipelined_stream.cc:127: // the connection than to close it and
start over.
nit:  Think we don't need this TODO any more, unless you mean in the case that
we've only sent out one request over the wire.

http://codereview.chromium.org/8591037/diff/1/net/http/http_pipelined_stream....
net/http/http_pipelined_stream.cc:134:
GetResponseInfo()->headers->GetContentLength());
Important:  This doesn't work with chunked encoding.  We'll need a test for
that, too.

Cancelled or otherwise in-need-of-draining chunk-encoded requests may be rare
enough that we can just cancel the transfer, first pass, and worry about doing
something smart in another CL.  Or you can try and handle it in this one, up to
you.  May require a fair bit of work to handle correctly, I'm not familiar with
the specifics.

http://codereview.chromium.org/8591037/diff/1/net/http/http_response_body_dra...
File net/http/http_response_body_drainer.cc (right):

http://codereview.chromium.org/8591037/diff/1/net/http/http_response_body_dra...
net/http/http_response_body_drainer.cc:35: read_buf_ = new IOBuffer(read_size_);
This could be a very large amount of memory, for, say, a download.  I'd suggest
just failing if num_bytes_to_drain > kDrainBodyBufferSize.  And we need a unit
test for that case, of course.

[willchan no longer on Chromium, 2011-11-20 18:09:21]

http://codereview.chromium.org/8591037/diff/1/net/http/http_pipelined_stream.cc
File net/http/http_pipelined_stream.cc (right):

http://codereview.chromium.org/8591037/diff/1/net/http/http_pipelined_stream....
net/http/http_pipelined_stream.cc:124: void
HttpPipelinedStream::Drain(HttpNetworkSession* session) {
I sort of suspect we want to defer the actual drain logic to the underlying
|pipeline_|, since it can probably make a better decision. It'll know how deep
the pipeline is. I guess the deeper the pipeline, the less willing we should be
to throw away the connection.

[James Simonsen, 2011-11-22 01:24:19]

http://codereview.chromium.org/8591037/diff/1/net/http/http_pipelined_connect...
File net/http/http_pipelined_connection_impl_unittest.cc (right):

http://codereview.chromium.org/8591037/diff/1/net/http/http_pipelined_connect...
net/http/http_pipelined_connection_impl_unittest.cc:1089:
TEST_F(HttpPipelinedConnectionImplTest, RecoverFromDrainOnRedirect) {
On 2011/11/18 15:45:38, Matt Menke wrote:
> Could you add a content-length: 0 test, too, out of general paranoia?  Looks
> like we don't have one yet.
> 
> We should also have a test where draining fails - Just report an error on the
> socket or something.
> 
> This is a complicated and important enough that we may want an async test as
> well, and/or an integration test.  Fine to land this without one, but do think
> it would be a good idea at some point.

Done. They all sounded good, so I added all of them in various places.

http://codereview.chromium.org/8591037/diff/1/net/http/http_pipelined_stream.cc
File net/http/http_pipelined_stream.cc (right):

http://codereview.chromium.org/8591037/diff/1/net/http/http_pipelined_stream....
net/http/http_pipelined_stream.cc:124: void
HttpPipelinedStream::Drain(HttpNetworkSession* session) {
On 2011/11/20 18:09:21, willchan wrote:
> I sort of suspect we want to defer the actual drain logic to the underlying
> |pipeline_|, since it can probably make a better decision. It'll know how deep
> the pipeline is. I guess the deeper the pipeline, the less willing we should
be
> to throw away the connection.

Done.

http://codereview.chromium.org/8591037/diff/1/net/http/http_pipelined_stream....
net/http/http_pipelined_stream.cc:127: // the connection than to close it and
start over.
On 2011/11/18 15:45:38, Matt Menke wrote:
> nit:  Think we don't need this TODO any more, unless you mean in the case that
> we've only sent out one request over the wire.

I want to be able to recover without calling Close(true), which will disconnect
the socket. That means evicting everything, draining all of the content, but
leaving the socket usable.

I'm not sure that's even something we can do reliably though. For instance, we
might have front-of-queue blocking on one of the responses and assume we've
drained it all, only to get the rest of the response after we restart the
pipeline.

I think I've talked myself out of this now, so yeah, I'll delete that comment.

http://codereview.chromium.org/8591037/diff/1/net/http/http_pipelined_stream....
net/http/http_pipelined_stream.cc:134:
GetResponseInfo()->headers->GetContentLength());
On 2011/11/18 15:45:38, Matt Menke wrote:
> Important:  This doesn't work with chunked encoding.  We'll need a test for
> that, too.
> 
> Cancelled or otherwise in-need-of-draining chunk-encoded requests may be rare
> enough that we can just cancel the transfer, first pass, and worry about doing
> something smart in another CL.  Or you can try and handle it in this one, up
to
> you.  May require a fair bit of work to handle correctly, I'm not familiar
with
> the specifics.

Thanks. Didn't know anything about chunked encoding. I'm going to pass on diving
in right now. Sometime later, I'll add a metric to see how often this happens in
the wild and decide whether or not we should tackle it.

http://codereview.chromium.org/8591037/diff/1/net/http/http_response_body_dra...
File net/http/http_response_body_drainer.cc (right):

http://codereview.chromium.org/8591037/diff/1/net/http/http_response_body_dra...
net/http/http_response_body_drainer.cc:35: read_buf_ = new IOBuffer(read_size_);
On 2011/11/18 15:45:38, Matt Menke wrote:
> This could be a very large amount of memory, for, say, a download.  I'd
suggest
> just failing if num_bytes_to_drain > kDrainBodyBufferSize.  And we need a unit
> test for that case, of course.

Done.

[mmenke, 2011-11-28 18:39:10]

LGTM, modulo comments.

http://codereview.chromium.org/8591037/diff/6001/net/http/http_pipelined_conn...
File net/http/http_pipelined_connection_impl_unittest.cc (right):

http://codereview.chromium.org/8591037/diff/6001/net/http/http_pipelined_conn...
net/http/http_pipelined_connection_impl_unittest.cc:1192: "Content-Length:
8\r\n\r\n"),
Remove "Content-Length", as it's not allowed to appear with chuncked encoding. 
While we currently just ignore "Content-Length" when it appears with
"Transfer-Encoding", it's best not for the correctness of the test to depend on
that.

http://codereview.chromium.org/8591037/diff/6001/net/http/http_pipelined_netw...
File net/http/http_pipelined_network_transaction_unittest.cc (right):

http://codereview.chromium.org/8591037/diff/6001/net/http/http_pipelined_netw...
net/http/http_pipelined_network_transaction_unittest.cc:459:
data_vector_[0]->SetStop(10);
Can we just do:

data_vector_[0]->SetStop(10);
data_vector_[0]->Run();

I think the 2 is a little hard to follow.

http://codereview.chromium.org/8591037/diff/6001/net/http/http_pipelined_stre...
File net/http/http_pipelined_stream.cc (right):

http://codereview.chromium.org/8591037/diff/6001/net/http/http_pipelined_stre...
net/http/http_pipelined_stream.cc:13: #include "net/http/http_response_info.h"
nit:  Is this needed?  We don't seem to do more than pass HttpResponseInfo
pointers around, so a forward declaration should be enough.

http://codereview.chromium.org/8591037/diff/6001/net/http/http_response_body_...
File net/http/http_response_body_drainer.cc (right):

http://codereview.chromium.org/8591037/diff/6001/net/http/http_response_body_...
net/http/http_response_body_drainer.cc:34: if (num_bytes_to_drain >
kDrainBodyBufferSize) {
Just thinking out loud:  A larger minimum may be better for pipelined streams if
we've already sent another request over the wire.  Maybe be worth looking into,
at some point.  Fairly low priority, of course.

[James Simonsen, 2011-11-29 23:57:28]

http://codereview.chromium.org/8591037/diff/6001/net/http/http_pipelined_conn...
File net/http/http_pipelined_connection_impl_unittest.cc (right):

http://codereview.chromium.org/8591037/diff/6001/net/http/http_pipelined_conn...
net/http/http_pipelined_connection_impl_unittest.cc:1192: "Content-Length:
8\r\n\r\n"),
On 2011/11/28 18:39:10, Matt Menke wrote:
> Remove "Content-Length", as it's not allowed to appear with chuncked encoding.

> While we currently just ignore "Content-Length" when it appears with
> "Transfer-Encoding", it's best not for the correctness of the test to depend
on
> that.

Done.

http://codereview.chromium.org/8591037/diff/6001/net/http/http_pipelined_netw...
File net/http/http_pipelined_network_transaction_unittest.cc (right):

http://codereview.chromium.org/8591037/diff/6001/net/http/http_pipelined_netw...
net/http/http_pipelined_network_transaction_unittest.cc:459:
data_vector_[0]->SetStop(10);
On 2011/11/28 18:39:10, Matt Menke wrote:
> Can we just do:
> 
> data_vector_[0]->SetStop(10);
> data_vector_[0]->Run();
> 
> I think the 2 is a little hard to follow.

Tried it, but it hangs in Run().

Honestly, the mock stream can be very subtle. Every time I think I have it
figured out, one of my tests starts hanging or fails. Sometimes it takes trial
and error to come up with a working combination.

http://codereview.chromium.org/8591037/diff/6001/net/http/http_pipelined_stre...
File net/http/http_pipelined_stream.cc (right):

http://codereview.chromium.org/8591037/diff/6001/net/http/http_pipelined_stre...
net/http/http_pipelined_stream.cc:13: #include "net/http/http_response_info.h"
On 2011/11/28 18:39:10, Matt Menke wrote:
> nit:  Is this needed?  We don't seem to do more than pass HttpResponseInfo
> pointers around, so a forward declaration should be enough.

Done.

http://codereview.chromium.org/8591037/diff/6001/net/http/http_response_body_...
File net/http/http_response_body_drainer.cc (right):

http://codereview.chromium.org/8591037/diff/6001/net/http/http_response_body_...
net/http/http_response_body_drainer.cc:34: if (num_bytes_to_drain >
kDrainBodyBufferSize) {
On 2011/11/28 18:39:10, Matt Menke wrote:
> Just thinking out loud:  A larger minimum may be better for pipelined streams
if
> we've already sent another request over the wire.  Maybe be worth looking
into,
> at some point.  Fairly low priority, of course.

Good point. I added a TODO.

[mmenke, 2011-11-30 00:52:59]

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

http://codereview.chromium.org/8591037/diff/14001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:614: if
(!stream->CanFindEndOfResponse() || headers->IsChunkEncoded()) {
One more comment.  Feel free to worry about it later.  I'm pretty sure we can
end up here and be not usable (Maybe another request was pending?).  If so, we
can probably just delete the stream, too.

[James Simonsen, 2011-11-30 19:39:14]

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

http://codereview.chromium.org/8591037/diff/14001/net/http/http_pipelined_con...
net/http/http_pipelined_connection_impl.cc:614: if
(!stream->CanFindEndOfResponse() || headers->IsChunkEncoded()) {
On 2011/11/30 00:52:59, Matt Menke wrote:
> One more comment.  Feel free to worry about it later.  I'm pretty sure we can
> end up here and be not usable (Maybe another request was pending?).  If so, we
> can probably just delete the stream, too.

Done.

[commit-bot: I haz the power, 2011-11-30 19:39:24]

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

[commit-bot: I haz the power, 2011-11-30 21:20:01]

Change committed as 112289