bug 819044 backout changes to spdy goaway handling r=backout a=akeybl
authorPatrick McManus <mcmanus@ducksong.com>
Tue, 12 Feb 2013 17:20:37 -0500
changeset 123969 332c4ccdfe5bd384f1155527880e42ad68297f86
parent 123968 3975c01ee4263fcc318a8571e85660e88b5d73d7
child 123970 6ee359dea6ead5d2b47e0b9b68c6b8d4b3ff5a5e
push id3357
push usermcmanus@ducksong.com
push dateTue, 12 Feb 2013 22:20:58 +0000
treeherdermozilla-aurora@332c4ccdfe5b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbackout, akeybl
bugs819044
milestone20.0a2
bug 819044 backout changes to spdy goaway handling r=backout a=akeybl
netwerk/protocol/http/SpdySession2.cpp
netwerk/protocol/http/SpdySession2.h
netwerk/protocol/http/SpdySession3.cpp
netwerk/protocol/http/SpdySession3.h
netwerk/protocol/http/SpdyStream2.h
netwerk/protocol/http/SpdyStream3.h
--- a/netwerk/protocol/http/SpdySession2.cpp
+++ b/netwerk/protocol/http/SpdySession2.cpp
@@ -87,45 +87,26 @@ PLDHashOperator
 SpdySession2::ShutdownEnumerator(nsAHttpTransaction *key,
                                 nsAutoPtr<SpdyStream2> &stream,
                                 void *closure)
 {
   SpdySession2 *self = static_cast<SpdySession2 *>(closure);
  
   // On a clean server hangup the server sets the GoAwayID to be the ID of
   // the last transaction it processed. If the ID of stream in the
-  // local stream is greater than that it can safely be restarted because the
-  // server guarantees it was not partially processed. Streams that have not
-  // registered an ID haven't actually been sent yet so they can always be
-  // restarted.
-  if (self->mCleanShutdown &&
-      (stream->StreamID() > self->mGoAwayID || !stream->HasRegisteredID()))
+  // local session is greater than that it can safely be restarted because the
+  // server guarantees it was not partially processed.
+  if (self->mCleanShutdown && (stream->StreamID() > self->mGoAwayID))
     self->CloseStream(stream, NS_ERROR_NET_RESET); // can be restarted
   else
     self->CloseStream(stream, NS_ERROR_ABORT);
 
   return PL_DHASH_NEXT;
 }
 
-PLDHashOperator
-SpdySession2::GoAwayEnumerator(nsAHttpTransaction *key,
-                               nsAutoPtr<SpdyStream2> &stream,
-                               void *closure)
-{
-  SpdySession2 *self = static_cast<SpdySession2 *>(closure);
-
-  // these streams were not processed by the server and can be restarted.
-  // Do that after the enumerator completes to avoid the risk of
-  // a restart event re-entrantly modifying this hash.
-  if (stream->StreamID() > self->mGoAwayID || !stream->HasRegisteredID())
-    self->mGoAwayStreamsToRestart.Push(stream);
-
-  return PL_DHASH_NEXT;
-}
-
 SpdySession2::~SpdySession2()
 {
   LOG3(("SpdySession2::~SpdySession2 %p mDownstreamState=%X",
         this, mDownstreamState));
 
   inflateEnd(&mDownstreamZlib);
   deflateEnd(&mUpstreamZlib);
   
@@ -1339,48 +1320,19 @@ SpdySession2::HandleGoAway(SpdySession2 
           self, self->mInputFrameDataSize));
     return NS_ERROR_ILLEGAL_VALUE;
   }
 
   self->mShouldGoAway = true;
   self->mGoAwayID =
     PR_ntohl(reinterpret_cast<uint32_t *>(self->mInputFrameBuffer.get())[2]);
   self->mCleanShutdown = true;
