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

net/spdy/spdy_session_spdy2_unittest.cc

Index: net/spdy/spdy_session_spdy2_unittest.cc
===================================================================
--- net/spdy/spdy_session_spdy2_unittest.cc	(revision 180807)
+++ net/spdy/spdy_session_spdy2_unittest.cc	(working copy)
@@ -4,13 +4,18 @@
 
 #include "net/spdy/spdy_session.h"
 
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/scoped_vector.h"
 #include "net/base/cert_test_util.h"
 #include "net/base/host_cache.h"
+#include "net/base/io_buffer.h"
 #include "net/base/ip_endpoint.h"
 #include "net/base/net_log_unittest.h"
 #include "net/base/test_data_directory.h"
+#include "net/base/test_data_stream.h"
 #include "net/spdy/spdy_io_buffer.h"
 #include "net/spdy/spdy_session_pool.h"
+#include "net/spdy/spdy_session_test_util.h"
 #include "net/spdy/spdy_stream.h"
 #include "net/spdy/spdy_test_util_spdy2.h"
 #include "testing/platform_test.h"
@@ -1622,4 +1627,184 @@
   spdy_session_pool_->Remove(session);
 }
 
+// Test that SpdySession::DoRead reads data from the socket without yielding.
+// This test makes 32k - 1 bytes of data available on the socket for reading. It
+// then verifies that it has read all the available data without yielding.
+TEST_F(SpdySessionSpdy2Test, ReadDataWithoutYielding) {
+  MockConnect connect_data(SYNCHRONOUS, OK);
+  BufferedSpdyFramer framer(2, false);
+
+  scoped_ptr<SpdyFrame> req1(ConstructSpdyGet(NULL, 0, false, 1, MEDIUM));
+  MockWrite writes[] = {
+    CreateMockWrite(*req1, 0),
+  };
+
+  // Build buffers of size 8k.
+  ASSERT_EQ(32 * 1024, kMaxReadBytes);
+  const int kPayloadSize = kMaxReadBytes / 4 - SpdyDataFrame::size();
+  TestDataStream test_stream;
+  scoped_refptr<net::IOBuffer> payload(new net::IOBuffer(kPayloadSize));
+  char* payload_data = payload->data();
+  test_stream.GetBytes(payload_data, kPayloadSize);
+
+  scoped_ptr<SpdyFrame> data_frame1(
+      framer.CreateDataFrame(1, payload_data, kPayloadSize, DATA_FLAG_NONE));
+  scoped_ptr<SpdyFrame> data_frame(
+      framer.CreateDataFrame(1, payload_data, kPayloadSize - 1, DATA_FLAG_FIN));

[Ryan Sleevi, 2013/02/07 02:43:50]

naming: data_frame1 and data_frame seem to have interesting/odd naming (one
numbered, the other not - you would expect data_frame1/data_frame2, for ex)

But perhaps we can improve this further:
data_frame1 -> partial_data_frame
data_frame -> nearly_full_data_frame

These same comments apply to the following test.

[ramant (doing other things), 2013/02/07 20:03:55]

Done.

+
+  scoped_ptr<SpdyFrame> resp1(ConstructSpdyGetSynReply(NULL, 0, 1));
+
+  // Write 1 byte less than kMaxReadBytes to check that DoRead reads up to 32k
+  // bytes.
+  MockRead reads[] = {
+    CreateMockRead(*resp1, 1),
+    CreateMockRead(*data_frame1, 2),
+    CreateMockRead(*data_frame1, 3, SYNCHRONOUS),
+    CreateMockRead(*data_frame1, 4, SYNCHRONOUS),
+    CreateMockRead(*data_frame, 5, SYNCHRONOUS),
+    MockRead(ASYNC, 0, 6)  // EOF
+  };
+
+  // Create SpdySession and SpdyStream and send the request.
+  DeterministicSocketData data(reads, arraysize(reads),
+                               writes, arraysize(writes));
+  data.set_connect_data(connect_data);
+  session_deps_.host_resolver->set_synchronous_mode(true);
+  session_deps_.deterministic_socket_factory->AddSocketDataProvider(&data);
+
+  SSLSocketDataProvider ssl(SYNCHRONOUS, OK);
+  session_deps_.deterministic_socket_factory->AddSSLSocketDataProvider(&ssl);
+
+  CreateDeterministicNetworkSession();
+
+  scoped_refptr<SpdySession> session = CreateInitializedSession();
+
+  scoped_refptr<SpdyStream> spdy_stream1;
+  TestCompletionCallback callback1;
+  GURL url1("http://www.google.com");
+  EXPECT_EQ(OK, session->CreateStream(url1, MEDIUM, &spdy_stream1,
+                                      BoundNetLog(), callback1.callback()));
+  EXPECT_EQ(0u, spdy_stream1->stream_id());
+
+  scoped_ptr<SpdyHeaderBlock> headers(new SpdyHeaderBlock);
+  (*headers)["method"] = "GET";
+  (*headers)["scheme"] = url1.scheme();
+  (*headers)["host"] = url1.host();
+  (*headers)["url"] = url1.path();
+  (*headers)["version"] = "HTTP/1.1";
+
+  spdy_stream1->set_spdy_headers(headers.Pass());
+  EXPECT_TRUE(spdy_stream1->HasUrl());
+  spdy_stream1->SendRequest(false);
+
+  // Set up the TaskObserver to verify SpdySession::DoRead doesn't post a task.
+  SpdySessionTestTaskObserver observer("spdy_session.cc", "DoRead");
+
+  // Run until 1st read.
+  EXPECT_EQ(0u, spdy_stream1->stream_id());
+  data.RunFor(2);
+  EXPECT_EQ(1u, spdy_stream1->stream_id());
+  EXPECT_EQ(0u, observer.posted_count());
+
+  // Read all the data and verify SpdySession::DoRead has not posted a task.
+  data.RunFor(4);
+
+  // Verify task observer's posted_count is zero, which indicates DoRead read
+  // all the available data.
+  EXPECT_EQ(0u, observer.posted_count());
+  EXPECT_TRUE(data.at_write_eof());
+  EXPECT_TRUE(data.at_read_eof());
+}
+
+// Test that SpdySession::DoRead yields while reading the data. This test makes
+// 32k + 1 bytes of data available on the socket for reading. It then verifies
+// that DoRead has yielded even though there is data available for it to read
+// (i.e, socket()->Read didn't return ERR_IO_PENDING during socket reads).
+TEST_F(SpdySessionSpdy2Test, TestYieldingDuringReadData) {
+  MockConnect connect_data(SYNCHRONOUS, OK);
+  BufferedSpdyFramer framer(2, false);
+
+  scoped_ptr<SpdyFrame> req1(ConstructSpdyGet(NULL, 0, false, 1, MEDIUM));
+  MockWrite writes[] = {
+    CreateMockWrite(*req1, 0),
+  };
+
+  // Build buffers of size 8k.
+  ASSERT_EQ(32 * 1024, kMaxReadBytes);
+  const int kPayloadSize = kMaxReadBytes / 4 - SpdyDataFrame::size();
+  TestDataStream test_stream;
+  scoped_refptr<net::IOBuffer> payload(new net::IOBuffer(kPayloadSize));
+  char* payload_data = payload->data();
+  test_stream.GetBytes(payload_data, kPayloadSize);
+
+  scoped_ptr<SpdyFrame> data_frame1(
+      framer.CreateDataFrame(1, payload_data, kPayloadSize, DATA_FLAG_NONE));
+  scoped_ptr<SpdyFrame> data_frame(
+      framer.CreateDataFrame(1, "h", 1, DATA_FLAG_FIN));
+
+  scoped_ptr<SpdyFrame> resp1(ConstructSpdyGetSynReply(NULL, 0, 1));
+
+  // Write 1 byte more than kMaxReadBytes to check that DoRead yields.
+  MockRead reads[] = {
+    CreateMockRead(*resp1, 1),
+    CreateMockRead(*data_frame1, 2),
+    CreateMockRead(*data_frame1, 3, SYNCHRONOUS),
+    CreateMockRead(*data_frame1, 4, SYNCHRONOUS),
+    CreateMockRead(*data_frame1, 5, SYNCHRONOUS),
+    CreateMockRead(*data_frame, 6, SYNCHRONOUS),

[Ryan Sleevi, 2013/02/07 02:43:50]

So one thing you're missing from only these two tests are frames where the
underlying socket has more data than kMaxReadBytes (which will be a common case)

Seems like you should have another test to handle say, 4 reads of kMaxReadBytes
/ 4 (-spdy size), one read of kMaxReadBytes (-spdysize), and perhaps some reads
of kMaxReadBytes + [some value] (-spdysize), and then check that your
assumptions are correct regarding the post task behaviours.

I would expect that it would never try to read > 32K from the underlying
transport.

I'm not sure if DeterministicSocketData supports setting up MockReads that are
GREATER than what the client asks for, but it does strike me as a bit of code
that's untested here.

[ramant (doing other things), 2013/02/07 20:03:55]

Created a test that is DISABLED. Added a TODO. Will submit a separate CL after
making changes to DeterministicSocketData .

+    MockRead(ASYNC, 0, 7)  // EOF
+  };
+
+  // Create SpdySession and SpdyStream and send the request.
+  DeterministicSocketData data(reads, arraysize(reads),
+                               writes, arraysize(writes));
+  data.set_connect_data(connect_data);
+  session_deps_.host_resolver->set_synchronous_mode(true);
+  session_deps_.deterministic_socket_factory->AddSocketDataProvider(&data);
+
+  SSLSocketDataProvider ssl(SYNCHRONOUS, OK);
+  session_deps_.deterministic_socket_factory->AddSSLSocketDataProvider(&ssl);
+
+  CreateDeterministicNetworkSession();
+
+  scoped_refptr<SpdySession> session = CreateInitializedSession();
+
+  scoped_refptr<SpdyStream> spdy_stream1;
+  TestCompletionCallback callback1;
+  GURL url1("http://www.google.com");
+  EXPECT_EQ(OK, session->CreateStream(url1, MEDIUM, &spdy_stream1,
+                                      BoundNetLog(), callback1.callback()));
+  EXPECT_EQ(0u, spdy_stream1->stream_id());
+
+  scoped_ptr<SpdyHeaderBlock> headers(new SpdyHeaderBlock);
+  (*headers)["method"] = "GET";
+  (*headers)["scheme"] = url1.scheme();
+  (*headers)["host"] = url1.host();
+  (*headers)["url"] = url1.path();
+  (*headers)["version"] = "HTTP/1.1";
+
+  spdy_stream1->set_spdy_headers(headers.Pass());
+  EXPECT_TRUE(spdy_stream1->HasUrl());
+  spdy_stream1->SendRequest(false);
+
+  // Set up the TaskObserver to verify SpdySession::DoRead posts a task.
+  SpdySessionTestTaskObserver observer("spdy_session.cc", "DoRead");
+
+  // Run until 1st read.
+  EXPECT_EQ(0u, spdy_stream1->stream_id());
+  data.RunFor(2);
+  EXPECT_EQ(1u, spdy_stream1->stream_id());
+  EXPECT_EQ(0u, observer.posted_count());
+
+  // Read all the data and verify SpdySession::DoRead has posted a task.
+  data.RunFor(6);
+
+  // Verify task observer's posted_count is 1, which indicates DoRead has posted
+  // only one task and thus yielded though there is data available for it to
+  // read.
+  EXPECT_EQ(1u, observer.posted_count());
+  EXPECT_TRUE(data.at_write_eof());
+  EXPECT_TRUE(data.at_read_eof());
+}
+
 }  // namespace net