Refactor HttpNetworkTransaction to eliminate the SPDY...

Refactor HttpNetworkTransaction to eliminate the SPDY
specific states of the state machine.  This required
adding two new states:

       STATE_INIT_STREAM
       STATE_INTI_STREAM_COMPLETE

The http_stream_ and spdy_http_stream_ member fields
have been removed, and replaced by a single stream_
member field which is initialized with either an
HttpBasicStream, or SpdyHttpStream depending on the
underlying connection.

In the process, the NetLog no longer receives TYPE_SPDY
events, only TYPE_HTTP, so spdy_network_transaction_unittest.cc
needed to be modified accordingly.

BUG=50267
TEST=none


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

[Mike Belshe, 2010-08-03 20:37:02]

Overall - looks great - just that one question

http://codereview.chromium.org/3064033/diff/1/2
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/3064033/diff/1/2#newcode1176
net/http/http_network_transaction.cc:1176: }
This is unfortunate (forking this state for spdy/non-spdy); so just using the
HTTP path failed?

[vandebo (ex-Chrome), 2010-08-03 21:41:06]

http://codereview.chromium.org/3064033/diff/6001/7001
File net/http/http_network_transaction.cc (left):

http://codereview.chromium.org/3064033/diff/6001/7001#oldcode1313
net/http/http_network_transaction.cc:1313: int
HttpNetworkTransaction::DoSpdyGetStream() {
Oh, I see, you just moved this code.  Well maybe you can clean it up a little as
I suggested?

http://codereview.chromium.org/3064033/diff/6001/7001
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/3064033/diff/6001/7001#newcode435
net/http/http_network_transaction.cc:435:
DCHECK(stream_->GetResponseInfo()->headers);
I think this can come out of the conditional.

http://codereview.chromium.org/3064033/diff/6001/7001#newcode493
net/http/http_network_transaction.cc:493: ! using_spdy_ &&
no space

http://codereview.chromium.org/3064033/diff/6001/7001#newcode506
net/http/http_network_transaction.cc:506:
static_cast<SpdyHttpStream*>(stream_.get())->Cancel();
Maybe Cancel should be added to the HttpStream interface?  For standard Http,
you could have it disconnect the socket.

http://codereview.chromium.org/3064033/diff/6001/7001#newcode972
net/http/http_network_transaction.cc:972: } else {
No need for an else here.  The rest of the code is mis-indented for it anyway.

http://codereview.chromium.org/3064033/diff/6001/7001#newcode975
net/http/http_network_transaction.cc:975: // First we get a SPDY session. 
Theoretically, we've just negotiated one, but
This comment is misleading - it sounds like you might just throw away a spdy
stream.  In fact you didn't create one if you already had one.

http://codereview.chromium.org/3064033/diff/6001/7001#newcode983
net/http/http_network_transaction.cc:983: if (spdy_pool->HasSession(pair)) {
Why not if(!spdy_session_.get()) ?

http://codereview.chromium.org/3064033/diff/6001/7001#newcode1003
net/http/http_network_transaction.cc:1003: spdy_session_.release();
This will probably give you a compile warning -> error (ignoring the result of
release), just move it into the previous line.

http://codereview.chromium.org/3064033/diff/6001/7001#newcode1096
net/http/http_network_transaction.cc:1096: if (request_headers_.empty()) {
&& !using_spdy_ will save a bit of work - since spdy ignores the passed in
headers, right?

http://codereview.chromium.org/3064033/diff/6001/7001#newcode1129
net/http/http_network_transaction.cc:1129: return
stream_->SendRequest(request_headers_, request_body,
room for &response_ ?

http://codereview.chromium.org/3064033/diff/6001/7001#newcode1156
net/http/http_network_transaction.cc:1156: if (using_spdy_) {
This is fine, but you might be able to integrate it better.  Mike might want
some of the error handling that is currently only done for Http results...

http://codereview.chromium.org/3064033/diff/6001/7001#newcode1273
net/http/http_network_transaction.cc:1273: if (!using_spdy_)
Maybe HttpStream should also have an is_initialized() method?

http://codereview.chromium.org/3064033/diff/6001/7001#newcode1299
net/http/http_network_transaction.cc:1299: if (!keep_alive)
can put the spdy code down here.

http://codereview.chromium.org/3064033/diff/6001/7001#newcode1638
net/http/http_network_transaction.cc:1638: ShouldResendFailedRequest(error);
It would be nice to unify the ShouldResetFailedRequest calls.  But maybe it's
best to wait for the next phase of refactoring to do that (presenting a spdy
session as a ClientSocketHandle).

http://codereview.chromium.org/3064033/diff/6001/7002
File net/http/http_network_transaction.h (right):

http://codereview.chromium.org/3064033/diff/6001/7002#newcode39
net/http/http_network_transaction.h:39: class SpdyHttpStream;
SpdyHttpStream is no longer needed

[rch (use chromium not google), 2010-08-03 21:45:10]

http://codereview.chromium.org/3064033/diff/1/2
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/3064033/diff/1/2#newcode1176
net/http/http_network_transaction.cc:1176: }
On 2010/08/03 20:37:02, Mike Belshe wrote:
> This is unfortunate (forking this state for spdy/non-spdy); so just using the
> HTTP path failed?

I totally agree; this is definitely unfortunate.  Just using the HTTP path
caused HttpNetworkTransactionTest.UseAlternateProtocolForNpnSpdy to crash on
this line:

    SSLClientSocket* ssl_socket =
        static_cast<SSLClientSocket*>(connection_->socket());

net_unittests: ./base/scoped_ptr.h:96: C* scoped_ptr<C>::operator->() const
[with C = net::ClientSocketHandle]: Assertion `ptr_ != __null' failed.

I'm sure more work could be done to merge these two paths (and probably should
be done).  At the moment, I think it might be a good idea to land this patch so
we have a stake in the ground.

[rch (use chromium not google), 2010-08-03 22:17:12]

http://codereview.chromium.org/3064033/diff/6001/7001
File net/http/http_network_transaction.cc (left):

http://codereview.chromium.org/3064033/diff/6001/7001#oldcode1313
net/http/http_network_transaction.cc:1313: int
HttpNetworkTransaction::DoSpdyGetStream() {
On 2010/08/03 21:41:06, vandebo wrote:
> Oh, I see, you just moved this code.  Well maybe you can clean it up a little
as
> I suggested?

Your suggestions are good ones.  I've addressed most of them, I think.

http://codereview.chromium.org/3064033/diff/6001/7001
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/3064033/diff/6001/7001#newcode435
net/http/http_network_transaction.cc:435:
DCHECK(stream_->GetResponseInfo()->headers);
On 2010/08/03 21:41:06, vandebo wrote:
> I think this can come out of the conditional.

Done.

http://codereview.chromium.org/3064033/diff/6001/7001#newcode493
net/http/http_network_transaction.cc:493: ! using_spdy_ &&
On 2010/08/03 21:41:06, vandebo wrote:
> no space

Done.

http://codereview.chromium.org/3064033/diff/6001/7001#newcode506
net/http/http_network_transaction.cc:506:
static_cast<SpdyHttpStream*>(stream_.get())->Cancel();
On 2010/08/03 21:41:06, vandebo wrote:
> Maybe Cancel should be added to the HttpStream interface?  For standard Http,
> you could have it disconnect the socket.

Agreed.  HttpNetworkTransaction is currently a bit schizophrenic about the
relationship between a HTTP stream and the connection.  Some times the code trys
to work with the stream, and other times with the connection.  Yuck!  I'm
hopeful that Mike's StreamFactory will help by hiding the connection from
HttpNetworkTransaction :>

I like the idea of adding Connect to HttpStream.  Are you thinking that
HttpBasicStream::Cancel() would basically implement the earlier block?  Namely:

  // If we still have an open socket, then make sure to disconnect it so it     
                                                                                
 
  // won't call us back and we don't try to reuse it later on. However,         
                                                                                
 
  // don't close the socket if we should keep the connection alive.             
                                                                                
 
  if (connection_.get() && connection_->is_initialized()) {
    // The STATE_NONE check guarantees there are no pending socket IOs that     
                                                                                
 
    // could try to call this object back after it is deleted.                  
                                                                                
 
    bool keep_alive = next_state_ == STATE_NONE &&
                      !using_spdy_ &&
                      stream_.get() &&
                      stream_->IsResponseBodyComplete() &&
                      stream_->CanFindEndOfResponse() &&
                      GetResponseHeaders()->IsKeepAlive();
    if (!keep_alive)
      connection_->socket()->Disconnect();
  }

http://codereview.chromium.org/3064033/diff/6001/7001#newcode972
net/http/http_network_transaction.cc:972: } else {
On 2010/08/03 21:41:06, vandebo wrote:
> No need for an else here.  The rest of the code is mis-indented for it anyway.

Done.

http://codereview.chromium.org/3064033/diff/6001/7001#newcode975
net/http/http_network_transaction.cc:975: // First we get a SPDY session. 
Theoretically, we've just negotiated one, but
On 2010/08/03 21:41:06, vandebo wrote:
> This comment is misleading - it sounds like you might just throw away a spdy
> stream.  In fact you didn't create one if you already had one.

I'm glad you think so!  I was tempted to remove it, but wasn't sure if I was
just being dense.  Done.

http://codereview.chromium.org/3064033/diff/6001/7001#newcode983
net/http/http_network_transaction.cc:983: if (spdy_pool->HasSession(pair)) {
On 2010/08/03 21:41:06, vandebo wrote:
> Why not if(!spdy_session_.get()) ?

Argh!  You're right, of course.  I did that initially, but when I merged down
some changes I ended up having to rework the CL and this got lost.  Thanks! 
Done.

http://codereview.chromium.org/3064033/diff/6001/7001#newcode1003
net/http/http_network_transaction.cc:1003: spdy_session_.release();
On 2010/08/03 21:41:06, vandebo wrote:
> This will probably give you a compile warning -> error (ignoring the result of
> release), just move it into the previous line.

Done.  Good point!

http://codereview.chromium.org/3064033/diff/6001/7001#newcode1096
net/http/http_network_transaction.cc:1096: if (request_headers_.empty()) {
On 2010/08/03 21:41:06, vandebo wrote:
> && !using_spdy_ will save a bit of work - since spdy ignores the passed in
> headers, right?

Done.

http://codereview.chromium.org/3064033/diff/6001/7001#newcode1129
net/http/http_network_transaction.cc:1129: return
stream_->SendRequest(request_headers_, request_body,
On 2010/08/03 21:41:06, vandebo wrote:
> room for &response_ ?

Done.

http://codereview.chromium.org/3064033/diff/6001/7001#newcode1156
net/http/http_network_transaction.cc:1156: if (using_spdy_) {
On 2010/08/03 21:41:06, vandebo wrote:
> This is fine, but you might be able to integrate it better.  Mike might want
> some of the error handling that is currently only done for Http results...

Yeah, mike commented on this one as well.  I'm on the same page, but I'm tempted
to leave it for now, and rework in a subsequent step?

http://codereview.chromium.org/3064033/diff/6001/7001#newcode1273
net/http/http_network_transaction.cc:1273: if (!using_spdy_)
On 2010/08/03 21:41:06, vandebo wrote:
> Maybe HttpStream should also have an is_initialized() method?

Sounds reasonable.  Should it return true iff InitializeStream() was successful,
or do we want something at the connection level?  (Should we do this in a second
CL?)

http://codereview.chromium.org/3064033/diff/6001/7001#newcode1299
net/http/http_network_transaction.cc:1299: if (!keep_alive)
On 2010/08/03 21:41:06, vandebo wrote:
> can put the spdy code down here.

Done.

http://codereview.chromium.org/3064033/diff/6001/7001#newcode1638
net/http/http_network_transaction.cc:1638: ShouldResendFailedRequest(error);
On 2010/08/03 21:41:06, vandebo wrote:
> It would be nice to unify the ShouldResetFailedRequest calls.  But maybe it's
> best to wait for the next phase of refactoring to do that (presenting a spdy
> session as a ClientSocketHandle).

I think a second CL is appropriate here.  We might also want to make HttpStream
a bit more knowledgeable about the connection it sits on top of.  Currently, it
hands the connection off the the HttpStreamParser, and then forgets about it. 
Perhaps it would make sense to keep a pointer to the connection around, and be
able to delegate to it.  That way, HttpStream could have a
ShouldResendFailedRequest(int) method.  Does that sound workable?  It would also
help with the Cancel() change you suggested.  (And IsInitialized, for that
matter).

http://codereview.chromium.org/3064033/diff/6001/7002
File net/http/http_network_transaction.h (right):

http://codereview.chromium.org/3064033/diff/6001/7002#newcode39
net/http/http_network_transaction.h:39: class SpdyHttpStream;
On 2010/08/03 21:41:06, vandebo wrote:
> SpdyHttpStream is no longer needed

Done.

[vandebo (ex-Chrome), 2010-08-04 00:45:42]

Try result?  LGTM otherwise.

http://codereview.chromium.org/3064033/diff/6001/7001
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/3064033/diff/6001/7001#newcode506
net/http/http_network_transaction.cc:506:
static_cast<SpdyHttpStream*>(stream_.get())->Cancel();
On 2010/08/03 22:17:12, Ryan Hamilton wrote:
> On 2010/08/03 21:41:06, vandebo wrote:
> > Maybe Cancel should be added to the HttpStream interface?  For standard
Http,
> > you could have it disconnect the socket.
> 
> Agreed.  HttpNetworkTransaction is currently a bit schizophrenic about the
> relationship between a HTTP stream and the connection.  Some times the code
trys
> to work with the stream, and other times with the connection.  Yuck!  I'm
> hopeful that Mike's StreamFactory will help by hiding the connection from
> HttpNetworkTransaction :>
> 
> I like the idea of adding Connect to HttpStream.  Are you thinking that
> HttpBasicStream::Cancel() would basically implement the earlier block? 
Namely:

Yea, I think the thing to keep in mind is that the stream factory is the way of
the future, at which point there won't be much use for the connection.  So
hiding all current accesses to connection in appropriate interfaces to
HttpStream is the preferred change.

If this is the only need for a Close type call, then maybe implicitly in the
HttpStream destructor is appropriate, I haven't looked.

http://codereview.chromium.org/3064033/diff/6001/7001#newcode1156
net/http/http_network_transaction.cc:1156: if (using_spdy_) {
On 2010/08/03 22:17:12, Ryan Hamilton wrote:
> On 2010/08/03 21:41:06, vandebo wrote:
> > This is fine, but you might be able to integrate it better.  Mike might want
> > some of the error handling that is currently only done for Http results...
> 
> Yeah, mike commented on this one as well.  I'm on the same page, but I'm
tempted
> to leave it for now, and rework in a subsequent step?

I mentally did this when writing the suggestion, I think it's no too bad.  If
you spend more than 10 min trying to do it (integrate, but no change in
behavior), forget it.

http://codereview.chromium.org/3064033/diff/6001/7001#newcode1273
net/http/http_network_transaction.cc:1273: if (!using_spdy_)
On 2010/08/03 22:17:12, Ryan Hamilton wrote:
> On 2010/08/03 21:41:06, vandebo wrote:
> > Maybe HttpStream should also have an is_initialized() method?
> 
> Sounds reasonable.  Should it return true iff InitializeStream() was
successful,
> or do we want something at the connection level?  (Should we do this in a
second
> CL?)

I think this should be implementation specific.  For the HttpBasicStream case,
it probably needs to check the connection as well as verify that InitStream has
been called.  Follow up CL makes sense here.

http://codereview.chromium.org/3064033/diff/6001/7001#newcode1299
net/http/http_network_transaction.cc:1299: if (!keep_alive)
On 2010/08/03 22:17:12, Ryan Hamilton wrote:
> On 2010/08/03 21:41:06, vandebo wrote:
> > can put the spdy code down here.
> Done.

I was actually suggesting in here... calculating done and keep_alive for spdy
won't hurt anything.

[Ryan Hamilton, 2010-08-04 15:47:23]

On 2010/08/04 00:45:42, vandebo wrote:
> Try result?  LGTM otherwise.
> 
> http://codereview.chromium.org/3064033/diff/6001/7001
> File net/http/http_network_transaction.cc (right):
> 
> http://codereview.chromium.org/3064033/diff/6001/7001#newcode506
> net/http/http_network_transaction.cc:506:
> static_cast<SpdyHttpStream*>(stream_.get())->Cancel();
> On 2010/08/03 22:17:12, Ryan Hamilton wrote:
> > On 2010/08/03 21:41:06, vandebo wrote:
> > > Maybe Cancel should be added to the HttpStream interface?  For standard
> Http,
> > > you could have it disconnect the socket.
> > 
> > Agreed.  HttpNetworkTransaction is currently a bit schizophrenic about the
> > relationship between a HTTP stream and the connection.  Some times the code
> trys
> > to work with the stream, and other times with the connection.  Yuck!  I'm
> > hopeful that Mike's StreamFactory will help by hiding the connection from
> > HttpNetworkTransaction :>
> > 
> > I like the idea of adding Connect to HttpStream.  Are you thinking that
> > HttpBasicStream::Cancel() would basically implement the earlier block? 
> Namely:
> 
> Yea, I think the thing to keep in mind is that the stream factory is the way
of
> the future, at which point there won't be much use for the connection.  So
> hiding all current accesses to connection in appropriate interfaces to
> HttpStream is the preferred change.

Sounds great.  I'll start a second CL to do just that!

> If this is the only need for a Close type call, then maybe implicitly in the
> HttpStream destructor is appropriate, I haven't looked.
> 
> http://codereview.chromium.org/3064033/diff/6001/7001#newcode1156
> net/http/http_network_transaction.cc:1156: if (using_spdy_) {
> On 2010/08/03 22:17:12, Ryan Hamilton wrote:
> > On 2010/08/03 21:41:06, vandebo wrote:
> > > This is fine, but you might be able to integrate it better.  Mike might
want
> > > some of the error handling that is currently only done for Http results...
> > 
> > Yeah, mike commented on this one as well.  I'm on the same page, but I'm
> tempted
> > to leave it for now, and rework in a subsequent step?
> 
> I mentally did this when writing the suggestion, I think it's no too bad.  If
> you spend more than 10 min trying to do it (integrate, but no change in
> behavior), forget it.

I must be screwing up something simple.  I get a crash from
HttpNetworkTransactionTest.UseAlternateProtocolForNpnSpdy, when
HttpNetworkTransaction attempts to get the SSL socket from the connection.  (I
believe this is because in the SPDY case, connection_ is not set).  As part of
the "hide connection_ in stream_"  CL, I'm sure I'll be able to get this
working.

> http://codereview.chromium.org/3064033/diff/6001/7001#newcode1273
> net/http/http_network_transaction.cc:1273: if (!using_spdy_)
> On 2010/08/03 22:17:12, Ryan Hamilton wrote:
> > On 2010/08/03 21:41:06, vandebo wrote:
> > > Maybe HttpStream should also have an is_initialized() method?
> > 
> > Sounds reasonable.  Should it return true iff InitializeStream() was
> successful,
> > or do we want something at the connection level?  (Should we do this in a
> second
> > CL?)
> 
> I think this should be implementation specific.  For the HttpBasicStream case,
> it probably needs to check the connection as well as verify that InitStream
has
> been called.  Follow up CL makes sense here.

Sounds good.

> http://codereview.chromium.org/3064033/diff/6001/7001#newcode1299
> net/http/http_network_transaction.cc:1299: if (!keep_alive)
> On 2010/08/03 22:17:12, Ryan Hamilton wrote:
> > On 2010/08/03 21:41:06, vandebo wrote:
> > > can put the spdy code down here.
> > Done.
> 
> I was actually suggesting in here... calculating done and keep_alive for spdy
> won't hurt anything.

Oh, hah!  I wondered why you wanted me to reverse the order of statements in an
if() block :>  You know, I think the SPDY case (of resetting stream_) might not
even be needed.  I'll see if I can remove it altogether...