-
-  // Find streams greater than the last-good ID and mark them for deletion 
-  // in the mGoAwayStreamsToRestart queue with the GoAwayEnumerator. They can
-  // be restarted.
-  self->mStreamTransactionHash.Enumerate(GoAwayEnumerator, self);
-
-  // Process the streams marked for deletion and restart.
-  uint32_t size = self->mGoAwayStreamsToRestart.GetSize();
-  for (uint32_t count = 0; count < size; ++count) {
-    SpdyStream2 *stream =
-      static_cast<SpdyStream2 *>(self->mGoAwayStreamsToRestart.PopFront());
-
-    self->CloseStream(stream, NS_ERROR_NET_RESET);
-    if (stream->HasRegisteredID())
-      self->mStreamIDHash.Remove(stream->StreamID());
-    self->mStreamTransactionHash.Remove(stream->Transaction());
-  }
-
-  // Queued streams can also be deleted from this session and restarted
-  // in another one. (they were never sent on the network so they implicitly
-  // are not covered by the last-good id.
-  size = self->mQueuedStreams.GetSize();
-  for (uint32_t count = 0; count < size; ++count) {
-    SpdyStream2 *stream =
-      static_cast<SpdyStream2 *>(self->mQueuedStreams.PopFront());
-    self->CloseStream(stream, NS_ERROR_NET_RESET);
-    self->mStreamTransactionHash.Remove(stream->Transaction());
-  }
-
-  LOG3(("SpdySession2::HandleGoAway %p GOAWAY Last-Good-ID 0x%X."
-        "live streams=%d\n", self, self->mGoAwayID,
-        self->mStreamTransactionHash.Count()));
+  
+  LOG3(("SpdySession2::HandleGoAway %p GOAWAY Last-Good-ID 0x%X.",
+        self, self->mGoAwayID));
   self->ResumeRecv();
   self->ResetDownstreamState();
   return NS_OK;
 }
 
 nsresult
 SpdySession2::HandleHeaders(SpdySession2 *self)
 {
--- a/netwerk/protocol/http/SpdySession2.h
+++ b/netwerk/protocol/http/SpdySession2.h
@@ -197,20 +197,16 @@ private:
   // a wrapper for all calls to the nshttpconnection level segment writer. Used
   // to track network I/O for timeout purposes
   nsresult   NetworkRead(nsAHttpSegmentWriter *, char *, uint32_t, uint32_t *);
   
   static PLDHashOperator ShutdownEnumerator(nsAHttpTransaction *,
                                             nsAutoPtr<SpdyStream2> &,
                                             void *);
 
-  static PLDHashOperator GoAwayEnumerator(nsAHttpTransaction *,
-                                          nsAutoPtr<SpdyStream2> &,
-                                          void *);
-
   // This is intended to be nsHttpConnectionMgr:nsHttpConnectionHandle taken
   // from the first transaction on this session. That object contains the
   // pointer to the real network-level nsHttpConnection object.
   nsRefPtr<nsAHttpConnection> mConnection;
 
   // The underlying socket transport object is needed to propogate some events
   nsISocketTransport         *mSocketTransport;
 
@@ -334,16 +330,13 @@ private:
   uint32_t             mOutputQueueSent;
   nsAutoArrayPtr<char> mOutputQueueBuffer;
 
   PRIntervalTime       mPingThreshold;
   PRIntervalTime       mLastReadEpoch;     // used for ping timeouts
   PRIntervalTime       mLastDataReadEpoch; // used for IdleTime()
   PRIntervalTime       mPingSentEpoch;
   uint32_t             mNextPingID;
-
-  // used as a temporary buffer while enumerating the stream hash during GoAway
-  nsDeque  mGoAwayStreamsToRestart;
 };
 
 }} // namespace mozilla::net
 
 #endif // mozilla_net_SpdySession2_h
--- a/netwerk/protocol/http/SpdySession3.cpp
+++ b/netwerk/protocol/http/SpdySession3.cpp
@@ -88,45 +88,26 @@ PLDHashOperator
 SpdySession3::ShutdownEnumerator(nsAHttpTransaction *key,
                                 nsAutoPtr<SpdyStream3> &stream,
                                 void *closure)
 {
   SpdySession3 *self = static_cast<SpdySession3 *>(closure);
  
   // On a clean server hangup the server sets the GoAwayID to be the ID of
   // the last transaction it processed. If the ID of stream in the
-  // local stream is greater than that it can safely be restarted because the
-  // server guarantees it was not partially processed. Streams that have not
-  // registered an ID haven't actually been sent yet so they can always be
-  // restarted.
-  if (self->mCleanShutdown &&
-      (stream->StreamID() > self->mGoAwayID || !stream->HasRegisteredID()))
+  // local session is greater than that it can safely be restarted because the
+  // server guarantees it was not partially processed.
+  if (self->mCleanShutdown && (stream->StreamID() > self->mGoAwayID))
     self->CloseStream(stream, NS_ERROR_NET_RESET); // can be restarted
   else
     self->CloseStream(stream, NS_ERROR_ABORT);
 
   return PL_DHASH_NEXT;
 }
 
