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,165 @@
   spdy_session_pool_->Remove(session);
 }
 
+TEST_F(SpdySessionSpdy2Test, SyncRead) {

[Ryan Sleevi, 2013/02/06 23:04:46]

Nit: You should have a high-level comment for the test indicating what is being
tested, along with possible highlights of comments within the test.

If this test were to fail, would it be obvious from this code what was the
reason (for the test or the failure?) Having taken a few weeks from reading this
code, I'm not sure I could put it together without really digging through things
again (meaning I'm having trouble reviewing :-p)

[ramant (doing other things), 2013/02/07 02:11:07]

+1 to the above.

Added comments on what the tests are trying to test. 

Done.

+  MockConnect connect_data(SYNCHRONOUS, OK);
+  BufferedSpdyFramer framer(2, false);
+
+  scoped_ptr<SpdyFrame> req1(ConstructSpdyGet(NULL, 0, false, 1, MEDIUM));
+  MockWrite writes[] = {
+    CreateMockWrite(*req1, 0),
+  };
+
+  const int kPayloadSize = 1600;
+  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, DATA_FLAG_FIN));

[Ryan Sleevi, 2013/02/06 23:04:46]

Is it legal to dupe IOBuffers like this? Just wondering why the testing
infrastructure doesn't enforce uniqueness here.

It's (probably) fine, I just want to make sure we don't cause any weird
ownership issues.

[ramant (doing other things), 2013/02/07 02:11:07]

SpdyFramer::CreateDataFrame calls SpdyFrameBuilder to allocate a buffer of size
greater than  kPayloadSize and copies the payload_data into that buffer.

+
+  scoped_ptr<SpdyFrame> resp1(ConstructSpdyGetSynReply(NULL, 0, 1));
+
+  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
+  };
+
+  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);
+
+  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());
+
+  // Do 4 reads.
+  data.RunFor(4);
+  EXPECT_EQ(0u, observer.posted_count());
+  EXPECT_TRUE(data.at_write_eof());
+  EXPECT_TRUE(data.at_read_eof());
+}
+
+TEST_F(SpdySessionSpdy2Test, ASyncRead) {

[Ryan Sleevi, 2013/02/06 23:04:46]

Naming: AsyncRead if it's asynchronous. It looks like you're saying "A
Sync[hronous] Read"

[ramant (doing other things), 2013/02/07 02:11:07]

Done.

+  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, DATA_FLAG_FIN));
+
+  scoped_ptr<SpdyFrame> resp1(ConstructSpdyGetSynReply(NULL, 0, 1));
+
+  // Send twice as many bytes as kMaxReadBytes.
+  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_frame1, 6, SYNCHRONOUS),
+    CreateMockRead(*data_frame1, 7, SYNCHRONOUS),
+    CreateMockRead(*data_frame1, 8, SYNCHRONOUS),
+    CreateMockRead(*data_frame, 9, SYNCHRONOUS),
+    MockRead(ASYNC, 0, 10)  // EOF
+  };
+
+  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);
+
+  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());
+
+  // Do 8 reads.
+  data.RunFor(8);
+  EXPECT_EQ(1u, observer.posted_count());
+  EXPECT_TRUE(data.at_write_eof());
+  EXPECT_TRUE(data.at_read_eof());
+}
+
 }  // namespace net