Slow start pipelining.

net/http/http_pipelined_network_transaction_unittest.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <string>
 
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_vector.h"
 #include "base/stringprintf.h"
 #include "base/utf_string_conversions.h"
@@ skipping 79 matching lines @@
   void ExpectResponse(const std::string& expected,
                       HttpNetworkTransaction& transaction) {
     scoped_refptr<IOBuffer> buffer(new IOBuffer(expected.size()));
     EXPECT_EQ(static_cast<int>(expected.size()),
               transaction.Read(buffer.get(), expected.size(), &callback_));
     std::string actual(buffer->data(), expected.size());
     EXPECT_THAT(actual, StrEq(expected));
     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;
     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()));
 
     TestOldCompletionCallback one_read_callback;
     scoped_refptr<IOBuffer> buffer(new IOBuffer(8));
     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));
 
     EXPECT_EQ(OK, two_callback.WaitForResult());
     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_;
   std::vector<scoped_refptr<DeterministicSocketData> > data_vector_;
   TestOldCompletionCallback callback_;
   ScopedVector<HttpRequestInfo> request_info_vector_;
   bool default_pipelining_enabled_;
 
   scoped_ptr<ProxyService> proxy_service_;
   MockHostResolver mock_resolver_;
@@ skipping 40 matching lines @@
   MockRead reads[] = {
     MockRead(false, 1, "HTTP/1.1 200 OK\r\n"),
     MockRead(false, 2, "Content-Length: 8\r\n\r\n"),
     MockRead(true, 4, "one.html"),
     MockRead(false, 5, "HTTP/1.1 200 OK\r\n"),
     MockRead(false, 6, "Content-Length: 8\r\n\r\n"),
     MockRead(false, 7, "two.html"),
   };
   AddExpectedConnection(reads, arraysize(reads), writes, arraysize(writes));
 
-  CompleteTwoRequests();
+  CompleteTwoRequests(0, 2);
 }
 
 TEST_F(HttpPipelinedNetworkTransactionTest, ReusesOnSpaceAvailable) {
   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"

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

I think a comment that we're still pipelining here, despite the fact that each
send only completes after the previous file is received, because we
asynchronously remove deleted streams from the pipeline would make things
clearer.

               "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"),
   };
   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"),
   };
   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);
 }
 
 TEST_F(HttpPipelinedNetworkTransactionTest, UnknownSizeEvictsToNewPipeline) {
   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, 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));
 
   MockWrite writes2[] = {
     MockWrite(false, 0, "GET /two.html HTTP/1.1\r\n"
               "Host: localhost\r\n"
               "Connection: keep-alive\r\n\r\n"),
   };
   MockRead reads2[] = {
     MockRead(false, 1, "HTTP/1.1 200 OK\r\n"),
     MockRead(false, 2, "Content-Length: 8\r\n\r\n"),
     MockRead(false, 3, "two.html"),
   };
   AddExpectedConnection(reads2, arraysize(reads2), writes2, arraysize(writes2));
 
-  CompleteTwoRequests();
+  CompleteTwoRequests(0, 1);
 }
 
 TEST_F(HttpPipelinedNetworkTransactionTest, ConnectionCloseEvictToNewPipeline) {
   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, 3, "GET /two.html HTTP/1.1\r\n"
@@ skipping 13 matching lines @@
               "Host: localhost\r\n"
               "Connection: keep-alive\r\n\r\n"),
   };
   MockRead reads2[] = {
     MockRead(false, 1, "HTTP/1.1 200 OK\r\n"),
     MockRead(false, 2, "Content-Length: 8\r\n\r\n"),
     MockRead(false, 3, "two.html"),
   };
   AddExpectedConnection(reads2, arraysize(reads2), writes2, arraysize(writes2));
 
-  CompleteTwoRequests();
+  CompleteTwoRequests(0, 2);
 }
 
 TEST_F(HttpPipelinedNetworkTransactionTest, ErrorEvictsToNewPipeline) {
   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, 3, "GET /two.html HTTP/1.1\r\n"
@@ skipping 115 matching lines @@
       ERR_IO_PENDING,
       transaction.Start(GetRequestInfo("one.html"), &callback_, BoundNetLog()));
   EXPECT_EQ(OK, callback_.WaitForResult());
 
   AuthCredentials credentials(ASCIIToUTF16("user"), ASCIIToUTF16("pass"));
   EXPECT_EQ(OK, transaction.RestartWithAuth(credentials, &callback_));
 
   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