-PLDHashOperator
-SpdySession3::GoAwayEnumerator(nsAHttpTransaction *key,
-                               nsAutoPtr<SpdyStream3> &stream,
-                               void *closure)
-{
-  SpdySession3 *self = static_cast<SpdySession3 *>(closure);
-
-  // these streams were not processed by the server and can be restarted.
-  // Do that after the enumerator completes to avoid the risk of
-  // a restart event re-entrantly modifying this hash.
-  if (stream->StreamID() > self->mGoAwayID || !stream->HasRegisteredID())
-    self->mGoAwayStreamsToRestart.Push(stream);
-
-  return PL_DHASH_NEXT;
-}
-
 SpdySession3::~SpdySession3()
 {
   LOG3(("SpdySession3::~SpdySession3 %p mDownstreamState=%X",
         this, mDownstreamState));
 
   inflateEnd(&mDownstreamZlib);
   deflateEnd(&mUpstreamZlib);
   
@@ -315,16 +296,17 @@ SpdySession3::AddStream(nsAHttpTransacti
   aHttpTransaction->SetConnection(this);
   SpdyStream3 *stream = new SpdyStream3(aHttpTransaction,
                                       this,
                                       mSocketTransport,
                                       mSendingChunkSize,
                                       &mUpstreamZlib,
                                       aPriority);
 
+  
   LOG3(("SpdySession3::AddStream session=%p stream=%p NextID=0x%X (tentative)",
         this, stream, mNextStreamID));
 
   mStreamTransactionHash.Put(aHttpTransaction, stream);
 
   if (RoomForMoreConcurrent()) {
     LOG3(("SpdySession3::AddStream %p stream %p activated immediately.",
           this, stream));
@@ -1216,49 +1198,20 @@ SpdySession3::HandleGoAway(SpdySession3 
           self, self->mInputFrameDataSize));
     return NS_ERROR_ILLEGAL_VALUE;
   }
 
   self->mShouldGoAway = true;
   self->mGoAwayID =
     PR_ntohl(reinterpret_cast<uint32_t *>(self->mInputFrameBuffer.get())[2]);
   self->mCleanShutdown = true;
-
-  // Find streams greater than the last-good ID and mark them for deletion 
-  // in the mGoAwayStreamsToRestart queue with the GoAwayEnumerator. They can
-  // be restarted.
-  self->mStreamTransactionHash.Enumerate(GoAwayEnumerator, self);
-
-  // Process the streams marked for deletion and restart.
-  uint32_t size = self->mGoAwayStreamsToRestart.GetSize();
-  for (uint32_t count = 0; count < size; ++count) {
-    SpdyStream3 *stream =
-      static_cast<SpdyStream3 *>(self->mGoAwayStreamsToRestart.PopFront());
-
-    self->CloseStream(stream, NS_ERROR_NET_RESET);
-    if (stream->HasRegisteredID())
-      self->mStreamIDHash.Remove(stream->StreamID());
-    self->mStreamTransactionHash.Remove(stream->Transaction());
-  }
-
-  // Queued streams can also be deleted from this session and restarted
-  // in another one. (they were never sent on the network so they implicitly
-  // are not covered by the last-good id.
-  size = self->mQueuedStreams.GetSize();
-  for (uint32_t count = 0; count < size; ++count) {
-    SpdyStream3 *stream =
-      static_cast<SpdyStream3 *>(self->mQueuedStreams.PopFront());
-    self->CloseStream(stream, NS_ERROR_NET_RESET);
-    self->mStreamTransactionHash.Remove(stream->Transaction());
-  }
-
-  LOG3(("SpdySession3::HandleGoAway %p GOAWAY Last-Good-ID 0x%X status 0x%X "
-        "live streams=%d\n", self, self->mGoAwayID, 
-        PR_ntohl(reinterpret_cast<uint32_t *>(self->mInputFrameBuffer.get())[3]),
-        self->mStreamTransactionHash.Count()));
+  
+  LOG3(("SpdySession3::HandleGoAway %p GOAWAY Last-Good-ID 0x%X status 0x%X\n",
+        self, self->mGoAwayID, 
+        PR_ntohl(reinterpret_cast<uint32_t *>(self->mInputFrameBuffer.get())[3])));
 
   self->ResumeRecv();
   self->ResetDownstreamState();
   return NS_OK;
 }
 
 nsresult
 SpdySession3::HandleHeaders(SpdySession3 *self)
