bug 819044 - better spdy stream cleanup when handling goaway r=honzab
☠☠ backed out by d7a482019f54 ☠ ☠
authorPatrick McManus <mcmanus@ducksong.com>
Tue, 18 Dec 2012 22:28:24 -0500
changeset 125602 d0797dfcab56c294a7a13693732874de54627964
parent 125601 a2a26dec9fc4f34a74409be7d8449d5302a851af
child 125603 70f1b2db9f5f87c945be774bdb844a2671979a5b
push id2151
push userlsblakk@mozilla.com
push dateTue, 19 Feb 2013 18:06:57 +0000
treeherdermozilla-beta@4952e88741ec [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershonzab
bugs819044
milestone20.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 - better spdy stream cleanup when handling goaway r=honzab
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,26 +87,45 @@ 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 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))
+  // 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()))
     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);
   
@@ -1320,19 +1339,48 @@ 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;
-  
-  LOG3(("SpdySession2::HandleGoAway %p GOAWAY Last-Good-ID 0x%X.",
-        self, self->mGoAwayID));
+
+  // 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()));
   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,16 +197,20 @@ 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;
 
@@ -330,13 +334,16 @@ 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,26 +88,45 @@ 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 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))
+  // 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()))
     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);
   
@@ -296,17 +315,16 @@ 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));
@@ -1198,20 +1216,49 @@ 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;
-  
-  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])));
+
+  // 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()));
 
   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,16 +208,20 @@ 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;
@@ -341,13 +345,16 @@ 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,16 +36,18 @@ 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; }
@@ -116,17 +118,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
+  // Flag is set when all http request headers have been read and ID is stable
   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,16 +35,18 @@ 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; }
@@ -138,17 +140,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
+  // Flag is set when all http request headers have been read and ID is stable
   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)