Slow start pipelining.

net/http/http_pipelined_host_impl_unittest.cc

Index: net/http/http_pipelined_host_impl_unittest.cc
diff --git a/net/http/http_pipelined_host_impl_unittest.cc b/net/http/http_pipelined_host_impl_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..36b67da2b0e5372fd00a739ecad0338fcb2156ce
--- /dev/null
+++ b/net/http/http_pipelined_host_impl_unittest.cc
@@ -0,0 +1,287 @@
+// 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 "net/http/http_pipelined_host_impl.h"
+
+#include "base/memory/scoped_ptr.h"
+#include "net/base/ssl_config_service.h"
+#include "net/http/http_pipelined_connection.h"
+#include "net/proxy/proxy_info.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using testing::_;
+using testing::NiceMock;
+using testing::Ref;
+using testing::Return;
+using testing::ReturnNull;
+
+static const int kMaxCapacity = 3;

[mmenke, 2011/11/28 23:05:28]

Think it's best to just use HttpPipelinedHostImpl::max_pipeline_depth() (Which
can be made static), so we don't have to duplicate this value all over the
place.

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

Done.

FYI, I expect max_pipeline_capacity() to become non-const eventually though.
It'll probably be a ways down the road, but the general idea is to tune it based
on the user's network connection to the server. For example, lots of tiny
requests to a slow server should prefer fewer, deeper pipelines. Otherwise, we
should generally prefer to avoid pipelining, because of front-of-queue blocking
and evictions.

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

Yea, figured that was the reason, but then you'll need to adjust the code here
to compensate, anyways.

