Keep pool of pre-connected DNS sockets

net/dns/dns_session_unittest.cc

+// Copyright (c) 2012 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/dns/dns_session.h"
+
+#include "base/bind.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/rand_util.h"
+#include "net/base/net_log.h"
+#include "net/dns/dns_protocol.h"
+#include "net/dns/dns_socket_pool.h"
+#include "net/socket/socket_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace net {
+
+namespace {
+
+class TestClientSocketFactory : public MockClientSocketFactory {

[szym, 2012/09/14 20:49:27]

Why are you deriving from MockClientSocketFactory? If you aren't really using
it, derive from ClientSocketFactory and add NOTIMPLEMENTED() to the calls you
don't want to implement.

Alternatively use MockClientSocketFactory as is (just beware that it does not
own SocketDataProviders either!)

[Deprecated (see juliatuttle), 2012/09/18 20:28:00]

Done.

+ public:
+  virtual DatagramClientSocket* CreateDatagramClientSocket(

[szym, 2012/09/14 20:49:27]

You will need to add a virtual destructor for this to compile on clang.

[Deprecated (see juliatuttle), 2012/09/18 20:28:00]

Done.

+      DatagramSocket::BindType bind_type,
+      const RandIntCallback& rand_int_cb,
+      net::NetLog* net_log,
+      const net::NetLog::Source& source) OVERRIDE;
+};
+
+struct PoolEvent {
+  enum { ALLOCATE, FREE } action_;

[szym, 2012/09/14 20:49:27]

nit: public fields should have no "_" suffix.

[Deprecated (see juliatuttle), 2012/09/18 20:28:00]

Done.

+  unsigned server_index_;
+};
+
+class DnsSessionTest : public testing::Test {
+ public:
+  void OnSocketAllocated(unsigned server_index);
+  void OnSocketFreed(unsigned server_index);
+
+ protected:
+  void Initialize(unsigned char num_servers);

[szym, 2012/09/14 20:49:27]

unsigned char? make it unsigned (int). If you want to ensure it's small add a
comment and a DCHECK in implementation.

[Deprecated (see juliatuttle), 2012/09/18 20:28:00]

Done.

+  scoped_ptr<DnsSession::SocketLease> Allocate(unsigned server_index);
+  bool Allocated(unsigned server_index);

[szym, 2012/09/14 20:49:27]

I'm ok with Allocated, but perhaps "DidAllocate" is more explicit.

[Deprecated (see juliatuttle), 2012/09/18 20:28:00]

Done.

+  bool Freed(unsigned server_index);
+  bool Ready();
+
+  DnsConfig config_;
+  scoped_ptr<TestClientSocketFactory> test_client_socket_factory_;
+  scoped_refptr<DnsSession> session_;
+  NetLog::Source source_;
+
+ private:
+  bool ExpectEvent(const PoolEvent& event);

[szym, 2012/09/14 20:49:27]

suggest: "ReceivedEvent" or "EventHappened" etc.

[Deprecated (see juliatuttle), 2012/09/18 20:28:00]

The problem with those is that either could be used for "we just got the event"
in addition to "we're checking if that event happened".  What about something
like PopEvent?

+  std::list<PoolEvent> events_;
+};
+
+class NullDnsSocketPool : public DnsSocketPool {
+ public:
+  NullDnsSocketPool(DnsSessionTest* test) : test_(test) { }

[szym, 2012/09/14 20:49:27]

need virtual destructor

[Deprecated (see juliatuttle), 2012/09/18 20:28:00]

Done.

+
+  virtual scoped_ptr<DatagramClientSocket> AllocateSocket(
+      unsigned server_index) OVERRIDE {
+    test_->OnSocketAllocated(server_index);
+    return CreateConnectedSocket(server_index).Pass();
+  }
+
+  virtual void FreeSocket(
+      unsigned server_index,
+      scoped_ptr<DatagramClientSocket> socket) OVERRIDE {
+    test_->OnSocketFreed(server_index);
+  }
+
+ private:
+  DnsSessionTest* test_;
+};
+
+void DnsSessionTest::Initialize(unsigned char num_servers) {
+  config_.nameservers.clear();
+  IPAddressNumber dns_ip;
+  bool rv = ParseIPLiteralToNumber("192.168.1.0", &dns_ip);
+  EXPECT_TRUE(rv);
+  for (unsigned char i = 0; i < num_servers; i++) {

[szym, 2012/09/14 20:49:27]

nit: ++i

[Deprecated (see juliatuttle), 2012/09/18 20:28:00]

Done.

+    dns_ip[3] = i;
+    IPEndPoint dns_endpoint(dns_ip, dns_protocol::kDefaultPort);
+    config_.nameservers.push_back(dns_endpoint);
+  }
+
+  test_client_socket_factory_.reset(new TestClientSocketFactory());
+  ClientSocketFactory* client_socket_factory =
+      static_cast<ClientSocketFactory*>(test_client_socket_factory_.get());
+
+  DnsSocketPool* dns_socket_pool =
+      static_cast<DnsSocketPool*>(new NullDnsSocketPool(this));

[szym, 2012/09/14 20:49:27]

No need for static_cast to base class.

[Deprecated (see juliatuttle), 2012/09/18 20:28:00]

Done.

+
+  session_ = new DnsSession(config_,
+                            client_socket_factory,
+                            scoped_ptr<DnsSocketPool>(dns_socket_pool),
+                            base::Bind(&base::RandInt),
+                            NULL /* NetLog */);
+
+  events_.clear();
+}
+
+scoped_ptr<DnsSession::SocketLease> DnsSessionTest::Allocate(
+    unsigned server_index) {
+  return session_->AllocateSocket(server_index, source_);
+}
+
+bool DnsSessionTest::Allocated(unsigned server_index) {
+  PoolEvent expected_event = { PoolEvent::ALLOCATE, server_index };
+  return ExpectEvent(expected_event);
+}
+
+bool DnsSessionTest::Freed(unsigned server_index) {
+  PoolEvent expected_event = { PoolEvent::FREE, server_index };
+  return ExpectEvent(expected_event);
+}
+
+bool DnsSessionTest::Ready() {

[szym, 2012/09/14 20:49:27]

suggest: "NoMoreEvents"

[Deprecated (see juliatuttle), 2012/09/18 20:28:00]

Done.

+  return events_.empty();
+}
+
+void DnsSessionTest::OnSocketAllocated(unsigned server_index) {
+  PoolEvent event = { PoolEvent::ALLOCATE, server_index };
+  events_.push_back(event);
+}
+
+void DnsSessionTest::OnSocketFreed(unsigned server_index) {
+  PoolEvent event = { PoolEvent::FREE, server_index };
+  events_.push_back(event);
+}
+
+bool DnsSessionTest::ExpectEvent(const PoolEvent& expected) {
+  if (events_.empty()) {
+    return false;
+  }
+
+  const PoolEvent actual = events_.front();
+  if ((expected.action_ != actual.action_)
+      || (expected.server_index_ != actual.server_index_)) {
+    return false;
+  }
+  events_.pop_front();
+
+  return true;
+}
+
+DatagramClientSocket* TestClientSocketFactory::CreateDatagramClientSocket(
+    DatagramSocket::BindType bind_type,
+    const RandIntCallback& rand_int_cb,
+    net::NetLog* net_log,
+    const net::NetLog::Source& source) {
+  // We're not actually expecting to send or receive any data, so use the
+  // simplest SocketDataProvider with no data supplied.
+  SocketDataProvider* data_provider = new StaticSocketDataProvider();

[szym, 2012/09/14 20:49:27]

You are leaking |data_provider|. MockUDPClientSocket won't own it.

[Deprecated (see juliatuttle), 2012/09/18 20:28:00]

Done.

+  MockUDPClientSocket* socket = new MockUDPClientSocket(data_provider, net_log);
+  data_provider->set_socket(socket);
+  return socket;
+}
+
+/*
+ * TODO(ttuttle):
+ * Mock out SocketPools and make sure the pools get the right calls.
+ * Test the default SocketPool implementation.
+ *   ...while in DnsSession or separately?

[szym, 2012/09/14 20:49:27]

Since you moved DnsSocketPool implementation to dns_socket_pool.cc, I suggest
testing it in dns_socket_pool_unittest.cc

+ */
+
+TEST_F(DnsSessionTest, AllocateFree) {
+  scoped_ptr<DnsSession::SocketLease> lease1, lease2;
+
+  Initialize(2);
+  EXPECT_TRUE(Ready());
+
+  lease1 = Allocate(0);
+  EXPECT_TRUE(Allocated(0));
+  EXPECT_TRUE(Ready());
+
+  lease2 = Allocate(1);
+  EXPECT_TRUE(Allocated(1));
+  EXPECT_TRUE(Ready());
+
+  lease1.reset();
+  EXPECT_TRUE(Freed(0));
+  EXPECT_TRUE(Ready());
+
+  lease2.reset();
+  EXPECT_TRUE(Freed(1));
+  EXPECT_TRUE(Ready());
+}
+
+}

[szym, 2012/09/14 20:49:27]

nit: // namespace

[Deprecated (see juliatuttle), 2012/09/18 20:28:00]

Done.

+
+}

[szym, 2012/09/14 20:49:27]

nit: // namespace net

[Deprecated (see juliatuttle), 2012/09/18 20:28:00]

Done.