Slow start pipelining.

net/http/http_pipelined_host_pool_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 "net/http/http_pipelined_host_pool.h"
+
+#include "base/memory/scoped_ptr.h"
+#include "net/base/ssl_config_service.h"
+#include "net/http/http_pipelined_host.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::Ref;
+using testing::Return;
+using testing::ReturnNull;
+
+namespace net {
+
+namespace {
+
+ClientSocketHandle* kDummyConnection =
+    reinterpret_cast<ClientSocketHandle*>(188);
+HttpPipelinedStream* kDummyStream =
+    reinterpret_cast<HttpPipelinedStream*>(99);
+
+class MockPoolDelegate : public HttpPipelinedHostPool::Delegate {
+ public:
+  MOCK_METHOD1(OnHttpPipelinedHostHasAdditionalCapacity,
+               void(const HostPortPair& origin));
+};
+
+class MockHostFactory : public HttpPipelinedHost::Factory {
+ public:
+  MOCK_METHOD4(CreateNewHost, HttpPipelinedHost*(
+      HttpPipelinedHost::Delegate* delegate, const HostPortPair& origin,
+      HttpPipelinedConnection::Factory* factory,
+      HttpPipelinedHost::Capability capability));
+};
+
+class MockHost : public HttpPipelinedHost {
+ public:
+  MockHost(const HostPortPair& origin)
+      : origin_(origin) {
+  }
+
+  MOCK_METHOD5(CreateStreamOnNewPipeline, HttpPipelinedStream*(
+      ClientSocketHandle* connection,
+      const SSLConfig& used_ssl_config,
+      const ProxyInfo& used_proxy_info,
+      const BoundNetLog& net_log,
+      bool was_npn_negotiated));
+  MOCK_METHOD0(CreateStreamOnExistingPipeline, HttpPipelinedStream*());
+  MOCK_METHOD0(IsExistingPipelineAvailable, bool());
+
+  virtual const HostPortPair& origin() const OVERRIDE { return origin_; }
+
+ private:
+  HostPortPair origin_;
+};
+
+class HttpPipelinedHostPoolTest : public testing::Test {
+ public:
+  HttpPipelinedHostPoolTest()
+      : origin_("host", 123),
+        factory_(new MockHostFactory),  // Owned by pool_.
+        host_(new MockHost(origin_)),  // Owned by pool_.
+        pool_(new HttpPipelinedHostPool(&delegate_, factory_)),
+        was_npn_negotiated_(false) {
+  }
+
+  void CreateDummyStream() {
+    EXPECT_CALL(*host_, CreateStreamOnNewPipeline(kDummyConnection,
+                                                  Ref(ssl_config_),
+                                                  Ref(proxy_info_),
+                                                  Ref(net_log_),
+                                                  was_npn_negotiated_))
+        .Times(1)
+        .WillOnce(Return(kDummyStream));
+    EXPECT_EQ(kDummyStream,
+              pool_->CreateStreamOnNewPipeline(origin_, kDummyConnection,
+                                               ssl_config_, proxy_info_,
+                                               net_log_, was_npn_negotiated_));
+  }
+
+  HostPortPair origin_;
+  MockPoolDelegate delegate_;
+  MockHostFactory* factory_;
+  MockHost* host_;
+  scoped_ptr<HttpPipelinedHostPool> pool_;
+
+  const SSLConfig ssl_config_;
+  const ProxyInfo proxy_info_;
+  const BoundNetLog net_log_;
+  bool was_npn_negotiated_;
+};
+
+TEST_F(HttpPipelinedHostPoolTest, DefaultUnknown) {
+  EXPECT_TRUE(pool_->IsHostEligibleForPipelining(origin_));
+  EXPECT_CALL(*factory_, CreateNewHost(pool_.get(), Ref(origin_), _,
+                                       HttpPipelinedHost::UNKNOWN))
+      .Times(1)
+      .WillOnce(Return(host_));
+
+  CreateDummyStream();
+  pool_->OnHostIdle(host_);

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

Might want to make sure |host_| gets deleted.  This goes for a bunch of other
tests in this file, too.

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

As with the other test file, I think this will be covered by Valgrind.

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

There were one or two tests in particular that I was concerned about, but can't
remember which ones, and they look fine to me now.

That having been said, in general, I think if a significant part of a test
involves when/if something is deleted, depending on Valgrind is bad for a number
of reasons:

1) Long cycle time.
2) Can be hard to pin down the cause, particularly for a memory sheriff with
just a partial stack track containing mangled symbols.
3) In some cases, can be difficult even to figure out which test had the
failure.
4) Valgrind is a pain, requires developers to have it set up, isn't available on
windows, etc.

