Http Pipelinining: Make unit tests more friendly to greedily reading from sockets.

Http Pipelinining:  Make unit tests more friendly to greedily
reading from sockets.

In particular, prevent the parser from reading beyond all ReadData in
HttpPipelinedConnectionImplTest.ConnectionSuddenlyClosedAfterResponse,
and fix a test that was trying to reuse a connection when
an earlier stream failed on it.

BUG=166915
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175423

[mmenke, 2013-01-07 21:57:15]

https://codereview.chromium.org/11795003/diff/6001/net/http/http_pipelined_co...
File net/http/http_pipelined_connection_impl_unittest.cc (right):

https://codereview.chromium.org/11795003/diff/6001/net/http/http_pipelined_co...
net/http/http_pipelined_connection_impl_unittest.cc:578: //
ERR_SOCKET_NOT_CONNECTED.
Greedily reading data would run right off the end of the list.

https://codereview.chromium.org/11795003/diff/6001/net/http/http_pipelined_co...
net/http/http_pipelined_connection_impl_unittest.cc:1033: data_->RunFor(1);
This test was really weird - we have a stream fail, but rather than closing it
as non-reusable, we were just deleting it.  At this point we also have an async
read pending when we switch from one parser to the other.  HttpStreamParser also
doesn't expect us to try and reuse the socket after reading only part of a
response, and as a result, the second parser is passed the leftover headers from
the first HttpStreamParser.

My refactoring doesn't like this code.  It's sufficiently weird that I don't
think it's worth figuring out just which of these conditions results in
different behavior in my refactored code.  The StreamDeleter::Close(true) call
prevents the connection from being reused, and result in eviction of the second
stream, which what the test is supposed to do, anyways.

[James Simonsen, 2013-01-07 22:02:24]

lgtm

https://codereview.chromium.org/11795003/diff/6001/net/http/http_pipelined_co...
File net/http/http_pipelined_connection_impl_unittest.cc (right):

https://codereview.chromium.org/11795003/diff/6001/net/http/http_pipelined_co...
net/http/http_pipelined_connection_impl_unittest.cc:1033: data_->RunFor(1);
On 2013/01/07 21:57:15, Matt Menke wrote:
> This test was really weird - we have a stream fail, but rather than closing it
> as non-reusable, we were just deleting it.  At this point we also have an
async
> read pending when we switch from one parser to the other.  HttpStreamParser
also
> doesn't expect us to try and reuse the socket after reading only part of a
> response, and as a result, the second parser is passed the leftover headers
from
> the first HttpStreamParser.
> 
> My refactoring doesn't like this code.  It's sufficiently weird that I don't
> think it's worth figuring out just which of these conditions results in
> different behavior in my refactored code.  The StreamDeleter::Close(true) call
> prevents the connection from being reused, and result in eviction of the
second
> stream, which what the test is supposed to do, anyways.

IIRC, this was the reproduction of a crash in the old code. If you've refactored
it away, then I don't think we need to worry about it.

[mmenke, 2013-01-07 22:05:47]

On 2013/01/07 22:02:24, James Simonsen wrote:
> lgtm
> 
>
https://codereview.chromium.org/11795003/diff/6001/net/http/http_pipelined_co...
> File net/http/http_pipelined_connection_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/11795003/diff/6001/net/http/http_pipelined_co...
> net/http/http_pipelined_connection_impl_unittest.cc:1033: data_->RunFor(1);
> On 2013/01/07 21:57:15, Matt Menke wrote:
> > This test was really weird - we have a stream fail, but rather than closing
it
> > as non-reusable, we were just deleting it.  At this point we also have an
> async
> > read pending when we switch from one parser to the other.  HttpStreamParser
> also
> > doesn't expect us to try and reuse the socket after reading only part of a
> > response, and as a result, the second parser is passed the leftover headers
> from
> > the first HttpStreamParser.
> > 
> > My refactoring doesn't like this code.  It's sufficiently weird that I don't
> > think it's worth figuring out just which of these conditions results in
> > different behavior in my refactored code.  The StreamDeleter::Close(true)
call
> > prevents the connection from being reused, and result in eviction of the
> second
> > stream, which what the test is supposed to do, anyways.
> 
> IIRC, this was the reproduction of a crash in the old code. If you've
refactored
> it away, then I don't think we need to worry about it.

