SPDY - implement greedy approach to read all the data and process it

net/spdy/spdy_network_transaction_spdy3_unittest.cc

Index: net/spdy/spdy_network_transaction_spdy3_unittest.cc
===================================================================
--- net/spdy/spdy_network_transaction_spdy3_unittest.cc	(revision 176979)
+++ net/spdy/spdy_network_transaction_spdy3_unittest.cc	(working copy)
@@ -11,9 +11,12 @@
 #include "base/bind_helpers.h"
 #include "base/file_util.h"
 #include "base/files/scoped_temp_dir.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/memory/scoped_vector.h"
 #include "net/base/auth.h"
+#include "net/base/io_buffer.h"
 #include "net/base/net_log_unittest.h"
+#include "net/base/test_data_stream.h"
 #include "net/base/upload_bytes_element_reader.h"
 #include "net/base/upload_data_stream.h"
 #include "net/base/upload_file_element_reader.h"
@@ -1046,8 +1049,8 @@
 
     out.rv = trans1->Start(&httpreq1, callback1.callback(), log);
     ASSERT_EQ(out.rv, ERR_IO_PENDING);
-    // run transaction 1 through quickly to force a read of our SETTINGS
-    // frame
+    // Run transaction 1 through quickly to force a read of our SETTINGS
+    // frame.
     out.rv = callback1.WaitForResult();
     ASSERT_EQ(OK, out.rv);
 
@@ -4504,6 +4507,187 @@
   EXPECT_EQ("messagemessagemessagemessage", out.response_data);
 }
 
+// Verify the case where SpdySession reads all the data synchronously without
+// yielding.

[Ryan Hamilton, 2013/01/19 00:32:13]

How does this test verify that the session yields?  It definitely verifies that
trans->Read() returns synchronously, but I'm not sure that's the same thing.

[ramant (doing other things), 2013/02/04 18:30:46]

With out DeterminsticSocketData changes and TaskObserver changes, checked if
"SpdySession::DoRead" posted a task or not. This method has only one task that
is getting posted.

In SyncRead case, this method won't post any task. In NonSyceRead case where it
yields, it posts a task.

Our changes to DeterministicSocketData show that all reads are happening in
SYNCHRONOUS mode. What do you think of this change.

Many many thanks for all your help.