[James Simonsen, 2011/12/01 20:36:42]

Agreed.

We also have the CHECK() in the destructor to make sure the host map is empty
and we delete and remove from the map at the same time. Hopefully that keeps us
pretty well covered.

I've also seen GMock warn me if mock objects aren't deleted, but I'm not sure
how that works.

+}
+
+TEST_F(HttpPipelinedHostPoolTest, RemembersIncapable) {
+  EXPECT_CALL(*factory_, CreateNewHost(pool_.get(), Ref(origin_), _,
+                                       HttpPipelinedHost::UNKNOWN))
+      .Times(1)
+      .WillOnce(Return(host_));
+
+  CreateDummyStream();
+  pool_->OnHostDeterminedCapability(host_, HttpPipelinedHost::INCAPABLE);
+  pool_->OnHostIdle(host_);
+  EXPECT_FALSE(pool_->IsHostEligibleForPipelining(origin_));
+  EXPECT_CALL(*factory_, CreateNewHost(pool_.get(), Ref(origin_), _,
+                                       HttpPipelinedHost::INCAPABLE))
+      .Times(0);
+  EXPECT_EQ(NULL,
+            pool_->CreateStreamOnNewPipeline(origin_, kDummyConnection,
+                                             ssl_config_, proxy_info_, net_log_,
+                                             was_npn_negotiated_));
+}
+
+TEST_F(HttpPipelinedHostPoolTest, RemembersCapable) {
+  EXPECT_CALL(*factory_, CreateNewHost(pool_.get(), Ref(origin_), _,
+                                       HttpPipelinedHost::UNKNOWN))
+      .Times(1)
+      .WillOnce(Return(host_));
+
+  CreateDummyStream();
+  pool_->OnHostDeterminedCapability(host_, HttpPipelinedHost::CAPABLE);
+  pool_->OnHostIdle(host_);
+  EXPECT_TRUE(pool_->IsHostEligibleForPipelining(origin_));
+
+  host_ = new MockHost(origin_);
+  EXPECT_CALL(*factory_, CreateNewHost(pool_.get(), Ref(origin_), _,
+                                       HttpPipelinedHost::CAPABLE))
+      .Times(1)
+      .WillOnce(Return(host_));
+  CreateDummyStream();
+  pool_->OnHostIdle(host_);

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

nit:  The results would be the same on remembering nothing (or UNKNOWN).  Might
want to be a bit more picky, though not sure if that can be done easily.

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

The difference is that CAPABLE is passed in to CreateNewHost(). If it hadn't
remembered anything, UNKNOWN would've been passed in.

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

Ahh, right, good point.

+}
+
+TEST_F(HttpPipelinedHostPoolTest, IncapableIsSticky) {
+  EXPECT_CALL(*factory_, CreateNewHost(pool_.get(), Ref(origin_), _,
+                                       HttpPipelinedHost::UNKNOWN))
+      .Times(1)
+      .WillOnce(Return(host_));
+
+  CreateDummyStream();

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

Maybe do a CAPABLE up here, too?

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

Good idea. Done.

+  pool_->OnHostDeterminedCapability(host_, HttpPipelinedHost::INCAPABLE);
+  pool_->OnHostDeterminedCapability(host_, HttpPipelinedHost::CAPABLE);
+  pool_->OnHostIdle(host_);
+  EXPECT_FALSE(pool_->IsHostEligibleForPipelining(origin_));
+}
+

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

You might also want a test where we don't remember anything, just have a second
request for a pipeline on a host after destruction (And maybe before the call to
OnHostIdle, too).  We could live without them, but should be simple enough to
do.

+}  // anonymous namespace
+
+}  // namespace net