Slow start pipelining.

net/http/http_pipelined_network_transaction_unittest.cc

Index: net/http/http_pipelined_network_transaction_unittest.cc
diff --git a/net/http/http_pipelined_network_transaction_unittest.cc b/net/http/http_pipelined_network_transaction_unittest.cc
index 8eba31a30497644a3eeb0c37786e9e3ab4f5ab6d..c79598cdfdb4d1daa09c8c38c740d6cd46196f2d 100644
--- a/net/http/http_pipelined_network_transaction_unittest.cc
+++ b/net/http/http_pipelined_network_transaction_unittest.cc
@@ -97,7 +97,7 @@ class HttpPipelinedNetworkTransactionTest : public testing::Test {
     EXPECT_EQ(OK, transaction.Read(buffer.get(), expected.size(), &callback_));
   }
 
-  void CompleteTwoRequests() {
+  void CompleteTwoRequests(int data_index, int num_async) {

[mmenke, 2011/11/29 16:52:20]

|num_async|'s meaning is not at all clear here, since it's not actually the
number of reads/writes tagged as asynchronous, but rather the number of
reads/writes that are performed asynchronously, after we wait for the first
write to have been sent, so the count generally includes the second write, which
is synchronous, but is started asynchronously by
HttpStreamFactoryImpl::Job::OnStreamReadyCallback.

That description is too confusing and obscure.  Instead, I'd just call it
stop_step, and use the index of the async event.  Then call SetStop(stop_step)
and Run().

[James Simonsen, 2011/12/01 01:17:10]

Done. Good idea. I like that.

     scoped_ptr<HttpNetworkTransaction> one_transaction(
         new HttpNetworkTransaction(session_.get()));
     TestOldCompletionCallback one_callback;
@@ -117,9 +117,9 @@ class HttpPipelinedNetworkTransactionTest : public testing::Test {
     EXPECT_EQ(ERR_IO_PENDING,
               one_transaction->Read(buffer.get(), 8, &one_read_callback));
 
-    data_vector_[0]->RunFor(2);
+    data_vector_[data_index]->RunFor(num_async);
     EXPECT_EQ(8, one_read_callback.WaitForResult());
-    data_vector_[0]->SetStop(10);
+    data_vector_[data_index]->SetStop(10);
     std::string actual(buffer->data(), 8);
     EXPECT_THAT(actual, StrEq("one.html"));
     EXPECT_EQ(OK, one_transaction->Read(buffer.get(), 8, &one_read_callback));
@@ -128,6 +128,44 @@ class HttpPipelinedNetworkTransactionTest : public testing::Test {
     ExpectResponse("two.html", two_transaction);
   }
 
+  void CompleteFourRequests() {
+    scoped_ptr<HttpNetworkTransaction> one_transaction(
+        new HttpNetworkTransaction(session_.get()));
+    TestOldCompletionCallback one_callback;
+    EXPECT_EQ(ERR_IO_PENDING,
+              one_transaction->Start(GetRequestInfo("one.html"), &one_callback,
+                                     BoundNetLog()));
+    EXPECT_EQ(OK, one_callback.WaitForResult());
+
+    HttpNetworkTransaction two_transaction(session_.get());
+    TestOldCompletionCallback two_callback;
+    EXPECT_EQ(ERR_IO_PENDING,
+              two_transaction.Start(GetRequestInfo("two.html"), &two_callback,
+                                    BoundNetLog()));
+
+    HttpNetworkTransaction three_transaction(session_.get());
+    TestOldCompletionCallback three_callback;
+    EXPECT_EQ(ERR_IO_PENDING,
+              three_transaction.Start(GetRequestInfo("three.html"),
+                                      &three_callback, BoundNetLog()));
+
+    HttpNetworkTransaction four_transaction(session_.get());
+    TestOldCompletionCallback four_callback;
+    EXPECT_EQ(ERR_IO_PENDING,
+              four_transaction.Start(GetRequestInfo("four.html"),
+                                     &four_callback, BoundNetLog()));
+
+    ExpectResponse("one.html", *one_transaction.get());
+    EXPECT_EQ(OK, two_callback.WaitForResult());
+    ExpectResponse("two.html", two_transaction);
+    EXPECT_EQ(OK, three_callback.WaitForResult());
+    ExpectResponse("three.html", three_transaction);
+
+    one_transaction.reset();
+    EXPECT_EQ(OK, four_callback.WaitForResult());
+    ExpectResponse("four.html", four_transaction);
+  }
+
   DeterministicMockClientSocketFactory factory_;
   ClientSocketPoolHistograms histograms_;
   MockTransportClientSocketPool pool_;
@@ -188,7 +226,7 @@ TEST_F(HttpPipelinedNetworkTransactionTest, ReusePipeline) {
   };
   AddExpectedConnection(reads, arraysize(reads), writes, arraysize(writes));
 
-  CompleteTwoRequests();
+  CompleteTwoRequests(0, 2);
 }
 
 TEST_F(HttpPipelinedNetworkTransactionTest, ReusesOnSpaceAvailable) {
@@ -226,41 +264,7 @@ TEST_F(HttpPipelinedNetworkTransactionTest, ReusesOnSpaceAvailable) {
   };
   AddExpectedConnection(reads, arraysize(reads), writes, arraysize(writes));
 
-  scoped_ptr<HttpNetworkTransaction> one_transaction(
-      new HttpNetworkTransaction(session_.get()));
-  TestOldCompletionCallback one_callback;
-  EXPECT_EQ(ERR_IO_PENDING,
-            one_transaction->Start(GetRequestInfo("one.html"), &one_callback,
-                                   BoundNetLog()));
-  EXPECT_EQ(OK, one_callback.WaitForResult());
-
-  HttpNetworkTransaction two_transaction(session_.get());
-  TestOldCompletionCallback two_callback;
-  EXPECT_EQ(ERR_IO_PENDING,
-            two_transaction.Start(GetRequestInfo("two.html"), &two_callback,
-                                  BoundNetLog()));
-
-  HttpNetworkTransaction three_transaction(session_.get());
-  TestOldCompletionCallback three_callback;
-  EXPECT_EQ(ERR_IO_PENDING,
-            three_transaction.Start(GetRequestInfo("three.html"),
-                                    &three_callback, BoundNetLog()));
-
-  HttpNetworkTransaction four_transaction(session_.get());
-  TestOldCompletionCallback four_callback;
-  EXPECT_EQ(ERR_IO_PENDING,
-            four_transaction.Start(GetRequestInfo("four.html"), &four_callback,
-                                   BoundNetLog()));
-
-  ExpectResponse("one.html", *one_transaction.get());
-  EXPECT_EQ(OK, two_callback.WaitForResult());
-  ExpectResponse("two.html", two_transaction);
-  EXPECT_EQ(OK, three_callback.WaitForResult());
-  ExpectResponse("three.html", three_transaction);
-
-  one_transaction.reset();
-  EXPECT_EQ(OK, four_callback.WaitForResult());
-  ExpectResponse("four.html", four_transaction);
+  CompleteFourRequests();
 
   ClientSocketPoolManager::set_max_sockets_per_group(old_max_sockets);
 }
@@ -272,14 +276,11 @@ TEST_F(HttpPipelinedNetworkTransactionTest, UnknownSizeEvictsToNewPipeline) {
     MockWrite(false, 0, "GET /one.html HTTP/1.1\r\n"
               "Host: localhost\r\n"
               "Connection: keep-alive\r\n\r\n"),
-    MockWrite(false, 2, "GET /two.html HTTP/1.1\r\n"
-              "Host: localhost\r\n"
-              "Connection: keep-alive\r\n\r\n"),
   };
   MockRead reads[] = {
     MockRead(false, 1, "HTTP/1.1 200 OK\r\n\r\n"),
-    MockRead(true, 3, "one.html"),
-    MockRead(false, OK, 4),
+    MockRead(true, 2, "one.html"),
+    MockRead(false, OK, 3),
   };
   AddExpectedConnection(reads, arraysize(reads), writes, arraysize(writes));
 
@@ -295,7 +296,7 @@ TEST_F(HttpPipelinedNetworkTransactionTest, UnknownSizeEvictsToNewPipeline) {
   };
   AddExpectedConnection(reads2, arraysize(reads2), writes2, arraysize(writes2));
 
-  CompleteTwoRequests();
+  CompleteTwoRequests(0, 1);
 }
 
 TEST_F(HttpPipelinedNetworkTransactionTest, ConnectionCloseEvictToNewPipeline) {
@@ -329,7 +330,7 @@ TEST_F(HttpPipelinedNetworkTransactionTest, ConnectionCloseEvictToNewPipeline) {
   };
   AddExpectedConnection(reads2, arraysize(reads2), writes2, arraysize(writes2));
 
-  CompleteTwoRequests();
+  CompleteTwoRequests(0, 2);
 }
 
 TEST_F(HttpPipelinedNetworkTransactionTest, ErrorEvictsToNewPipeline) {
@@ -465,6 +466,130 @@ TEST_F(HttpPipelinedNetworkTransactionTest, BasicHttpAuthentication) {
   ExpectResponse("one.html", transaction);
 }
 
+TEST_F(HttpPipelinedNetworkTransactionTest, OldVersionDisablesPipelining) {

[mmenke, 2011/11/29 16:52:20]

I suggest making the second and third responses HTTP/1.1, to make sure it
disables pipelining on other transactions, rather than on just the current
HTTP/1.0 transaction.

[mmenke, 2011/11/29 16:52:20]

I also think it would be good to have a "NoLengthDoesNotDisablePipelining" one,
which sends one with no length, and then makes sure that we still pipeline
afterwards.  Also maybe one that errors out.  Once you have the infrastructure,
adding these is pretty simple, and these tests are fast, so no reason not to
over test.

[James Simonsen, 2011/12/01 01:17:10]

Done.

[James Simonsen, 2011/12/01 01:17:10]

I'm actually not crazy about adding a ton of tests here. I prefer to test at the
lower levels if at all possible. I'd like to leave this file for integration
tests, where several components all have to function correctly together. I think
the tests that are already here ensure that the proper signals are connected to
each other (i.e. CAPABLE and INCAPABLE are learned, remembered, and honored).

For the cases you mention, I'd rather just test at the lower levels and make
sure that unknown size and errors send the correct signals back to the
HttpPipelinedHost. I think we already have these.

Feel free to push back. Like you said, it's not hard to add them, but I'd like
to keep the test code manageable and minimize overlap.

[mmenke, 2011/12/01 02:17:31]

I think that's reasonable.  I just get a bit paranoid as the complexity of the
underlying code increases.

+  Initialize();
+
+  MockWrite writes[] = {
+    MockWrite(false, 0, "GET /pipelined.html HTTP/1.1\r\n"
+              "Host: localhost\r\n"
+              "Connection: keep-alive\r\n\r\n"),
+  };
+  MockRead reads[] = {
+    MockRead(false, 1, "HTTP/1.0 200 OK\r\n"),
+    MockRead(false, 2, "Content-Length: 14\r\n\r\n"),
+    MockRead(false, 3, "pipelined.html"),
+  };
+  AddExpectedConnection(reads, arraysize(reads), writes, arraysize(writes));
+
+  MockWrite writes2[] = {
+    MockWrite(false, 0, "GET /one.html HTTP/1.1\r\n"
+              "Host: localhost\r\n"
+              "Connection: keep-alive\r\n\r\n"),
+  };
+  MockRead reads2[] = {
+    MockRead(false, 1, "HTTP/1.0 200 OK\r\n"),
+    MockRead(false, 2, "Content-Length: 8\r\n\r\n"),
+    MockRead(true, 3, "one.html"),
+    MockRead(false, OK, 4),
+  };
+  AddExpectedConnection(reads2, arraysize(reads2), writes2, arraysize(writes2));
+
+  MockWrite writes3[] = {
+    MockWrite(false, 0, "GET /two.html HTTP/1.1\r\n"
+              "Host: localhost\r\n"
+              "Connection: keep-alive\r\n\r\n"),
+  };
+  MockRead reads3[] = {
+    MockRead(false, 1, "HTTP/1.0 200 OK\r\n"),
+    MockRead(false, 2, "Content-Length: 8\r\n\r\n"),
+    MockRead(false, 3, "two.html"),
+    MockRead(false, OK, 4),
+  };
+  AddExpectedConnection(reads3, arraysize(reads3), writes3, arraysize(writes3));
+
+  HttpNetworkTransaction one_transaction(session_.get());
+  TestOldCompletionCallback one_callback;
+  EXPECT_EQ(ERR_IO_PENDING,
+            one_transaction.Start(GetRequestInfo("pipelined.html"),
+                                  &one_callback, BoundNetLog()));
+  EXPECT_EQ(OK, one_callback.WaitForResult());
+  ExpectResponse("pipelined.html", one_transaction);
+
+  CompleteTwoRequests(1, 1);
+}
+
+TEST_F(HttpPipelinedNetworkTransactionTest, PipelinesImmediatelyIfKnownGood) {

[mmenke, 2011/11/29 16:52:20]

The first 4 transactions are just to label the pipeline as good, and then the
other two are to make sure pipelining is instantly set up on a newly created
pipelined connection, after deleting the first, which is why both sends are
before all the reads?  I think this is sufficiently non-obvious that a comment
about this would be useful.

I also think "five" and "six" should be renamed to "new-pipeline-one" and
"new-pipeline-two" or some such, to make it a little clearer where they fit into
things.

[James Simonsen, 2011/12/01 01:17:10]

Done.

+  int old_max_sockets = ClientSocketPoolManager::max_sockets_per_group();
+  ClientSocketPoolManager::set_max_sockets_per_group(1);
+  Initialize();
+
+  MockWrite writes[] = {
+    MockWrite(false, 0, "GET /one.html HTTP/1.1\r\n"
+              "Host: localhost\r\n"
+              "Connection: keep-alive\r\n\r\n"),
+    MockWrite(false, 4, "GET /two.html HTTP/1.1\r\n"
+              "Host: localhost\r\n"
+              "Connection: keep-alive\r\n\r\n"),
+    MockWrite(false, 7, "GET /three.html HTTP/1.1\r\n"
+              "Host: localhost\r\n"
+              "Connection: keep-alive\r\n\r\n"),
+    MockWrite(false, 12, "GET /four.html HTTP/1.1\r\n"
+              "Host: localhost\r\n"
+              "Connection: keep-alive\r\n\r\n"),
+    MockWrite(false, 16, "GET /five.html HTTP/1.1\r\n"
+              "Host: localhost\r\n"
+              "Connection: keep-alive\r\n\r\n"),
+    MockWrite(false, 17, "GET /six.html HTTP/1.1\r\n"
+              "Host: localhost\r\n"
+              "Connection: keep-alive\r\n\r\n"),
+  };
+  MockRead reads[] = {
+    MockRead(false, 1, "HTTP/1.1 200 OK\r\n"),
+    MockRead(false, 2, "Content-Length: 8\r\n\r\n"),
+    MockRead(false, 3, "one.html"),
+    MockRead(false, 5, "HTTP/1.1 200 OK\r\n"),
+    MockRead(false, 6, "Content-Length: 8\r\n\r\n"),
+    MockRead(false, 8, "two.html"),
+    MockRead(false, 9, "HTTP/1.1 200 OK\r\n"),
+    MockRead(false, 10, "Content-Length: 10\r\n\r\n"),
+    MockRead(false, 11, "three.html"),
+    MockRead(false, 13, "HTTP/1.1 200 OK\r\n"),
+    MockRead(false, 14, "Content-Length: 9\r\n\r\n"),
+    MockRead(false, 15, "four.html"),
+    MockRead(true, 18, "HTTP/1.1 200 OK\r\n"),
+    MockRead(true, 19, "Content-Length: 9\r\n\r\n"),
+    MockRead(false, 20, "five.html"),
+    MockRead(false, 21, "HTTP/1.1 200 OK\r\n"),
+    MockRead(false, 22, "Content-Length: 8\r\n\r\n"),
+    MockRead(false, 23, "six.html"),
+  };
+  AddExpectedConnection(reads, arraysize(reads), writes, arraysize(writes));
+
+  CompleteFourRequests();
+
+  HttpNetworkTransaction five_transaction(session_.get());
+  TestOldCompletionCallback five_callback;
+  EXPECT_EQ(ERR_IO_PENDING,
+            five_transaction.Start(GetRequestInfo("five.html"), &five_callback,
+                                   BoundNetLog()));
+  MessageLoop::current()->RunAllPending();
+
+  HttpNetworkTransaction six_transaction(session_.get());
+  TestOldCompletionCallback six_callback;
+  EXPECT_EQ(ERR_IO_PENDING,
+            six_transaction.Start(GetRequestInfo("six.html"), &six_callback,
+                                  BoundNetLog()));
+
+  data_vector_[0]->RunFor(3);
+  EXPECT_EQ(OK, five_callback.WaitForResult());
+  data_vector_[0]->StopAfter(100);
+  ExpectResponse("five.html", five_transaction);
+  EXPECT_EQ(OK, six_callback.WaitForResult());
+  ExpectResponse("six.html", six_transaction);
+
+  ClientSocketPoolManager::set_max_sockets_per_group(old_max_sockets);
+}
+
 }  // anonymous namespace
 
 }  // namespace net