SpdySession go-away on network change

In addition to making sessions unavailable, also
abort inactive streams and close idle sessions.

BUG=324653

Review URL: https://codereview.chromium.org/321513002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@275570 0039d316-1c4b-4281-b951-d872f2087c98

net/spdy/spdy_session.cc

@@ -51,10 +51,6 @@ const int kReadBufferSize = 8 * 1024;
 const int kDefaultConnectionAtRiskOfLossSeconds = 10;
 const int kHungIntervalSeconds = 10;
 
-// As we always act as the client, start at 1 for the first stream id.
-const SpdyStreamId kFirstStreamId = 1;
-const SpdyStreamId kLastStreamId = 0x7fffffff;
-
 // Minimum seconds that unclaimed pushed streams will be kept in memory.
 const int kMinPushedStreamLifetimeSeconds = 300;
 

net/spdy/spdy_session.h

@@ -61,6 +61,11 @@ const int kMaxReadBytesWithoutYielding = 32 * 1024;
 // The initial receive window size for both streams and sessions.
 const int32 kDefaultInitialRecvWindowSize = 10 * 1024 * 1024;  // 10MB
 
+// First and last valid stream IDs. As we always act as the client,
+// start at 1 for the first stream id.
+const SpdyStreamId kFirstStreamId = 1;
+const SpdyStreamId kLastStreamId = 0x7fffffff;
+
 class BoundNetLog;
 struct LoadTimingInfo;
 class SpdyStream;
@@ -365,6 +370,18 @@ class NET_EXPORT SpdySession : public BufferedSpdyFramerVisitorInterface,
   // will not close any streams.
   void MakeUnavailable();
 
+  // Closes all active streams with stream id's greater than
+  // |last_good_stream_id|, as well as any created or pending
+  // streams. Must be called only when |availability_state_| >=
+  // STATE_GOING_AWAY. After this function, DcheckGoingAway() will
+  // pass. May be called multiple times.
+  void StartGoingAway(SpdyStreamId last_good_stream_id, Error status);
+
+  // Must be called only when going away (i.e., DcheckGoingAway()
+  // passes). If there are no more active streams and the session
+  // isn't closed yet, close it.
+  void MaybeFinishGoingAway();
+
   // Retrieves information on the current state of the SPDY session as a
   // Value.  Caller takes possession of the returned value.
   base::Value* GetInfoAsValue() const;
@@ -738,18 +755,6 @@ class NET_EXPORT SpdySession : public BufferedSpdyFramerVisitorInterface,
   // are no active streams or unclaimed pushed streams.
   void DcheckDraining() const;
 
-  // Closes all active streams with stream id's greater than
-  // |last_good_stream_id|, as well as any created or pending
-  // streams. Must be called only when |availability_state_| >=
-  // STATE_GOING_AWAY. After this function, DcheckGoingAway() will
-  // pass. May be called multiple times.
-  void StartGoingAway(SpdyStreamId last_good_stream_id, Error status);
-
-  // Must be called only when going away (i.e., DcheckGoingAway()
-  // passes). If there are no more active streams and the session
-  // isn't closed yet, close it.
-  void MaybeFinishGoingAway();
-
   // If the session is already draining, does nothing. Otherwise, moves
   // the session to the draining state.
   void DoDrainSession(Error err, const std::string& description);

net/spdy/spdy_session_pool.cc

