SPDY flow control: fix for WINDOW_UPDATEs arriving while request is being sent.

BUG=48100
TEST=net_unittests

Review URL: http://codereview.chromium.org/3048058

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

8 files changed, 93 insertions, 76 deletions

diff --git a/net/http/http_network_transaction.h b/net/http/http_network_transaction.h
index 12f06e3..23eb44f 100644
--- a/net/http/http_network_transaction.h
+++ b/net/http/http_network_transaction.h
@@ -84,7 +84,7 @@ class HttpNetworkTransaction : public HttpTransaction {
 
  private:
   FRIEND_TEST_ALL_PREFIXES(HttpNetworkTransactionTest, ResetStateForRestart);
-  FRIEND_TEST_ALL_PREFIXES(SpdyNetworkTransactionTest, WindowUpdate);
+  FRIEND_TEST_ALL_PREFIXES(SpdyNetworkTransactionTest, WindowUpdateReceived);
   FRIEND_TEST_ALL_PREFIXES(SpdyNetworkTransactionTest, WindowUpdateOverflow);
   FRIEND_TEST_ALL_PREFIXES(SpdyNetworkTransactionTest, FlowControlStallResume);
 
diff --git a/net/spdy/spdy_http_stream.cc b/net/spdy/spdy_http_stream.cc
index 1b284bc..52c99f4 100644
--- a/net/spdy/spdy_http_stream.cc
+++ b/net/spdy/spdy_http_stream.cc
@@ -343,12 +343,9 @@ int SpdyHttpStream::OnSendBody() {
                                   spdy::DATA_FLAG_FIN);
 }
 
