bug 819044 backout changes to spdy goaway handling r=backout
authorPatrick McManus <mcmanus@ducksong.com>
Wed, 13 Feb 2013 10:50:42 -0500
changeset 131632 5e63610b9072fcc9a52f9a5fd8f866e5b28a49f8
parent 131631 2da3edf254fb39ad77385834a3c1b11333c7d2b2
child 131633 dcc017a7cd4d7f9659f6b16c68b6a5e36e61238e
push id2323
push userbbajaj@mozilla.com
push dateMon, 01 Apr 2013 19:47:02 +0000
treeherdermozilla-beta@7712be144d91 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbackout
bugs819044
milestone21.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
bug 819044 backout changes to spdy goaway handling r=backout
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
@@ -88,45 +88,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);
   
@@ -1340,48 +1321,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
@@ -89,45 +89,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);
   
@@ -316,16 +297,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));
@@ -1268,49 +1250,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
@@ -215,20 +215,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;
@@ -352,16 +348,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)