@@ -278,11 +278,14 @@ void SpdySessionPool::OnIPAddressChanged() {
     if (!*it)
       continue;
 
-    // For OSs that terminate TCP connections upon relevant network changes
-    // there is no need to explicitly close SpdySessions, instead simply mark
-    // the sessions as deprecated so they aren't reused.
+// For OSs that terminate TCP connections upon relevant network changes,
+// attempt to preserve active streams by marking all sessions as going
+// away, rather than explicitly closing them. Streams may still fail due
+// to a generated TCP reset.
 #if defined(OS_ANDROID) || defined(OS_WIN) || defined(OS_IOS)
     (*it)->MakeUnavailable();
+    (*it)->StartGoingAway(kLastStreamId, OK);
+    (*it)->MaybeFinishGoingAway();
 #else
     (*it)->CloseSessionOnError(ERR_NETWORK_CHANGED,
                                "Closing current sessions.");

net/spdy/spdy_session_pool_unittest.cc

@@ -510,12 +510,17 @@ TEST_P(SpdySessionPoolTest, IPPoolingCloseIdleSessions) {
 // for crbug.com/379469.
 TEST_P(SpdySessionPoolTest, IPAddressChanged) {
   MockConnect connect_data(SYNCHRONOUS, OK);
+  session_deps_.host_resolver->set_synchronous_mode(true);
+  SpdyTestUtil spdy_util(GetParam());
+
   MockRead reads[] = {
       MockRead(SYNCHRONOUS, ERR_IO_PENDING)  // Stall forever.
   };
-  session_deps_.host_resolver->set_synchronous_mode(true);
+  scoped_ptr<SpdyFrame> req(
+      spdy_util.ConstructSpdyGet("http://www.a.com", false, 1, MEDIUM));
+  MockWrite writes[] = {CreateMockWrite(*req, 1)};
 
-  StaticSocketDataProvider data(reads, arraysize(reads), NULL, 0);
+  DelayedSocketData data(1, reads, arraysize(reads), writes, arraysize(writes));
   data.set_connect_data(connect_data);
   session_deps_.socket_factory->AddSocketDataProvider(&data);
 
@@ -524,71 +529,75 @@ TEST_P(SpdySessionPoolTest, IPAddressChanged) {
 
   CreateNetworkSession();
 
-  // Set up session 1: Available, but idle.
-  const std::string kTestHost1("http://www.a.com");
-  HostPortPair test_host_port_pair1(kTestHost1, 80);
-  SpdySessionKey key1(
-      test_host_port_pair1, ProxyServer::Direct(), PRIVACY_MODE_DISABLED);
-  base::WeakPtr<SpdySession> session1 =
-      CreateInsecureSpdySession(http_session_, key1, BoundNetLog());
-  EXPECT_TRUE(session1->IsAvailable());
-
-  // Set up session 2: Going away, but with an active stream.
+  // Set up session A: Going away, but with an active stream.
   session_deps_.socket_factory->AddSocketDataProvider(&data);
-  const std::string kTestHost2("http://www.b.com");
-  HostPortPair test_host_port_pair2(kTestHost2, 80);
-  SpdySessionKey key2(
-      test_host_port_pair2, ProxyServer::Direct(), PRIVACY_MODE_DISABLED);
-  base::WeakPtr<SpdySession> session2 =
-      CreateInsecureSpdySession(http_session_, key2, BoundNetLog());
-  GURL url2(kTestHost2);
-  base::WeakPtr<SpdyStream> spdy_stream2 = CreateStreamSynchronously(
-      SPDY_BIDIRECTIONAL_STREAM, session2, url2, MEDIUM, BoundNetLog());
-  test::StreamDelegateDoNothing delegate2(spdy_stream2);
-  spdy_stream2->SetDelegate(&delegate2);
+  const std::string kTestHostA("http://www.a.com");
+  HostPortPair test_host_port_pairA(kTestHostA, 80);
+  SpdySessionKey keyA(
+      test_host_port_pairA, ProxyServer::Direct(), PRIVACY_MODE_DISABLED);
+  base::WeakPtr<SpdySession> sessionA =
+      CreateInsecureSpdySession(http_session_, keyA, BoundNetLog());
+  GURL urlA(kTestHostA);
+  base::WeakPtr<SpdyStream> spdy_streamA = CreateStreamSynchronously(
+      SPDY_BIDIRECTIONAL_STREAM, sessionA, urlA, MEDIUM, BoundNetLog());
+  test::StreamDelegateDoNothing delegateA(spdy_streamA);
+  spdy_streamA->SetDelegate(&delegateA);
 
   scoped_ptr<SpdyHeaderBlock> headers(
-      SpdyTestUtil(GetParam()).ConstructGetHeaderBlock(url2.spec()));
-  spdy_stream2->SendRequestHeaders(headers.Pass(), NO_MORE_DATA_TO_SEND);
-  EXPECT_TRUE(spdy_stream2->HasUrlFromHeaders());
-
-  session2->MakeUnavailable();
-  EXPECT_TRUE(session2->IsGoingAway());
-
-  // Set up session 3: Draining.
+      spdy_util.ConstructGetHeaderBlock(urlA.spec()));
+  spdy_streamA->SendRequestHeaders(headers.Pass(), NO_MORE_DATA_TO_SEND);
+  EXPECT_TRUE(spdy_streamA->HasUrlFromHeaders());
+
+  base::MessageLoop::current()->RunUntilIdle();  // Allow headers to write.
+  EXPECT_TRUE(delegateA.send_headers_completed());
+
+  sessionA->MakeUnavailable();
+  EXPECT_TRUE(sessionA->IsGoingAway());
+  EXPECT_FALSE(delegateA.StreamIsClosed());
+
+  // Set up session B: Available, but idle.
+  const std::string kTestHostB("http://www.b.com");
+  HostPortPair test_host_port_pairB(kTestHostB, 80);
+  SpdySessionKey keyB(
+      test_host_port_pairB, ProxyServer::Direct(), PRIVACY_MODE_DISABLED);
+  base::WeakPtr<SpdySession> sessionB =
+      CreateInsecureSpdySession(http_session_, keyB, BoundNetLog());
+  EXPECT_TRUE(sessionB->IsAvailable());
+
+  // Set up session C: Draining.
   session_deps_.socket_factory->AddSocketDataProvider(&data);
-  const std::string kTestHost3("http://www.c.com");
-  HostPortPair test_host_port_pair3(kTestHost3, 80);
-  SpdySessionKey key3(
-      test_host_port_pair3, ProxyServer::Direct(), PRIVACY_MODE_DISABLED);
-  base::WeakPtr<SpdySession> session3 =
-      CreateInsecureSpdySession(http_session_, key3, BoundNetLog());
+  const std::string kTestHostC("http://www.c.com");
+  HostPortPair test_host_port_pairC(kTestHostC, 80);
+  SpdySessionKey keyC(
+      test_host_port_pairC, ProxyServer::Direct(), PRIVACY_MODE_DISABLED);
+  base::WeakPtr<SpdySession> sessionC =
+      CreateInsecureSpdySession(http_session_, keyC, BoundNetLog());
 
-  session3->CloseSessionOnError(ERR_SPDY_PROTOCOL_ERROR, "Error!");
-  EXPECT_TRUE(session3->IsDraining());
+  sessionC->CloseSessionOnError(ERR_SPDY_PROTOCOL_ERROR, "Error!");
+  EXPECT_TRUE(sessionC->IsDraining());
 
   spdy_session_pool_->OnIPAddressChanged();
 
 #if defined(OS_ANDROID) || defined(OS_WIN) || defined(OS_IOS)
-  // TODO(jgraettinger): This should be draining when crbug.com/324653 is fixed.
-  EXPECT_TRUE(session1->IsGoingAway());
-  EXPECT_TRUE(session2->IsGoingAway());
-  EXPECT_TRUE(session3->IsDraining());
+  EXPECT_TRUE(sessionA->IsGoingAway());
+  EXPECT_TRUE(sessionB->IsDraining());
+  EXPECT_TRUE(sessionC->IsDraining());
 
-  EXPECT_FALSE(delegate2.StreamIsClosed());
+  EXPECT_EQ(1u, sessionA->num_active_streams());  // Stream is still active.
+  EXPECT_FALSE(delegateA.StreamIsClosed());
 
-  session1->CloseSessionOnError(ERR_ABORTED, "Closing");
-  session2->CloseSessionOnError(ERR_ABORTED, "Closing");
+  sessionA->CloseSessionOnError(ERR_ABORTED, "Closing");
+  sessionB->CloseSessionOnError(ERR_ABORTED, "Closing");
 
-  EXPECT_TRUE(delegate2.StreamIsClosed());
-  EXPECT_EQ(ERR_ABORTED, delegate2.WaitForClose());
+  EXPECT_TRUE(delegateA.StreamIsClosed());
+  EXPECT_EQ(ERR_ABORTED, delegateA.WaitForClose());
 #else
-  EXPECT_TRUE(session1->IsDraining());
-  EXPECT_TRUE(session2->IsDraining());
-  EXPECT_TRUE(session3->IsDraining());
+  EXPECT_TRUE(sessionA->IsDraining());
+  EXPECT_TRUE(sessionB->IsDraining());
+  EXPECT_TRUE(sessionC->IsDraining());
 
-  EXPECT_TRUE(delegate2.StreamIsClosed());
-  EXPECT_EQ(ERR_NETWORK_CHANGED, delegate2.WaitForClose());
+  EXPECT_TRUE(delegateA.StreamIsClosed());
+  EXPECT_EQ(ERR_NETWORK_CHANGED, delegateA.WaitForClose());
 #endif  // defined(OS_ANDROID) || defined(OS_WIN) || defined(OS_IOS)
 }