--- a/netwerk/protocol/http/SpdySession3.h
+++ b/netwerk/protocol/http/SpdySession3.h
@@ -208,20 +208,16 @@ private:
   // a wrapper for all calls to the nshttpconnection level segment writer. Used
   // to track network I/O for timeout purposes
   nsresult   NetworkRead(nsAHttpSegmentWriter *, char *, uint32_t, uint32_t *);
   
   static PLDHashOperator ShutdownEnumerator(nsAHttpTransaction *,
                                             nsAutoPtr<SpdyStream3> &,
                                             void *);
 
-  static PLDHashOperator GoAwayEnumerator(nsAHttpTransaction *,
-                                          nsAutoPtr<SpdyStream3> &,
-                                          void *);
-
   static PLDHashOperator UpdateServerRwinEnumerator(nsAHttpTransaction *,
                                                     nsAutoPtr<SpdyStream3> &,
                                                     void *);
 
   // This is intended to be nsHttpConnectionMgr:nsHttpConnectionHandle taken
   // from the first transaction on this session. That object contains the
   // pointer to the real network-level nsHttpConnection object.
   nsRefPtr<nsAHttpConnection> mConnection;
@@ -345,16 +341,13 @@ private:
   uint32_t             mOutputQueueSent;
   nsAutoArrayPtr<char> mOutputQueueBuffer;
 
   PRIntervalTime       mPingThreshold;
   PRIntervalTime       mLastReadEpoch;     // used for ping timeouts
   PRIntervalTime       mLastDataReadEpoch; // used for IdleTime()
   PRIntervalTime       mPingSentEpoch;
   uint32_t             mNextPingID;
-
-  // used as a temporary buffer while enumerating the stream hash during GoAway
-  nsDeque  mGoAwayStreamsToRestart;
 };
 
 }} // namespace mozilla::net
 
 #endif // mozilla_net_SpdySession3_h
--- a/netwerk/protocol/http/SpdyStream2.h
+++ b/netwerk/protocol/http/SpdyStream2.h
@@ -36,18 +36,16 @@ public:
   // returns false if called more than once
   bool GetFullyOpen() {return mFullyOpen;}
   void SetFullyOpen() 
   {
     NS_ABORT_IF_FALSE(!mFullyOpen, "SetFullyOpen already open");
     mFullyOpen = 1;
   }
 
-  bool HasRegisteredID() { return mStreamID != 0; }
-
   nsAHttpTransaction *Transaction()
   {
     return mTransaction;
   }
 
   void Close(nsresult reason);
 
   void SetRecvdFin(bool aStatus) { mRecvdFin = aStatus ? 1 : 0; }
@@ -118,17 +116,17 @@ private:
   nsAHttpSegmentWriter        *mSegmentWriter;
 
   // The 24 bit SPDY stream ID
   uint32_t                    mStreamID;
 
   // The quanta upstream data frames are chopped into
   uint32_t                    mChunkSize;
 
-  // Flag is set when all http request headers have been read and ID is stable
+  // Flag is set when all http request headers have been read
   uint32_t                     mSynFrameComplete     : 1;
 
   // Flag is set when the HTTP processor has more data to send
   // but has blocked in doing so.
   uint32_t                     mRequestBlockedOnRead : 1;
 
   // Flag is set when a FIN has been placed on a data or syn packet
   // (i.e after the client has closed)
--- a/netwerk/protocol/http/SpdyStream3.h
+++ b/netwerk/protocol/http/SpdyStream3.h
@@ -35,18 +35,16 @@ public:
   // returns false if called more than once
   bool GetFullyOpen() {return mFullyOpen;}
   void SetFullyOpen() 
   {
     NS_ABORT_IF_FALSE(!mFullyOpen, "SetFullyOpen already open");
     mFullyOpen = 1;
   }
 
-  bool HasRegisteredID() { return mStreamID != 0; }
-
   nsAHttpTransaction *Transaction()
   {
     return mTransaction;
   }
 
   void Close(nsresult reason);
 
   void SetRecvdFin(bool aStatus) { mRecvdFin = aStatus ? 1 : 0; }
@@ -140,17 +138,17 @@ private:
   nsAHttpSegmentWriter        *mSegmentWriter;
 
   // The 24 bit SPDY stream ID
   uint32_t                    mStreamID;
 
   // The quanta upstream data frames are chopped into
   uint32_t                    mChunkSize;
 
-  // Flag is set when all http request headers have been read and ID is stable
+  // Flag is set when all http request headers have been read
   uint32_t                     mSynFrameComplete     : 1;
 
   // Flag is set when the HTTP processor has more data to send
   // but has blocked in doing so.
   uint32_t                     mRequestBlockedOnRead : 1;
 
   // Flag is set when a FIN has been placed on a data or syn packet
   // (i.e after the client has closed)