+TEST_P(SpdyNetworkTransactionSpdy3Test, BufferedSyncRead) {
+  BufferedSpdyFramer framer(3, false);
+
+  scoped_ptr<SpdyFrame> req(ConstructSpdyGet(NULL, 0, false, 1, LOWEST));
+  MockWrite writes[] = { CreateMockWrite(*req) };
+
+  scoped_ptr<SpdyFrame> syn_reply(
+      ConstructSpdyGetSynReply(NULL, 0, 1));
+  syn_reply->set_flags(CONTROL_FLAG_NONE);  // Turn off FIN bit.
+
+  const int kPayloadSize = 1600;
+  net::TestDataStream test_stream;

[Ryan Hamilton, 2013/01/19 00:32:13]

nit: no need for test::, I think.  (And in the next test)

[ramant (doing other things), 2013/02/04 18:30:46]

Made all these changes in SpdySession unit tests.

Done.

+  scoped_refptr<net::IOBuffer> payload(new net::IOBuffer(kPayloadSize));
+  test_stream.GetBytes(payload->data(), kPayloadSize);
+  char* payload_data = payload->data();

[Ryan Hamilton, 2013/01/19 00:32:13]

nit: move this line above the previous, and use payload_data instead of
payload->data() as the first argument for GetBytes().  (And in the next test).

[ramant (doing other things), 2013/02/04 18:30:46]

Made all these changes in SpdySession unit tests.

Done.

+  std::string payload_content(payload_data, kPayloadSize);
+  scoped_ptr<SpdyFrame> data_frame(
+      framer.CreateDataFrame(1, payload_data, kPayloadSize, DATA_FLAG_FIN));
+  const SpdyFrame* frames[2] = {
+    syn_reply.get(),
+    data_frame.get()
+  };
+  char combined_frames[5000];
+  int combined_frames_len =
+      CombineFrames(frames, arraysize(frames),
+                    combined_frames, arraysize(combined_frames));
+
+  MockRead reads[] = {
+    MockRead(ASYNC, combined_frames, combined_frames_len),
+    MockRead(ASYNC, 0, 0)  // EOF
+  };
+
+  DelayedSocketData data(1, reads, arraysize(reads),
+                         writes, arraysize(writes));
+
+  NormalSpdyTransactionHelper helper(CreateGetRequest(),
+                                     BoundNetLog(), GetParam(), NULL);
+  helper.RunPreTestSetup();
+  helper.AddData(&data);
+  HttpNetworkTransaction* trans = helper.trans();
+
+  TestCompletionCallback callback;
+  int rv = trans->Start(
+      &CreateGetRequest(), callback.callback(), BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+
+  TransactionHelperResult out = helper.output();
+  out.rv = callback.WaitForResult();
+  EXPECT_EQ(out.rv, OK);
+
+  const HttpResponseInfo* response = trans->GetResponseInfo();
+  EXPECT_TRUE(response->headers != NULL);
+  EXPECT_TRUE(response->was_fetched_via_spdy);
+  out.status_line = response->headers->GetStatusLine();
+  out.response_info = *response;  // Make a copy so we can verify.
+
+  // Read Data.
+  TestCompletionCallback read_callback;
+
+  std::string content;
+  int reads_completed = 0;
+  do {
+    // Read data in chunks.
+    scoped_refptr<net::IOBuffer> buf(new net::IOBuffer(kPayloadSize));
+    rv = trans->Read(buf, kPayloadSize, read_callback.callback());

[Ryan Hamilton, 2013/01/19 00:32:13]

nit: I would just do ASSERT_LE(0, rv); 

[ramant (doing other things), 2013/02/04 18:30:46]

Deleted this test.

+    if (rv > 0) {
+      content.append(buf->data(), rv);
+    } else if (rv < 0) {
+      FAIL() << "Unexpected read error: " << rv;
+    }
+    reads_completed++;
+  } while (rv > 0);
+
+  out.response_data.swap(content);
+
+  // Flush the MessageLoop while the SpdySessionDependencies (in particular, the
+  // MockClientSocketFactory) are still alive.
+  MessageLoop::current()->RunUntilIdle();

[Ryan Hamilton, 2013/01/19 00:32:13]

Can you do this as the last thing line in this test?

[ramant (doing other things), 2013/02/04 18:30:46]

Deleted this test.

+
+  // Verify that we consumed all test data.
+  helper.VerifyDataConsumed();
+
+  EXPECT_EQ(OK, out.rv);
+  EXPECT_EQ("HTTP/1.1 200 OK", out.status_line);
+  EXPECT_EQ(payload_content, out.response_data);
+}
+
+// Verify the case where SpdySession reads kMaxReadBytes data synchronously and
+// then reads more data after yielding.

[Ryan Hamilton, 2013/01/19 00:32:13]

I don't think I see how this shows that the session yields?

[ramant (doing other things), 2013/02/04 18:30:46]

Wrote the new unit tests.

+TEST_P(SpdyNetworkTransactionSpdy3Test, BufferedASyncRead) {
+  BufferedSpdyFramer framer(3, false);
+
+  scoped_ptr<SpdyFrame> req(ConstructSpdyGet(NULL, 0, false, 1, LOWEST));
+  MockWrite writes[] = { CreateMockWrite(*req) };
+
+  scoped_ptr<SpdyFrame> syn_reply(
+      ConstructSpdyGetSynReply(NULL, 0, 1));
+  syn_reply->set_flags(CONTROL_FLAG_NONE);  // Turn off FIN bit.
+
+  const int kPayloadSize = kMaxReadBytes * 2;
+  net::TestDataStream test_stream;
+  scoped_refptr<net::IOBuffer> payload(new net::IOBuffer(kPayloadSize));
+  test_stream.GetBytes(payload->data(), kPayloadSize);
+  char* payload_data = payload->data();
+  std::string payload_content(payload_data, kPayloadSize);
+  scoped_ptr<SpdyFrame> data_frame(
+      framer.CreateDataFrame(1, payload_data, kPayloadSize, DATA_FLAG_FIN));
+  const SpdyFrame* frames[2] = {
+    syn_reply.get(),
+    data_frame.get()
+  };
+  int heap_size = kPayloadSize * 2;
+  scoped_ptr<char, base::FreeDeleter>
+      combined_frames(static_cast<char*>(malloc(heap_size)));
+  CHECK(combined_frames.get());
+  int combined_frames_len =
+      CombineFrames(frames, arraysize(frames),
+                    combined_frames.get(), heap_size);
+
+  MockRead reads[] = {
+    MockRead(ASYNC, combined_frames.get(), combined_frames_len),
+    MockRead(ASYNC, 0, 0)  // EOF
+  };
+
+  DelayedSocketData data(1, reads, arraysize(reads),
+                         writes, arraysize(writes));
+
+  NormalSpdyTransactionHelper helper(CreateGetRequest(),
+                                     BoundNetLog(), GetParam(), NULL);
+  helper.RunPreTestSetup();
+  helper.AddData(&data);
+  HttpNetworkTransaction* trans = helper.trans();
+
+  TestCompletionCallback callback;
+  int rv = trans->Start(
+      &CreateGetRequest(), callback.callback(), BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+
+  TransactionHelperResult out = helper.output();
+  out.rv = callback.WaitForResult();
+  EXPECT_EQ(out.rv, OK);
+
+  const HttpResponseInfo* response = trans->GetResponseInfo();
+  EXPECT_TRUE(response->headers != NULL);
+  EXPECT_TRUE(response->was_fetched_via_spdy);
+  out.status_line = response->headers->GetStatusLine();
+  out.response_info = *response;  // Make a copy so we can verify.
+
+  // Read Data.
+  TestCompletionCallback read_callback;
+
+  std::string content;
+  int reads_completed = 0;
+  do {
+    // Read data in chunks.
+    scoped_refptr<net::IOBuffer> buf(new net::IOBuffer(kMaxReadBytes));
+    rv = trans->Read(buf, kMaxReadBytes, read_callback.callback());
+    if (rv > 0) {
+      content.append(buf->data(), rv);
+    } else if (rv < 0) {
+      FAIL() << "Unexpected read error: " << rv;
+    }
+    reads_completed++;
+  } while (rv > 0);
+
+  out.response_data.swap(content);
+
+  // Flush the MessageLoop while the SpdySessionDependencies (in particular, the
+  // MockClientSocketFactory) are still alive.
+  MessageLoop::current()->RunUntilIdle();
+
+  // Verify that we consumed all test data.
+  helper.VerifyDataConsumed();
+
+  EXPECT_EQ(OK, out.rv);
+  EXPECT_EQ("HTTP/1.1 200 OK", out.status_line);
+  EXPECT_EQ(payload_content, out.response_data);
+}
+
 // Verify the case where we buffer data and close the connection.
 TEST_P(SpdyNetworkTransactionSpdy3Test, BufferedClosed) {
   BufferedSpdyFramer framer(3, false);