Are you sure we were deleting a stream with a pending read, that was unusable,
without marking it as unreusable?

If we were marking it as unreuseable - that certainly can happen, but not
marking as unreusable shouldn't happen, unless there's a bug elsewhere.

[mmenke, 2013-01-07 22:13:26]

On 2013/01/07 22:05:47, Matt Menke wrote:
> On 2013/01/07 22:02:24, James Simonsen wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/11795003/diff/6001/net/http/http_pipelined_co...
> > File net/http/http_pipelined_connection_impl_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/11795003/diff/6001/net/http/http_pipelined_co...
> > net/http/http_pipelined_connection_impl_unittest.cc:1033: data_->RunFor(1);
> > On 2013/01/07 21:57:15, Matt Menke wrote:
> > > This test was really weird - we have a stream fail, but rather than
closing
> it
> > > as non-reusable, we were just deleting it.  At this point we also have an
> > async
> > > read pending when we switch from one parser to the other. 
HttpStreamParser
> > also
> > > doesn't expect us to try and reuse the socket after reading only part of a
> > > response, and as a result, the second parser is passed the leftover
headers
> > from
> > > the first HttpStreamParser.
> > > 
> > > My refactoring doesn't like this code.  It's sufficiently weird that I
don't
> > > think it's worth figuring out just which of these conditions results in
> > > different behavior in my refactored code.  The StreamDeleter::Close(true)
> call
> > > prevents the connection from being reused, and result in eviction of the
> > second
> > > stream, which what the test is supposed to do, anyways.
> > 
> > IIRC, this was the reproduction of a crash in the old code. If you've
> refactored
> > it away, then I don't think we need to worry about it.
> 
> Are you sure we were deleting a stream with a pending read, that was unusable,
> without marking it as unreusable?
> 
> If we were marking it as unreuseable - that certainly can happen, but not
> marking as unreusable shouldn't happen, unless there's a bug elsewhere.

Oops - I still had the close commented out in that upload.  Sorry if that was
confusing you.  Was still trying to figure out why there was a difference when I
decided it wasn't worth the effort.

[James Simonsen, 2013-01-07 22:40:23]

On 2013/01/07 22:13:26, Matt Menke wrote:
> On 2013/01/07 22:05:47, Matt Menke wrote:
> > On 2013/01/07 22:02:24, James Simonsen wrote:
> > > lgtm
> > > 
> > >
> >
>
https://codereview.chromium.org/11795003/diff/6001/net/http/http_pipelined_co...
> > > File net/http/http_pipelined_connection_impl_unittest.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/11795003/diff/6001/net/http/http_pipelined_co...
> > > net/http/http_pipelined_connection_impl_unittest.cc:1033:
data_->RunFor(1);
> > > On 2013/01/07 21:57:15, Matt Menke wrote:
> > > > This test was really weird - we have a stream fail, but rather than
> closing
> > it
> > > > as non-reusable, we were just deleting it.  At this point we also have
an
> > > async
> > > > read pending when we switch from one parser to the other. 
> HttpStreamParser
> > > also
> > > > doesn't expect us to try and reuse the socket after reading only part of
a
> > > > response, and as a result, the second parser is passed the leftover
> headers
> > > from
> > > > the first HttpStreamParser.
> > > > 
> > > > My refactoring doesn't like this code.  It's sufficiently weird that I
> don't
> > > > think it's worth figuring out just which of these conditions results in
> > > > different behavior in my refactored code.  The
StreamDeleter::Close(true)
> > call
> > > > prevents the connection from being reused, and result in eviction of the
> > > second
> > > > stream, which what the test is supposed to do, anyways.
> > > 
> > > IIRC, this was the reproduction of a crash in the old code. If you've
> > refactored
> > > it away, then I don't think we need to worry about it.
> > 
> > Are you sure we were deleting a stream with a pending read, that was
unusable,
> > without marking it as unreusable?
> > 
> > If we were marking it as unreuseable - that certainly can happen, but not
> > marking as unreusable shouldn't happen, unless there's a bug elsewhere.
> 
> Oops - I still had the close commented out in that upload.  Sorry if that was
> confusing you.  Was still trying to figure out why there was a difference when
I
> decided it wasn't worth the effort.