-void SpdyHttpStream::OnSendBodyComplete(int status) {
+bool SpdyHttpStream::OnSendBodyComplete(int status) {
   CHECK(request_body_stream_.get());
   request_body_stream_->DidConsume(status);
-}
-
-bool SpdyHttpStream::IsFinishedSendingBody() {
   return request_body_stream_->eof();
 }
 
diff --git a/net/spdy/spdy_http_stream.h b/net/spdy/spdy_http_stream.h
index 066ed8c..b333742 100644
--- a/net/spdy/spdy_http_stream.h
+++ b/net/spdy/spdy_http_stream.h
@@ -85,8 +85,7 @@ class SpdyHttpStream : public SpdyStream::Delegate, public HttpStream {
 
   virtual bool OnSendHeadersComplete(int status);
   virtual int OnSendBody();
-  virtual void OnSendBodyComplete(int status);
-  virtual bool IsFinishedSendingBody();
+  virtual bool OnSendBodyComplete(int status);
 
   // Called by the SpdySession when a response (e.g. a SYN_REPLY) has been
   // received for this stream.
@@ -117,6 +116,8 @@ class SpdyHttpStream : public SpdyStream::Delegate, public HttpStream {
   bool ShouldResendFailedRequest(int error) const;
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(SpdyNetworkTransactionTest, FlowControlStallResume);
+
   // Call the user callback.
   void DoCallback(int rv);
 
diff --git a/net/spdy/spdy_network_transaction_unittest.cc b/net/spdy/spdy_network_transaction_unittest.cc
index 8f0d5fc..ca4f56c 100644
--- a/net/spdy/spdy_network_transaction_unittest.cc
+++ b/net/spdy/spdy_network_transaction_unittest.cc
@@ -1371,33 +1371,75 @@ TEST_P(SpdyNetworkTransactionTest, ResponseWithTwoSynReplies) {
 
 // Test that sent data frames and received WINDOW_UPDATE frames change
 // the send_window_size_ correctly.
-TEST_P(SpdyNetworkTransactionTest, WindowUpdate) {
+
+// WINDOW_UPDATE is different than most other frames in that it can arrive
+// while the client is still sending the request body.  In order to enforce
+// this scenario, we feed a couple of dummy frames and give a delay of 0 to
+// socket data provider, so that initial read that is done as soon as the
+// stream is created, succeeds and schedules another read.  This way reads
+// and writes are interleaved; after doing a full frame write, SpdyStream
+// will break out of DoLoop and will read and process a WINDOW_UPDATE.
+// Once our WINDOW_UPDATE is read, we cannot send SYN_REPLY right away
+// since request has not been completely written, therefore we feed
+// enough number of WINDOW_UPDATEs to finish the first read and cause a
+// write, leading to a complete write of request body; after that we send
+// a reply with a body, to cause a graceful shutdown.
+
+// TODO(agayev): develop a socket data provider where both, reads and
+// writes are ordered so that writing tests like these are easy, right now
+// we are working around the limitations as described above and it's not
+// deterministic, tests may fail under specific circumstances.
+TEST_P(SpdyNetworkTransactionTest, WindowUpdateReceived) {
   SpdySession::SetFlowControl(true);
 
-  scoped_ptr<spdy::SpdyFrame> req(ConstructSpdyPost(kUploadDataSize, NULL, 0));
-  scoped_ptr<spdy::SpdyFrame> body(ConstructSpdyBodyFrame(1, true));
+  static int kFrameCount = 2;
+  scoped_ptr<std::string> content(
+      new std::string(kMaxSpdyFrameChunkSize, 'a'));
+  scoped_ptr<spdy::SpdyFrame> req(ConstructSpdyPost(
+      kMaxSpdyFrameChunkSize * kFrameCount, NULL, 0));
+  scoped_ptr<spdy::SpdyFrame> body(
+      ConstructSpdyBodyFrame(1, content->c_str(), content->size(), false));
+  scoped_ptr<spdy::SpdyFrame> body_end(
+      ConstructSpdyBodyFrame(1, content->c_str(), content->size(), true));
+
   MockWrite writes[] = {
     CreateMockWrite(*req),
     CreateMockWrite(*body),
+    CreateMockWrite(*body_end),
   };
 
   static const int kDeltaWindowSize = 0xff;
+  static const int kDeltaCount = 4;
   scoped_ptr<spdy::SpdyFrame> window_update(
       ConstructSpdyWindowUpdate(1, kDeltaWindowSize));
-  scoped_ptr<spdy::SpdyFrame> reply(ConstructSpdyPostSynReply(NULL, 0));
+  scoped_ptr<spdy::SpdyFrame> window_update_dummy(
+      ConstructSpdyWindowUpdate(2, kDeltaWindowSize));
+  scoped_ptr<spdy::SpdyFrame> resp(ConstructSpdyPostSynReply(NULL, 0));
   MockRead reads[] = {
+    CreateMockRead(*window_update_dummy),
+    CreateMockRead(*window_update_dummy),
+    CreateMockRead(*window_update),     // Four updates, therefore window
+    CreateMockRead(*window_update),     // size should increase by
+    CreateMockRead(*window_update),     // kDeltaWindowSize * 4
     CreateMockRead(*window_update),
-    CreateMockRead(*reply),
-    CreateMockRead(*body),
+    CreateMockRead(*resp),
+    CreateMockRead(*body_end),
     MockRead(true, 0, 0)  // EOF
   };
 
   scoped_refptr<DelayedSocketData> data(
-      new DelayedSocketData(2, reads, arraysize(reads),
+      new DelayedSocketData(0, reads, arraysize(reads),
                             writes, arraysize(writes)));
 
-  NormalSpdyTransactionHelper helper(CreatePostRequest(),
-                                     BoundNetLog(), GetParam());
+  // Setup the request
+  HttpRequestInfo request;
+  request.method = "POST";
+  request.url = GURL(kDefaultURL);
+  request.upload_data = new UploadData();
+  for (int i = 0; i < kFrameCount; ++i)
+    request.upload_data->AppendBytes(content->c_str(), content->size());
+
+  NormalSpdyTransactionHelper helper(request, BoundNetLog(), GetParam());
   helper.AddData(data.get());
   helper.RunPreTestSetup();
 
@@ -1410,18 +1452,20 @@ TEST_P(SpdyNetworkTransactionTest, WindowUpdate) {
   rv = callback.WaitForResult();
   EXPECT_EQ(OK, rv);
 
-  // ASSERT_TRUE(trans->spdy_http_stream_ != NULL);
-  // ASSERT_TRUE(trans->spdy_http_stream_->stream() != NULL);
-  // EXPECT_EQ(spdy::kInitialWindowSize + kDeltaWindowSize - kUploadDataSize,
-  //           trans->spdy_http_stream_->stream()->send_window_size());
+  SpdyHttpStream* stream =
+      static_cast<SpdyHttpStream*>(trans->stream_.get());
+  ASSERT_TRUE(stream != NULL);
+  ASSERT_TRUE(stream->stream() != NULL);
+  EXPECT_EQ(spdy::kInitialWindowSize +
+            kDeltaWindowSize * kDeltaCount -
+            kMaxSpdyFrameChunkSize * kFrameCount,
+            stream->stream()->send_window_size());
   helper.VerifyDataConsumed();
   SpdySession::SetFlowControl(false);
 }
 
-// Test that WINDOW_UPDATE frame causing overflow is handled correctly.
-// Since WINDOW_UPDATEs should appear only when we're in the middle of a
-// long POST, we create a few full frame writes to force a WINDOW_UPDATE in
-// between.
+// Test that WINDOW_UPDATE frame causing overflow is handled correctly.  We
+// use the same trick as in the above test to enforce our scenario.
 TEST_P(SpdyNetworkTransactionTest, WindowUpdateOverflow) {
   SpdySession::SetFlowControl(true);
 
@@ -1429,13 +1473,10 @@ TEST_P(SpdyNetworkTransactionTest, WindowUpdateOverflow) {
   // set content-length header correctly)
   static int kFrameCount = 3;
 
-  // Construct content for a data frame of maximum size.
   scoped_ptr<std::string> content(
       new std::string(kMaxSpdyFrameChunkSize, 'a'));
-
   scoped_ptr<spdy::SpdyFrame> req(ConstructSpdyPost(
       kMaxSpdyFrameChunkSize * kFrameCount, NULL, 0));
-
   scoped_ptr<spdy::SpdyFrame> body(
       ConstructSpdyBodyFrame(1, content->c_str(), content->size(), false));
   scoped_ptr<spdy::SpdyFrame> rst(
@@ -1456,21 +1497,6 @@ TEST_P(SpdyNetworkTransactionTest, WindowUpdateOverflow) {
       ConstructSpdyWindowUpdate(2, kDeltaWindowSize));
   scoped_ptr<spdy::SpdyFrame> reply(ConstructSpdyPostSynReply(NULL, 0));
 
-  // 1. Once we start writing a request, we do not read until the whole
-  // request is written completely, UNLESS, the first ReadSocket() call
-  // during the stream creation succeeded and caused another ReadSocket()
-  // to be scheduled.
-
-  // 2. We are trying to simulate WINDOW_UPDATE frame being received in the
-  // middle of a POST request's body being sent.
-
-  // In order to achieve 2, we force 1 to happen by giving
-  // DelayedSocketData a write delay of 0 and feeding SPDY dummy
-  // window_update2 frames which will be ignored, but will cause
-  // ReadSocket() to be scheduled and reads and writes to be interleaved,
-  // which will cause our window_update frame to be read right in the
-  // middle of a POST request being sent.
-
   MockRead reads[] = {
     CreateMockRead(*window_update2),
     CreateMockRead(*window_update2),
@@ -1516,7 +1542,7 @@ TEST_P(SpdyNetworkTransactionTest, WindowUpdateOverflow) {
 
 // Test that after hitting a send window size of 0, the write process
 // stalls and upon receiving WINDOW_UPDATE frame write resumes.
-//
+
 // This test constructs a POST request followed by enough data frames
 // containing 'a' that would make the window size 0, followed by another
 // data frame containing default content (which is "hello!") and this frame
@@ -1568,7 +1594,7 @@ TEST_P(SpdyNetworkTransactionTest, FlowControlStallResume) {
   writes[i++] = CreateMockWrite(*body2);
   writes[i] = CreateMockWrite(*body3);
 
-  // Construct read frame, just give enough space to upload the rest of the
+  // Construct read frame, give enough space to upload the rest of the
   // data.
   scoped_ptr<spdy::SpdyFrame> window_update(
       ConstructSpdyWindowUpdate(1, kUploadDataSize));
@@ -1608,10 +1634,13 @@ TEST_P(SpdyNetworkTransactionTest, FlowControlStallResume) {
   EXPECT_EQ(ERR_IO_PENDING, rv);
 
   MessageLoop::current()->RunAllPending(); // Write as much as we can.
-  // ASSERT_TRUE(trans->spdy_http_stream_ != NULL);
-  // ASSERT_TRUE(trans->spdy_http_stream_->stream() != NULL);
-  // EXPECT_EQ(0, trans->spdy_http_stream_->stream()->send_window_size());
-  // EXPECT_FALSE(trans->spdy_http_stream_->IsFinishedSendingBody());
+
+  SpdyHttpStream* stream =
+      static_cast<SpdyHttpStream*>(trans->stream_.get());
+  ASSERT_TRUE(stream != NULL);
+  ASSERT_TRUE(stream->stream() != NULL);
+  EXPECT_EQ(0, stream->stream()->send_window_size());
+  EXPECT_FALSE(stream->request_body_stream_->eof());
 
   data->ForceNextRead();   // Read in WINDOW_UPDATE frame.
   rv = callback.WaitForResult();
diff --git a/net/spdy/spdy_session.cc b/net/spdy/spdy_session.cc
index 549dd53..b1e9c23 100644
--- a/net/spdy/spdy_session.cc
+++ b/net/spdy/spdy_session.cc
@@ -433,8 +433,10 @@ int SpdySession::WriteStreamData(spdy::SpdyStreamId stream_id,
 
   // Obey send window size of the stream if flow control is enabled.
   if (use_flow_control_) {
-    if (stream->send_window_size() <= 0)
+    if (stream->send_window_size() <= 0) {
+      stream->set_stalled_by_flow_control(true);
       return ERR_IO_PENDING;
+    }
     int new_len = std::min(len, stream->send_window_size());
     if (new_len < len) {
       len = new_len;
diff --git a/net/spdy/spdy_stream.cc b/net/spdy/spdy_stream.cc
index 0204db6..09129fc 100644
--- a/net/spdy/spdy_stream.cc
+++ b/net/spdy/spdy_stream.cc
@@ -16,7 +16,7 @@ SpdyStream::SpdyStream(
     : continue_buffering_data_(true),
       stream_id_(stream_id),
       priority_(0),
-      window_update_write_pending_(false),
+      stalled_by_flow_control_(false),
       send_window_size_(spdy::kInitialWindowSize),
       pushed_(pushed),
       metrics_(Singleton<BandwidthMetrics>::get()),
@@ -88,9 +88,9 @@ void SpdyStream::IncreaseSendWindowSize(int delta_window_size) {
   DCHECK_GE(delta_window_size, 1);
   int new_window_size = send_window_size_ + delta_window_size;
 
-  // We shouldn't be receving WINDOW_UPDATE before or after that state,
-  // since before means we've not written SYN_STREAM yet, and after means
-  // we've written a DATA frame with FIN bit.
+  // We should ignore WINDOW_UPDATEs received before or after this state,
+  // since before means we've not written SYN_STREAM yet (i.e. it's too
+  // early) and after means we've written a DATA frame with FIN bit.
   if (io_state_ != STATE_SEND_BODY_COMPLETE)
     return;
 
@@ -112,16 +112,8 @@ void SpdyStream::IncreaseSendWindowSize(int delta_window_size) {
             << " by " << delta_window_size << " bytes";
   send_window_size_ = new_window_size;
 
-  // If the stream was stalled due to consumed window size, restart the
-  // I/O loop.
-  if (!closed() && !delegate_->IsFinishedSendingBody()) {
-    // Don't start the I/O loop for every WINDOW_UPDATE frame received,
-    // since we may receive many of them before the "write" due to first
-    // received WINDOW_UPDATE is completed.
-    if (window_update_write_pending_)
-      return;
-
-    window_update_write_pending_ = true;
+  if (stalled_by_flow_control_) {
+    stalled_by_flow_control_ = false;
     io_state_ = STATE_SEND_BODY;
     DoLoop(OK);
   }
@@ -235,7 +227,6 @@ void SpdyStream::OnDataReceived(const char* data, int length) {
 
 // This function is only called when an entire frame is written.
 void SpdyStream::OnWriteComplete(int bytes) {
-  window_update_write_pending_ = false;
   DCHECK_LE(0, bytes);
   send_bytes_ += bytes;
   if (cancelled() || closed())
@@ -408,8 +399,7 @@ int SpdyStream::DoSendBodyComplete(int result) {
   if (!delegate_)
     return ERR_UNEXPECTED;
 
-  delegate_->OnSendBodyComplete(result);
-  if (!delegate_->IsFinishedSendingBody())
+  if (!delegate_->OnSendBodyComplete(result))
     io_state_ = STATE_SEND_BODY;
   else
     io_state_ = STATE_WAITING_FOR_RESPONSE;
diff --git a/net/spdy/spdy_stream.h b/net/spdy/spdy_stream.h
index 4f315db..1b0a3f6 100644
--- a/net/spdy/spdy_stream.h
+++ b/net/spdy/spdy_stream.h
@@ -48,10 +48,8 @@ class SpdyStream : public base::RefCounted<SpdyStream> {
 
     // Called when data has been sent. |status| indicates network error
     // or number of bytes has been sent.
-    virtual void OnSendBodyComplete(int status) = 0;
-
     // Returns true if no more data to be sent.
-    virtual bool IsFinishedSendingBody() = 0;
+    virtual bool OnSendBodyComplete(int status) = 0;
 
     // Called when SYN_STREAM or SYN_REPLY received. |status| indicates network
     // error. Returns network error code.
@@ -110,6 +108,10 @@ class SpdyStream : public base::RefCounted<SpdyStream> {
     send_window_size_ = window_size;
   }
 
+  void set_stalled_by_flow_control(bool stalled) {
+    stalled_by_flow_control_ = stalled;
+  }
+
   // Increases |send_window_size_| with delta extracted from a WINDOW_UPDATE
   // frame; sends a RST_STREAM if delta overflows |send_window_size_| and
   // removes the stream from the session.
@@ -218,9 +220,8 @@ class SpdyStream : public base::RefCounted<SpdyStream> {
   std::string path_;
   int priority_;
 
-  // Tracks when a window update has already triggered a write, but
-  // the write has not yet completed.
-  bool window_update_write_pending_;
+  // Flow control variables.
+  bool stalled_by_flow_control_;
   int send_window_size_;
 
   const bool pushed_;
diff --git a/net/spdy/spdy_stream_unittest.cc b/net/spdy/spdy_stream_unittest.cc
index 64abcc1..713b3cd 100644
--- a/net/spdy/spdy_stream_unittest.cc
+++ b/net/spdy/spdy_stream_unittest.cc
@@ -55,11 +55,8 @@ class TestSpdyStreamDelegate : public SpdyStream::Delegate {
     ADD_FAILURE() << "OnSendBody should not be called";
     return ERR_UNEXPECTED;
   }
-  virtual void OnSendBodyComplete(int status) {
+  virtual bool OnSendBodyComplete(int status) {
     ADD_FAILURE() << "OnSendBodyComplete should not be called";
-  }
-
-  virtual bool IsFinishedSendingBody() {
     return true;
   }