bug 819044 - better spdy stream cleanup when handling goaway r=honzab
authorPatrick McManus <mcmanus@ducksong.com>
Thu, 20 Dec 2012 13:27:15 -0500
changeset 125023 f06b4ca5c9a4259f2a16c80b215f18b99aee592c
parent 125022 8cda8247066bb28349a90b811f21c4e45cb88fdf
child 125024 db5c280d190765122a3802d44074a898cc215d40
push id24747
push usermcmanus@ducksong.com
push dateSat, 16 Mar 2013 23:20:41 +0000
treeherdermozilla-inbound@f06b4ca5c9a4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershonzab
bugs819044
milestone22.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
@@ -88,26 +88,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);
   
@@ -1319,19 +1338,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
@@ -89,26 +89,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);
   
@@ -295,17 +314,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));
@@ -1248,20 +1266,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
@@ -215,16 +215,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;
@@ -348,13 +352,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)