No, I'm not sure. I don't remember enough of the detail. Sorry. :(

I agree it seems like a situation that shouldn't happen.

I know we improved the handling of reusable pipelines in later CLs. This test
comes from the original CL. It could be that this scenario never happens in
practice now.

[mmenke, 2013-01-07 22:56:31]

On 2013/01/07 22:40:23, James Simonsen wrote:
> On 2013/01/07 22:13:26, Matt Menke wrote:
> > On 2013/01/07 22:05:47, Matt Menke wrote:
> > > On 2013/01/07 22:02:24, James Simonsen wrote:
> > > > lgtm
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/11795003/diff/6001/net/http/http_pipelined_co...
> > > > File net/http/http_pipelined_connection_impl_unittest.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/11795003/diff/6001/net/http/http_pipelined_co...
> > > > net/http/http_pipelined_connection_impl_unittest.cc:1033:
> data_->RunFor(1);
> > > > On 2013/01/07 21:57:15, Matt Menke wrote:
> > > > > This test was really weird - we have a stream fail, but rather than
> > closing
> > > it
> > > > > as non-reusable, we were just deleting it.  At this point we also have
> an
> > > > async
> > > > > read pending when we switch from one parser to the other. 
> > HttpStreamParser
> > > > also
> > > > > doesn't expect us to try and reuse the socket after reading only part
of
> a
> > > > > response, and as a result, the second parser is passed the leftover
> > headers
> > > > from
> > > > > the first HttpStreamParser.
> > > > > 
> > > > > My refactoring doesn't like this code.  It's sufficiently weird that I
> > don't
> > > > > think it's worth figuring out just which of these conditions results
in
> > > > > different behavior in my refactored code.  The
> StreamDeleter::Close(true)
> > > call
> > > > > prevents the connection from being reused, and result in eviction of
the
> > > > second
> > > > > stream, which what the test is supposed to do, anyways.
> > > > 
> > > > IIRC, this was the reproduction of a crash in the old code. If you've
> > > refactored
> > > > it away, then I don't think we need to worry about it.
> > > 
> > > Are you sure we were deleting a stream with a pending read, that was
> unusable,
> > > without marking it as unreusable?
> > > 
> > > If we were marking it as unreuseable - that certainly can happen, but not
> > > marking as unreusable shouldn't happen, unless there's a bug elsewhere.
> > 
> > Oops - I still had the close commented out in that upload.  Sorry if that
was
> > confusing you.  Was still trying to figure out why there was a difference
when
> I
> > decided it wasn't worth the effort.
> 
> No, I'm not sure. I don't remember enough of the detail. Sorry. :(
> 
> I agree it seems like a situation that shouldn't happen.
> 
> I know we improved the handling of reusable pipelines in later CLs. This test
> comes from the original CL. It could be that this scenario never happens in
> practice now.

Thanks anyways.

I'll go ahead and land this, and assume there was another bug that resulted in
the issue.  Looks like it was from the original HttpPipelining commit, so may
well have been due to a pipelining bug that was fixed before we even landed the
first CL.  Or could have surfaced only when messing with unit tests.

[commit-bot: I haz the power, 2013-01-07 22:58:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/11795003/15001