Keep pool of pre-connected DNS sockets

net/dns/dns_socket_pool.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_socket_pool.h"
+
+#include "base/logging.h"
+#include "base/rand_util.h"
+#include "net/base/ip_endpoint.h"
+#include "net/base/net_errors.h"
+#include "net/base/rand_callback.h"
+#include "net/socket/client_socket_factory.h"
+#include "net/udp/datagram_client_socket.h"
+
+namespace net {
+
+namespace {
+
+// On Windows, we can't request specific (random) ports, since that will
+// trigger firewall prompts, so request default ones, but keep a pile of
+// them.  Everywhere else, request fresh, random ports each time.
+#if defined(OS_WIN)
+const DatagramSocket::BindType kBindType = DatagramSocket::DEFAULT_BIND;
+const unsigned kInitialPoolSize = 256;

[szym, 2012/09/26 11:16:12]

Some comment explaining how these constants will be used would help.

[Deprecated (see juliatuttle), 2012/09/26 20:44:50]

Done.

+const unsigned kAllocateMinSize = 256;
+const unsigned kRetainMaxSize = 0;
+#else
+const DatagramSocket::BindType kBindType = DatagramSocket::RANDOM_BIND;
+const unsigned kInitialPoolSize = 0;
+const unsigned kAllocateMinSize = 0;
+const unsigned kRetainMaxSize = 0;
+#endif
+
+} // namespace
+
+DnsSocketPool::DnsSocketPool(ClientSocketFactory* socket_factory)
+    : socket_factory_(socket_factory),
+      net_log_(NULL),
+      nameservers_(NULL),
+      initialized_(false) {
+}
+
+void DnsSocketPool::InitializeInternal(
+    NetLog* net_log,
+    const std::vector<IPEndPoint>* nameservers) {
+  // net_log can be NULL if we don't want to log

[szym, 2012/09/26 11:16:12]

If you want to keep this comment put it in the declaration in the header.

[Deprecated (see juliatuttle), 2012/09/26 20:44:50]

Deleting it instead.

+  DCHECK(nameservers);
+
+  CHECK(!initialized_);

[szym, 2012/09/26 11:16:12]

A DCHECK would probably suffice for now.

[Deprecated (see juliatuttle), 2012/09/26 20:44:50]

Done.

+
+  net_log_ = net_log;
+  nameservers_ = nameservers;
+  initialized_ = true;
+}
+
+scoped_ptr<DatagramClientSocket> DnsSocketPool::CreateConnectedSocket(
+    unsigned server_index) {
+  DCHECK_LT(server_index, nameservers_->size());
+
+  scoped_ptr<DatagramClientSocket> socket;
+
+  NetLog::Source no_source;
+  socket.reset(socket_factory_->CreateDatagramClientSocket(
+      kBindType, base::Bind(&base::RandInt), net_log_, no_source));
+
+  if (socket.get()) {
+    int rv = socket->Connect((*nameservers_)[server_index]);
+    if (rv != OK) {
+      LOG(WARNING) << "Failed to connect socket: " << rv;
+      socket.reset();
+    }
+  } else {
+    LOG(WARNING) << "Failed to create socket.";
+  }
+
+  return socket.Pass();
+}
+
+class DefaultDnsSocketPool : public DnsSocketPool {
+ public:
+  DefaultDnsSocketPool(ClientSocketFactory* factory)
+     : DnsSocketPool(factory) {
+  };
+

[szym, 2012/09/26 11:16:12]

IMPORTANT: all the retained sockets are leaking out on destruction. Add a
destructor. Make it virtual.

[Deprecated (see juliatuttle), 2012/09/26 20:44:50]

Done.

+  virtual void Initialize(
+      NetLog* net_log,
+      const std::vector<IPEndPoint>* nameservers) OVERRIDE;
+
+  virtual scoped_ptr<DatagramClientSocket> AllocateSocket(
+      unsigned server_index) OVERRIDE;
+
+  virtual void FreeSocket(
+      unsigned server_index,
+      scoped_ptr<DatagramClientSocket> socket) OVERRIDE;
+
+ private:
+  std::vector<std::vector<DatagramClientSocket*> > pools_;

[szym, 2012/09/26 11:16:12]

vector<DatagramClientSocket*> is used 4 times. I suggest a typedef SocketVector.

[Deprecated (see juliatuttle), 2012/09/26 20:44:50]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(DefaultDnsSocketPool);
+};
+
+// static
+scoped_ptr<DnsSocketPool> DnsSocketPool::CreateDefault(
+    ClientSocketFactory* factory) {
+  return scoped_ptr<DnsSocketPool>(new DefaultDnsSocketPool(factory));
+}
+
+void DefaultDnsSocketPool::Initialize(
+    NetLog* net_log,
+    const std::vector<IPEndPoint>* nameservers) OVERRIDE {
+  DCHECK(pools_.empty());
+
+  InitializeInternal(net_log, nameservers);
+
+  const unsigned num_servers = nameservers->size();
+  pools_.resize(num_servers);
+  for (unsigned server_index = 0; server_index < num_servers; ++server_index) {
+    std::vector<DatagramClientSocket*>& pool = pools_[server_index];
+    for (unsigned pool_index = 0; pool_index < kInitialPoolSize; ++pool_index) {
+      DatagramClientSocket* socket =
+          CreateConnectedSocket(server_index).release();
+      if (!socket)
+        break;
+      pool.push_back(socket);
+    }
+  }
+}
+
+scoped_ptr<DatagramClientSocket> DefaultDnsSocketPool::AllocateSocket(
+    unsigned server_index) {
+  DCHECK_LT(server_index, pools_.size());
+  std::vector<DatagramClientSocket*>& pool = pools_[server_index];
+
+  // Allocate one socket more than the minimum, since we're about to remove one

[szym, 2012/09/26 11:16:12]

I'm guessing this is so that if kAllocateMinSize == 0, you still allocate one
socket to be used. If that's the reasoning, I suggest defining kAllocateMinSize
as the "minimum number of sockets to allocate from", and setting it to 1 on
!OS_WIN.

[Deprecated (see juliatuttle), 2012/09/26 20:44:50]

Done.

+  while (pool.size() < kAllocateMinSize + 1) {
+    DatagramClientSocket* socket =
+        CreateConnectedSocket(server_index).release();
+    if (!socket) {

[szym, 2012/09/26 11:16:12]

nit: no need for {} (at least for consistency with above)

[Deprecated (see juliatuttle), 2012/09/26 20:44:50]

Done.

+      break;
+    }
+    pool.push_back(socket);
+  }
+
+  if (pool.size() < kAllocateMinSize) {

[szym, 2012/09/26 11:16:12]

No +1 here, so a bit confused. If you removed +1, you wouldn't need extra
comments.

[Deprecated (see juliatuttle), 2012/09/26 20:44:50]

Done.

+    LOG(WARNING) << "Low DNS port entropy: wanted " << kAllocateMinSize
+                 << " sockets to choose from, but only have " << pool.size();
+  }
+
+  unsigned socket_index = base::RandInt(0, pool.size() - 1);
+  DatagramClientSocket* socket = pool[socket_index];
+  pool[socket_index] = pool.back();
+  pool.pop_back();
+
+  return scoped_ptr<DatagramClientSocket>(socket);
+}
+
+void DefaultDnsSocketPool::FreeSocket(
+    unsigned server_index,
+    scoped_ptr<DatagramClientSocket> socket) {
+  DCHECK_LT(server_index, pools_.size());
+  std::vector<DatagramClientSocket*>& pool = pools_[server_index];
+
+  if (pool.size() < kRetainMaxSize)
+    pool.push_back(socket.release());
+}
+
+} // namespace net