+
+namespace net {
+
+static ClientSocketHandle* kDummyConnection =
+    reinterpret_cast<ClientSocketHandle*>(84);

[mmenke, 2011/11/28 23:05:28]

Can you use an anonymous namespace here, too?  And ditch the static.  Not that
there's anything wrong with static, but doing it to the whole file makes
conflicts less likely.  Same goes for your other tests.

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

Done.

+static HttpPipelinedStream* kDummyStream =
+    reinterpret_cast<HttpPipelinedStream*>(42);
+
+class MockHostDelegate : public HttpPipelinedHost::Delegate {
+ public:
+  MOCK_METHOD1(OnHostIdle, void(HttpPipelinedHost* host));
+  MOCK_METHOD1(OnHostHasAdditionalCapacity, void(HttpPipelinedHost* host));
+  MOCK_METHOD2(OnHostDeterminedCapability,
+               void(HttpPipelinedHost* host,
+                    HttpPipelinedHost::Capability capability));
+};
+
+class MockPipelineFactory : public HttpPipelinedConnection::Factory {
+ public:
+  MOCK_METHOD6(CreateNewPipeline, HttpPipelinedConnection*(
+      ClientSocketHandle* connection,
+      HttpPipelinedConnection::Delegate* delegate,
+      const SSLConfig& used_ssl_config,
+      const ProxyInfo& used_proxy_info,
+      const BoundNetLog& net_log,
+      bool was_npn_negotiated));
+};
+
+class MockPipeline : public HttpPipelinedConnection {
+ public:
+  MockPipeline(int depth, bool usable, bool active)
+      : depth_(depth),
+        usable_(usable),
+        active_(active) {
+  }
+
+  void SetState(int depth, bool usable, bool active) {
+    depth_ = depth;
+    usable_ = usable;
+    active_ = active;
+  }
+
+  virtual int depth() const OVERRIDE { return depth_; }
+  virtual bool usable() const OVERRIDE { return usable_; }
+  virtual bool active() const OVERRIDE { return active_; }
+
+  MOCK_METHOD0(CreateNewStream, HttpPipelinedStream*());
+  MOCK_METHOD1(OnStreamDeleted, void(int pipeline_id));
+  MOCK_CONST_METHOD0(used_ssl_config, const SSLConfig&());
+  MOCK_CONST_METHOD0(used_proxy_info, const ProxyInfo&());
+  MOCK_CONST_METHOD0(source, const NetLog::Source&());
+  MOCK_CONST_METHOD0(was_npn_negotiated, bool());
+
+ private:
+  int depth_;
+  bool usable_;
+  bool active_;
+};
+
+class HttpPipelinedHostImplTest : public testing::Test {
+ public:
+  HttpPipelinedHostImplTest()
+      : origin_("host", 123),
+        factory_(new MockPipelineFactory),  // Owned by host_.
+        host_(new HttpPipelinedHostImpl(&delegate_, origin_, factory_,
+                                        HttpPipelinedHost::CAPABLE)) {
+  }
+
+  void SetCapability(HttpPipelinedHost::Capability capability) {
+    factory_ = new MockPipelineFactory;
+    host_.reset(new HttpPipelinedHostImpl(
+        &delegate_, origin_, factory_, capability));
+  }
+
+  MockPipeline* AddTestPipeline(int depth, bool usable, bool active) {
+    MockPipeline* pipeline = new MockPipeline(depth, usable, active);
+    EXPECT_CALL(*factory_, CreateNewPipeline(kDummyConnection, host_.get(),
+                                             Ref(ssl_config_), Ref(proxy_info_),
+                                             Ref(net_log_), true))
+        .Times(1)
+        .WillOnce(Return(pipeline));
+    EXPECT_CALL(*pipeline, CreateNewStream())
+        .Times(1)
+        .WillOnce(Return(kDummyStream));
+    EXPECT_EQ(kDummyStream, host_->CreateStreamOnNewPipeline(
+        kDummyConnection, ssl_config_, proxy_info_, net_log_, true));
+    return pipeline;
+  }
+
+  void ClearTestPipeline(MockPipeline* pipeline) {
+    pipeline->SetState(0, true, true);
+    host_->OnPipelineHasCapacity(pipeline);
+  }
+
+  NiceMock<MockHostDelegate> delegate_;
+  HostPortPair origin_;
+  MockPipelineFactory* factory_;
+  scoped_ptr<HttpPipelinedHostImpl> host_;
+
+  SSLConfig ssl_config_;
+  ProxyInfo proxy_info_;
+  BoundNetLog net_log_;
+};
+
+TEST_F(HttpPipelinedHostImplTest, Delegate) {
+  EXPECT_TRUE(origin_.Equals(host_->origin()));
+}
+
+TEST_F(HttpPipelinedHostImplTest, OnHostIdle) {

[mmenke, 2011/11/28 23:05:28]

nit:  Maybe OnHostIdleUnusablePipeline?

Or maybe OnUnusablePipelineHasCapacity for this, and OnUsablePipelineHasCapacity
for the next one?

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

Done. Went with the latter.

+  MockPipeline* pipeline = AddTestPipeline(0, false, true);
+
+  EXPECT_CALL(delegate_, OnHostHasAdditionalCapacity(host_.get()))
+      .Times(0);
+  EXPECT_CALL(delegate_, OnHostIdle(host_.get()))
+      .Times(1);
+  host_->OnPipelineHasCapacity(pipeline);

[mmenke, 2011/11/28 23:05:28]

Might want to make sure |pipeline| gets deleted.  Could have factory keep track,
or check that |host_| has no pipelines, or use some other method.  This goes for
a bunch of other tests in this file, too.

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

I think we're already covered here. Valgrind will report any leaks. And
~HttpPipelinedHostImpl() will CHECK() fail if its pipelines_ map isn't empty.

+}
+
+TEST_F(HttpPipelinedHostImplTest, OnHostHasAdditionalCapacity) {
+  MockPipeline* pipeline = AddTestPipeline(1, true, true);
+
+  EXPECT_CALL(delegate_, OnHostHasAdditionalCapacity(host_.get()))
+      .Times(2);

[mmenke, 2011/11/28 23:05:28]

I think you should split this into two lines, one for each call.  Make for
clearer code.

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

Done.

+  EXPECT_CALL(delegate_, OnHostIdle(host_.get()))
+      .Times(0);
+
+  host_->OnPipelineHasCapacity(pipeline);
+
+  EXPECT_CALL(delegate_, OnHostIdle(host_.get()))
+      .Times(1);
+  ClearTestPipeline(pipeline);
+}
+
+TEST_F(HttpPipelinedHostImplTest, IgnoresUnusablePipeline) {
+  MockPipeline* pipeline = AddTestPipeline(1, false, true);
+
+  EXPECT_FALSE(host_->IsExistingPipelineAvailable());
+  EXPECT_EQ(NULL, host_->CreateStreamOnExistingPipeline());
+
+  ClearTestPipeline(pipeline);
+}
+
+TEST_F(HttpPipelinedHostImplTest, IgnoresInactivePipeline) {
+  MockPipeline* pipeline = AddTestPipeline(1, true, false);
+
+  EXPECT_FALSE(host_->IsExistingPipelineAvailable());
+  EXPECT_EQ(NULL, host_->CreateStreamOnExistingPipeline());
+
+  ClearTestPipeline(pipeline);
+}
+
+TEST_F(HttpPipelinedHostImplTest, IgnoresFullPipeline) {
+  MockPipeline* pipeline = AddTestPipeline(kMaxCapacity, true, true);
+
+  EXPECT_FALSE(host_->IsExistingPipelineAvailable());
+  EXPECT_EQ(NULL, host_->CreateStreamOnExistingPipeline());
+
+  ClearTestPipeline(pipeline);
+}
+
+TEST_F(HttpPipelinedHostImplTest, PicksLeastLoadedPipeline) {
+  MockPipeline* full_pipeline = AddTestPipeline(kMaxCapacity, true, true);
+  MockPipeline* usable_pipeline = AddTestPipeline(kMaxCapacity - 1, true, true);
+  MockPipeline* empty_pipeline = AddTestPipeline(0, true, true);
+
+  EXPECT_TRUE(host_->IsExistingPipelineAvailable());
+  EXPECT_CALL(*empty_pipeline, CreateNewStream())
+      .Times(1)
+      .WillOnce(ReturnNull());
+  EXPECT_EQ(NULL, host_->CreateStreamOnExistingPipeline());
+
+  ClearTestPipeline(full_pipeline);
+  ClearTestPipeline(usable_pipeline);
+  ClearTestPipeline(empty_pipeline);
+}
+
+TEST_F(HttpPipelinedHostImplTest, EmptyPipelineIsRemoved) {
+  MockPipeline* empty_pipeline = AddTestPipeline(0, true, true);
+
+  EXPECT_TRUE(host_->IsExistingPipelineAvailable());
+  EXPECT_CALL(*empty_pipeline, CreateNewStream())
+      .Times(1)
+      .WillOnce(Return(kDummyStream));
+  EXPECT_EQ(kDummyStream, host_->CreateStreamOnExistingPipeline());

[mmenke, 2011/11/28 23:05:28]

Is there really a need to create a dummy stream here?  I could see putting it in
another test, just to make sure the infrastructure is working, but seems kinda
odd in this test, since it doesn't have any effect on anything.

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

Hmm. I guess not. The whole test doesn't seem all that useful. It probably lost
its meaning weeks ago.

+
+  ClearTestPipeline(empty_pipeline);
+
+  EXPECT_FALSE(host_->IsExistingPipelineAvailable());
+  EXPECT_EQ(NULL, host_->CreateStreamOnExistingPipeline());
+}
+
+TEST_F(HttpPipelinedHostImplTest, OpensUpOnPipelineSuccess) {
+  SetCapability(HttpPipelinedHost::UNKNOWN);
+  MockPipeline* pipeline = AddTestPipeline(1, true, true);
+
+  EXPECT_EQ(NULL, host_->CreateStreamOnExistingPipeline());
+  EXPECT_CALL(delegate_, OnHostHasAdditionalCapacity(host_.get()))
+      .Times(1);
+  host_->OnPipelineFeedback(pipeline, HttpPipelinedConnection::OK);
+
+  EXPECT_CALL(*pipeline, CreateNewStream())
+      .Times(1)
+      .WillOnce(Return(kDummyStream));
+  EXPECT_EQ(kDummyStream, host_->CreateStreamOnExistingPipeline());

[mmenke, 2011/11/28 23:05:28]

You don't seem to have a test here that this call would fail before you call
OnPipelineFeedback.

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

I'm probably misunderstanding you. The 4th line of this function is supposed to
be the failure before OnPipelineFeedback.

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

You aren't misunderstanding me.  I just managed to miss that call repeatedly
during review, somehow.  Maybe it's the hypnotic green background.  Carry on.

+
+  EXPECT_CALL(delegate_, OnHostHasAdditionalCapacity(host_.get()))
+      .Times(1);
+  ClearTestPipeline(pipeline);
+}
+
+TEST_F(HttpPipelinedHostImplTest, ShutsDownOnOldVersion) {
+  SetCapability(HttpPipelinedHost::UNKNOWN);
+  MockPipeline* pipeline = AddTestPipeline(1, true, true);
+
+  EXPECT_EQ(NULL, host_->CreateStreamOnExistingPipeline());
+  EXPECT_CALL(delegate_, OnHostHasAdditionalCapacity(host_.get()))
+      .Times(0);
+  EXPECT_CALL(delegate_,
+              OnHostDeterminedCapability(host_.get(),
+                                         HttpPipelinedHost::INCAPABLE))
+      .Times(1);
+  host_->OnPipelineFeedback(pipeline,
+                            HttpPipelinedConnection::OLD_HTTP_VERSION);
+
+  ClearTestPipeline(pipeline);
+  EXPECT_EQ(NULL, host_->CreateStreamOnExistingPipeline());

[mmenke, 2011/11/28 23:05:28]

This doesn't seem to be testing anything.  ClearTestPipeline sets the depth of
the pipeline to 0 and calls OnPipelineHasCapacity, which deletes pipelines with
a depth of 0, so there's no existing pipeline to use, regardless of whether or
not we correctly prevent pipelining in response to OLD_HTTP_VERSION.

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

Done.

+}
+
+TEST_F(HttpPipelinedHostImplTest, ConnectionCloseHasNoEffect) {
+  SetCapability(HttpPipelinedHost::UNKNOWN);
+  MockPipeline* pipeline = AddTestPipeline(1, true, true);
+
+  EXPECT_CALL(delegate_, OnHostHasAdditionalCapacity(host_.get()))
+      .Times(0);
+  EXPECT_CALL(delegate_, OnHostDeterminedCapability(host_.get(), _))
+      .Times(0);
+  host_->OnPipelineFeedback(pipeline,
+                            HttpPipelinedConnection::MUST_CLOSE_CONNECTION);
+  EXPECT_EQ(NULL, host_->CreateStreamOnExistingPipeline());
+
+  EXPECT_CALL(delegate_, OnHostHasAdditionalCapacity(host_.get()))
+      .Times(1);
+  ClearTestPipeline(pipeline);
+}
+
+TEST_F(HttpPipelinedHostImplTest, SuccessesLeadToCapable) {
+  SetCapability(HttpPipelinedHost::UNKNOWN);
+  MockPipeline* pipeline = AddTestPipeline(1, true, true);
+
+  EXPECT_CALL(delegate_, OnHostHasAdditionalCapacity(host_.get()))
+      .Times(1);
+  EXPECT_CALL(delegate_,
+              OnHostDeterminedCapability(host_.get(),
+                                         HttpPipelinedHost::CAPABLE))
+      .Times(1);
+  host_->OnPipelineFeedback(pipeline, HttpPipelinedConnection::OK);
+
+  pipeline->SetState(3, true, true);
+  host_->OnPipelineFeedback(pipeline, HttpPipelinedConnection::OK);
+  host_->OnPipelineFeedback(pipeline, HttpPipelinedConnection::OK);
+
+  EXPECT_CALL(delegate_, OnHostHasAdditionalCapacity(host_.get()))
+      .Times(1);
+  ClearTestPipeline(pipeline);
+}
+
+}  // namespace net