Change SPDY to call request_callback after data is sent.

This cl makes SPDY to match HTTP and QUIC implementations
and call the request_callback only after data is sent.

The unittests are corrected accordingly.

Review-Url: https://codereview.chromium.org/2064593002
Cr-Commit-Position: refs/heads/master@{#400629}

net/spdy/spdy_http_stream.cc

@@ -302,13 +302,12 @@ void SpdyHttpStream::Cancel() {
 }
 
 void SpdyHttpStream::OnRequestHeadersSent() {
-  if (!request_callback_.is_null())
-    DoRequestCallback(OK);
-
-  // TODO(akalin): Do this immediately after sending the request
-  // headers.
-  if (HasUploadData())
+  if (HasUploadData()) {
     ReadAndSendRequestBodyData();
+  } else {
+    if (!request_callback_.is_null())
+      DoRequestCallback(OK);
+  }
 }
 
 SpdyResponseHeadersStatus SpdyHttpStream::OnResponseHeadersUpdated(
@@ -436,9 +435,16 @@ void SpdyHttpStream::OnStreamCreated(
 void SpdyHttpStream::ReadAndSendRequestBodyData() {
   CHECK(HasUploadData());
   CHECK_EQ(request_body_buf_size_, 0);
-
-  if (request_info_->upload_data_stream->IsEOF())
+  if (request_info_->upload_data_stream->IsEOF()) {
+    // This callback does not happen to be called, because it is called
+    // in OnRequestBodyReadCompleted() function when eof happens, and then it
+    // is cleared. But better to be paranoid and handle this just in case.
+    // Generally, this eof check just makes sure it is really the eof and then
+    // doloop exists.
+    if (!request_callback_.is_null())
+      DoRequestCallback(OK);
     return;
+  }
 
   // Read the data from the request body stream.
   const int rv = request_info_->upload_data_stream
@@ -458,9 +464,12 @@ void SpdyHttpStream::OnRequestBodyReadCompleted(int status) {
   CHECK_GE(status, 0);
   request_body_buf_size_ = status;
   const bool eof = request_info_->upload_data_stream->IsEOF();
-  // Only the final fame may have a length of 0.
+  // Only the final frame may have a length of 0.
   if (eof) {
     CHECK_GE(request_body_buf_size_, 0);
+    // Call the cb only after all data is sent.
+    if (!request_callback_.is_null())
+      DoRequestCallback(OK);
   } else {
     CHECK_GT(request_body_buf_size_, 0);
   }
@@ -533,7 +542,6 @@ void SpdyHttpStream::DoBufferedReadCallback() {
 void SpdyHttpStream::DoRequestCallback(int rv) {
   CHECK_NE(rv, ERR_IO_PENDING);
   CHECK(!request_callback_.is_null());
-
   // Since Run may result in being called back, reset request_callback_ in
   // advance.
   base::ResetAndReturn(&request_callback_).Run(rv);

net/spdy/spdy_http_stream_unittest.cc

@@ -390,6 +390,64 @@ TEST_P(SpdyHttpStreamTest, SendChunkedPost) {
   EXPECT_FALSE(HasSpdySession(http_session_->spdy_session_pool(), key));
 }
 
+// This unittest tests the request callback is properly called and handled.
+TEST_P(SpdyHttpStreamTest, SendChunkedPostLastEmpty) {
+  std::unique_ptr<SpdySerializedFrame> req(
+      spdy_util_.ConstructChunkedSpdyPost(NULL, 0));
+  std::unique_ptr<SpdySerializedFrame> chunk(
+      spdy_util_.ConstructSpdyBodyFrame(1, nullptr, 0, true));
+  MockWrite writes[] = {
+      CreateMockWrite(*req, 0),  // request
+      CreateMockWrite(*chunk, 1),
+  };
+
+  std::unique_ptr<SpdySerializedFrame> resp(
+      spdy_util_.ConstructSpdyPostSynReply(NULL, 0));
+  MockRead reads[] = {
+      CreateMockRead(*resp, 2),
+      CreateMockRead(*chunk, 3),
+      MockRead(SYNCHRONOUS, 0, 4)  // EOF
+  };
+
+  HostPortPair host_port_pair("www.example.org", 80);
+  SpdySessionKey key(host_port_pair, ProxyServer::Direct(),
+                     PRIVACY_MODE_DISABLED);
+  InitSession(reads, arraysize(reads), writes, arraysize(writes), key);
+  EXPECT_EQ(spdy_util_.spdy_version(), session_->GetProtocolVersion());
+
+  ChunkedUploadDataStream upload_stream(0);
+  upload_stream.AppendData(nullptr, 0, true);
+
+  HttpRequestInfo request;
+  request.method = "POST";
+  request.url = GURL("http://www.example.org/");
+  request.upload_data_stream = &upload_stream;
+
+  ASSERT_EQ(OK, upload_stream.Init(TestCompletionCallback().callback()));
+
+  TestCompletionCallback callback;
+  HttpResponseInfo response;
+  HttpRequestHeaders headers;
+  BoundNetLog net_log;
+  SpdyHttpStream http_stream(session_, true);
+  ASSERT_EQ(OK, http_stream.InitializeStream(&request, DEFAULT_PRIORITY,
+                                             net_log, CompletionCallback()));
+  EXPECT_EQ(ERR_IO_PENDING,
+            http_stream.SendRequest(headers, &response, callback.callback()));
+  EXPECT_TRUE(HasSpdySession(http_session_->spdy_session_pool(), key));
+
+  EXPECT_EQ(OK, callback.WaitForResult());
+
+  EXPECT_EQ(static_cast<int64_t>(req->size() + chunk->size()),
+            http_stream.GetTotalSentBytes());
+  EXPECT_EQ(static_cast<int64_t>(resp->size() + chunk->size()),
+            http_stream.GetTotalReceivedBytes());
+
+  // Because the server closed the connection, there shouldn't be a session
+  // in the pool anymore.
+  EXPECT_FALSE(HasSpdySession(http_session_->spdy_session_pool(), key));
+}
+
 TEST_P(SpdyHttpStreamTest, ConnectionClosedDuringChunkedPost) {
   BufferedSpdyFramer framer(spdy_util_.spdy_version());
 
@@ -437,7 +495,7 @@ TEST_P(SpdyHttpStreamTest, ConnectionClosedDuringChunkedPost) {
             http_stream.SendRequest(headers, &response, callback.callback()));
   EXPECT_TRUE(HasSpdySession(http_session_->spdy_session_pool(), key));
 
-  EXPECT_EQ(OK, callback.WaitForResult());
+  EXPECT_EQ(ERR_CONNECTION_CLOSED, callback.WaitForResult());
 
   EXPECT_EQ(static_cast<int64_t>(req->size() + body->size()),
             http_stream.GetTotalSentBytes());
@@ -520,8 +578,7 @@ TEST_P(SpdyHttpStreamTest, DelayedSendChunkedPost) {
 
   // Complete the initial request write and the first chunk.
   base::RunLoop().RunUntilIdle();
-  ASSERT_TRUE(callback.have_result());
-  EXPECT_EQ(OK, callback.WaitForResult());
+  ASSERT_FALSE(callback.have_result());
 
   // Now append the final two chunks which will enqueue two more writes.
   upload_stream.AppendData(kUploadData1, kUploadData1Size, false);
@@ -529,6 +586,8 @@ TEST_P(SpdyHttpStreamTest, DelayedSendChunkedPost) {
 
   // Finish writing all the chunks and do all reads.
   base::RunLoop().RunUntilIdle();
+  ASSERT_TRUE(callback.have_result());
+  EXPECT_EQ(OK, callback.WaitForResult());
 
   EXPECT_EQ(static_cast<int64_t>(req->size() + chunk1->size() + chunk2->size() +
                                  chunk3->size()),
@@ -620,8 +679,7 @@ TEST_P(SpdyHttpStreamTest, DelayedSendChunkedPostWithEmptyFinalDataFrame) {
 
   // Complete the initial request write and the first chunk.
   base::RunLoop().RunUntilIdle();
-  ASSERT_TRUE(callback.have_result());
-  EXPECT_EQ(OK, callback.WaitForResult());
+  ASSERT_FALSE(callback.have_result());
 
   EXPECT_EQ(static_cast<int64_t>(req->size() + chunk1->size()),
             http_stream->GetTotalSentBytes());
@@ -632,6 +690,8 @@ TEST_P(SpdyHttpStreamTest, DelayedSendChunkedPostWithEmptyFinalDataFrame) {
 
   // Finish writing the final frame, and perform all reads.
   base::RunLoop().RunUntilIdle();
+  ASSERT_TRUE(callback.have_result());
+  EXPECT_EQ(OK, callback.WaitForResult());
 
   // Check response headers.
   ASSERT_EQ(OK, http_stream->ReadResponseHeaders(callback.callback()));
@@ -835,8 +895,7 @@ TEST_P(SpdyHttpStreamTest, DelayedSendChunkedPostWithWindowUpdate) {
 
   // Complete the initial request write and first chunk.
   base::RunLoop().RunUntilIdle();
-  ASSERT_TRUE(callback.have_result());
-  EXPECT_EQ(OK, callback.WaitForResult());
+  ASSERT_FALSE(callback.have_result());
 
   EXPECT_EQ(static_cast<int64_t>(req->size()),
             http_stream->GetTotalSentBytes());
@@ -844,6 +903,9 @@ TEST_P(SpdyHttpStreamTest, DelayedSendChunkedPostWithWindowUpdate) {
 
   upload_stream.AppendData(kUploadData, kUploadDataSize, true);
 
+  ASSERT_TRUE(callback.have_result());
+  EXPECT_EQ(OK, callback.WaitForResult());
+
   // Verify that the window size has decreased.
   ASSERT_TRUE(http_stream->stream() != NULL);
   EXPECT_NE(static_cast<int>(

net/spdy/spdy_network_transaction_unittest.cc

@@ -280,7 +280,7 @@ class SpdyNetworkTransactionTest
         session_->spdy_session_pool()->CloseCurrentSessions(ERR_ABORTED);
     }
 
-    void WaitForHeaders() { output_.rv = callback_.WaitForResult(); }
+    void WaitForCallbackToComplete() { output_.rv = callback_.WaitForResult(); }
 
     // Most tests will want to call this function. In particular, the MockReads
     // should end with an empty read, and that read needs to be processed to
@@ -2051,8 +2051,7 @@ TEST_P(SpdyNetworkTransactionTest, ResponseBeforePostCompletes) {
 
   ASSERT_TRUE(helper.StartDefaultTest());
 
-  helper.WaitForHeaders();
-  EXPECT_EQ(OK, helper.output().rv);
+  base::RunLoop().RunUntilIdle();
 
   // Process the request headers, SYN_REPLY, and response body.
   // The request body is still in flight.
@@ -2061,6 +2060,8 @@ TEST_P(SpdyNetworkTransactionTest, ResponseBeforePostCompletes) {
 
   // Finish sending the request body.
   upload_chunked_data_stream()->AppendData(kUploadData, kUploadDataSize, true);
+  helper.WaitForCallbackToComplete();
+  EXPECT_EQ(OK, helper.output().rv);
 
   std::string response_body;
   EXPECT_EQ(OK, ReadTransaction(helper.trans(), &response_body));