Prioritize which HTTP requests get a socket first by adding a priority level to various methods and classes. Fix lint errors along the way.

R=darin,wtc
BUG=8993
Review URL: http://codereview.chromium.org/42541

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@12470 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 163 insertions, 89 deletions

diff --git a/net/base/client_socket_handle.cc b/net/base/client_socket_handle.cc
index ff81c0a..f5ab056 100644
--- a/net/base/client_socket_handle.cc
+++ b/net/base/client_socket_handle.cc
@@ -18,10 +18,11 @@ ClientSocketHandle::~ClientSocketHandle() {
 }
 
 int ClientSocketHandle::Init(const std::string& group_name,
+                             int priority,
                              CompletionCallback* callback) {
   Reset();
   group_name_ = group_name;
-  return pool_->RequestSocket(this, callback);
+  return pool_->RequestSocket(this, priority, callback);
 }
 
 void ClientSocketHandle::Reset() {
diff --git a/net/base/client_socket_handle.h b/net/base/client_socket_handle.h
index c107a16..e0d9e73 100644
--- a/net/base/client_socket_handle.h
+++ b/net/base/client_socket_handle.h
@@ -32,7 +32,8 @@ class ClientSocketHandle {
 
   // Initializes a ClientSocketHandle object, which involves talking to the
   // ClientSocketPool to locate a socket to possibly reuse.  This method
-  // returns either OK or ERR_IO_PENDING.
+  // returns either OK or ERR_IO_PENDING.  On ERR_IO_PENDING, |priority| is
+  // used to determine the placement in ClientSocketPool's wait list.
   //
   // If this method succeeds, then the socket member will be set to an existing
   // socket if an existing socket was available to reuse.  Otherwise, the
@@ -43,7 +44,9 @@ class ClientSocketHandle {
   //
   // Init may be called multiple times.
   //
-  int Init(const std::string& group_name, CompletionCallback* callback);
+  int Init(const std::string& group_name,
+           int priority,
+           CompletionCallback* callback);
 
   // An initialized handle can be reset, which causes it to return to the
   // un-initialized state.  This releases the underlying socket, which in the
diff --git a/net/base/client_socket_pool.cc b/net/base/client_socket_pool.cc
index 2092017..651fbe2 100644
--- a/net/base/client_socket_pool.cc
+++ b/net/base/client_socket_pool.cc
@@ -41,7 +41,21 @@ ClientSocketPool::~ClientSocketPool() {
   DCHECK(group_map_.empty());
 }
 
+// InsertRequestIntoQueue inserts the request into the queue based on
+// priority.  Highest priorities are closest to the front.  Older requests are
+// prioritized over requests of equal priority.
+//
+// static
+void ClientSocketPool::InsertRequestIntoQueue(const Request& r,
+                                              RequestQueue* pending_requests) {
+  RequestQueue::iterator it = pending_requests->begin();
+  while (it != pending_requests->end() && r.priority <= it->priority)
+    ++it;
+  pending_requests->insert(it, r);
+}
+
 int ClientSocketPool::RequestSocket(ClientSocketHandle* handle,
+                                    int priority,
                                     CompletionCallback* callback) {
   Group& group = group_map_[handle->group_name_];
 
@@ -51,7 +65,8 @@ int ClientSocketPool::RequestSocket(ClientSocketHandle* handle,
     r.handle = handle;
     DCHECK(callback);
     r.callback = callback;
-    group.pending_requests.push_back(r);
+    r.priority = priority;
+    InsertRequestIntoQueue(r, &group.pending_requests);
     return ERR_IO_PENDING;
   }
 
@@ -183,7 +198,7 @@ void ClientSocketPool::DoReleaseSocket(const std::string& group_name,
   if (!group.pending_requests.empty()) {
     Request r = group.pending_requests.front();
     group.pending_requests.pop_front();
-    int rv = RequestSocket(r.handle, NULL);
+    int rv = RequestSocket(r.handle, r.priority, NULL);
     DCHECK(rv == OK);
     r.callback->Run(rv);
     return;
diff --git a/net/base/client_socket_pool.h b/net/base/client_socket_pool.h
index a313a80..dd08c24 100644
--- a/net/base/client_socket_pool.h
+++ b/net/base/client_socket_pool.h
@@ -35,7 +35,8 @@ class ClientSocketPool : public base::RefCounted<ClientSocketPool> {
   // handle will be initialized without a socket such that the consumer needs
   // to supply a socket, or 3) the handle will be added to a wait list until a
   // socket is available to reuse or the opportunity to create a new socket
-  // arises.  The completion callback is notified in the 3rd case.
+  // arises.  The completion callback is notified in the 3rd case.  |priority|
+  // will determine the placement into the wait list.
   //
   // If this function returns OK, then |handle| is initialized upon return.
   // The |handle|'s is_initialized method will return true in this case.  If a
@@ -47,7 +48,9 @@ class ClientSocketPool : public base::RefCounted<ClientSocketPool> {
   // If ERR_IO_PENDING is returned, then the completion callback will be called
   // when |handle| has been initialized.
   //
-  int RequestSocket(ClientSocketHandle* handle, CompletionCallback* callback);
+  int RequestSocket(ClientSocketHandle* handle,
+                    int priority,
+                    CompletionCallback* callback);
 
   // Called to cancel a RequestSocket call that returned ERR_IO_PENDING.  The
   // same handle parameter must be passed to this method as was passed to the
@@ -75,30 +78,12 @@ class ClientSocketPool : public base::RefCounted<ClientSocketPool> {
 
   typedef scoped_ptr<ClientSocket> ClientSocketPtr;
 
-  ~ClientSocketPool();
-
-  // Closes all idle sockets if |force| is true.  Else, only closes idle
-  // sockets that timed out or can't be reused.
-  void CleanupIdleSockets(bool force);
-
-  // Called when the number of idle sockets changes.
-  void IncrementIdleCount();
-  void DecrementIdleCount();
-
-  // Called via PostTask by ReleaseSocket.
-  void DoReleaseSocket(const std::string& group_name, ClientSocketPtr* ptr);
-
-  // Called when timer_ fires.  This method scans the idle sockets removing
-  // sockets that timed out or can't be reused.
-  void OnCleanupTimerFired() {
-    CleanupIdleSockets(false);
-  }
-
   // A Request is allocated per call to RequestSocket that results in
   // ERR_IO_PENDING.
   struct Request {
     ClientSocketHandle* handle;
     CompletionCallback* callback;
+    int priority;
   };
 
   // Entry for a persistent socket which became idle at time |start_time|.
@@ -116,16 +101,41 @@ class ClientSocketPool : public base::RefCounted<ClientSocketPool> {
     bool ShouldCleanup(base::TimeTicks now) const;
   };
 
+  typedef std::deque<Request> RequestQueue;
+
   // A Group is allocated per group_name when there are idle sockets or pending
   // requests.  Otherwise, the Group object is removed from the map.
   struct Group {
     Group() : active_socket_count(0) {}
     std::deque<IdleSocket> idle_sockets;
-    std::deque<Request> pending_requests;
+    RequestQueue pending_requests;
     int active_socket_count;
   };
 
   typedef std::map<std::string, Group> GroupMap;
+
+  ~ClientSocketPool();
+
+  static void InsertRequestIntoQueue(const Request& r,
+                                     RequestQueue* pending_requests);
+
+  // Closes all idle sockets if |force| is true.  Else, only closes idle
+  // sockets that timed out or can't be reused.
+  void CleanupIdleSockets(bool force);
+
+  // Called when the number of idle sockets changes.
+  void IncrementIdleCount();
+  void DecrementIdleCount();
+
+  // Called via PostTask by ReleaseSocket.
+  void DoReleaseSocket(const std::string& group_name, ClientSocketPtr* ptr);
+
+  // Called when timer_ fires.  This method scans the idle sockets removing
+  // sockets that timed out or can't be reused.
+  void OnCleanupTimerFired() {
+    CleanupIdleSockets(false);
+  }
+
   GroupMap group_map_;
 
   // Timer used to periodically prune idle sockets that timed out or can't be
diff --git a/net/base/client_socket_pool_unittest.cc b/net/base/client_socket_pool_unittest.cc
index 3fb5fd4..8849781 100644
--- a/net/base/client_socket_pool_unittest.cc
+++ b/net/base/client_socket_pool_unittest.cc
@@ -11,10 +11,16 @@
 
 namespace {
 
-typedef testing::Test ClientSocketPoolTest;
-
 const int kMaxSocketsPerGroup = 6;
 
+// Note that the first and the last are the same, the first should be handled
+// before the last, since it was inserted first.
+const int kPriorities[10] = { 1, 7, 9, 5, 6, 2, 8, 3, 4, 1 };
+
+// This is the number of extra requests beyond the first few that use up all
+// available sockets in the socket group.
+const int kNumPendingRequests = arraysize(kPriorities);
+
 class MockClientSocket : public net::ClientSocket {
  public:
   MockClientSocket() : connected_(false) {
@@ -59,12 +65,16 @@ int MockClientSocket::allocation_count = 0;
 
 class TestSocketRequest : public CallbackRunner< Tuple1<int> > {
  public:
-  explicit TestSocketRequest(net::ClientSocketPool* pool) : handle(pool) {}
+  TestSocketRequest(
+      net::ClientSocketPool* pool,
+      std::vector<TestSocketRequest*>* request_order)
+      : handle(pool), request_order_(request_order) {}
 
   net::ClientSocketHandle handle;
 
   void EnsureSocket() {
     DCHECK(handle.is_initialized());
+    request_order_->push_back(this);
     if (!handle.socket()) {
       handle.set_socket(new MockClientSocket());
       handle.socket()->Connect(NULL);
@@ -78,20 +88,32 @@ class TestSocketRequest : public CallbackRunner< Tuple1<int> > {
   }
 
   static int completion_count;
+
+ private:
+  std::vector<TestSocketRequest*>* request_order_;
 };
 
 int TestSocketRequest::completion_count = 0;
 
-}  // namespace
+class ClientSocketPoolTest : public testing::Test {
+ protected:
+  ClientSocketPoolTest()
+      : pool_(new net::ClientSocketPool(kMaxSocketsPerGroup)) {}
 
-TEST(ClientSocketPoolTest, Basic) {
-  scoped_refptr<net::ClientSocketPool> pool =
-      new net::ClientSocketPool(kMaxSocketsPerGroup);
+  virtual void SetUp() {
+    MockClientSocket::allocation_count = 0;
+    TestSocketRequest::completion_count = 0;
+  }
+
+  scoped_refptr<net::ClientSocketPool> pool_;
+  std::vector<TestSocketRequest*> request_order_;
+};
 
-  TestSocketRequest r(pool);
+TEST_F(ClientSocketPoolTest, Basic) {
+  TestSocketRequest r(pool_.get(), &request_order_);
   int rv;
 
-  rv = r.handle.Init("a", &r);
+  rv = r.handle.Init("a", 0, &r);
   EXPECT_EQ(net::OK, rv);
   EXPECT_TRUE(r.handle.is_initialized());
 
@@ -101,14 +123,11 @@ TEST(ClientSocketPoolTest, Basic) {
   MessageLoop::current()->RunAllPending();
 }
 
-TEST(ClientSocketPoolTest, WithIdleConnection) {
-  scoped_refptr<net::ClientSocketPool> pool =
-      new net::ClientSocketPool(kMaxSocketsPerGroup);
-
-  TestSocketRequest r(pool);
+TEST_F(ClientSocketPoolTest, WithIdleConnection) {
+  TestSocketRequest r(pool_.get(), &request_order_);
   int rv;
 
-  rv = r.handle.Init("a", &r);
+  rv = r.handle.Init("a", 0, &r);
   EXPECT_EQ(net::OK, rv);
   EXPECT_TRUE(r.handle.is_initialized());
 
@@ -123,27 +142,24 @@ TEST(ClientSocketPoolTest, WithIdleConnection) {
   MessageLoop::current()->RunAllPending();
 }
 
-TEST(ClientSocketPoolTest, PendingRequests) {
-  scoped_refptr<net::ClientSocketPool> pool =
-      new net::ClientSocketPool(kMaxSocketsPerGroup);
-
+TEST_F(ClientSocketPoolTest, PendingRequests) {
   int rv;
 
-  scoped_ptr<TestSocketRequest> reqs[kMaxSocketsPerGroup + 10];
-  for (size_t i = 0; i < arraysize(reqs); ++i)
-    reqs[i].reset(new TestSocketRequest(pool));
+  scoped_ptr<TestSocketRequest> reqs[kMaxSocketsPerGroup + kNumPendingRequests];
 
-  // Reset
-  MockClientSocket::allocation_count = 0;
-  TestSocketRequest::completion_count = 0;
+  for (size_t i = 0; i < arraysize(reqs); ++i)
+    reqs[i].reset(new TestSocketRequest(pool_.get(), &request_order_));
 
   // Create connections or queue up requests.
-  for (size_t i = 0; i < arraysize(reqs); ++i) {
-    rv = reqs[i]->handle.Init("a", reqs[i].get());
-    if (rv != net::ERR_IO_PENDING) {
-      EXPECT_EQ(net::OK, rv);
-      reqs[i]->EnsureSocket();
-    }
+  for (int i = 0; i < kMaxSocketsPerGroup; ++i) {
+    rv = reqs[i]->handle.Init("a", 5, reqs[i].get());
+    EXPECT_EQ(net::OK, rv);
+    reqs[i]->EnsureSocket();
+  }
+  for (int i = 0; i < kNumPendingRequests; ++i) {
+    rv = reqs[kMaxSocketsPerGroup + i]->handle.Init(
+        "a", kPriorities[i], reqs[kMaxSocketsPerGroup + i].get());
+    EXPECT_EQ(net::ERR_IO_PENDING, rv);
   }
 
   // Release any connections until we have no connections.
@@ -160,26 +176,36 @@ TEST(ClientSocketPoolTest, PendingRequests) {
   } while (released_one);
 
   EXPECT_EQ(kMaxSocketsPerGroup, MockClientSocket::allocation_count);
-  EXPECT_EQ(10, TestSocketRequest::completion_count);
-}
+  EXPECT_EQ(kNumPendingRequests, TestSocketRequest::completion_count);
 
-TEST(ClientSocketPoolTest, PendingRequests_NoKeepAlive) {
-  scoped_refptr<net::ClientSocketPool> pool =
-      new net::ClientSocketPool(kMaxSocketsPerGroup);
+  for (int i = 0; i < kMaxSocketsPerGroup; ++i) {
+    EXPECT_EQ(request_order_[i], reqs[i].get()) <<
+        "Request " << i << " was not in order.";
+  }
 
+  for (int i = 0; i < kNumPendingRequests - 1; ++i) {
+    int index_in_queue = (kNumPendingRequests - 1) - kPriorities[i];
+    EXPECT_EQ(request_order_[kMaxSocketsPerGroup + index_in_queue],
+              reqs[kMaxSocketsPerGroup + i].get()) <<
+        "Request " << kMaxSocketsPerGroup + i << " was not in order.";
+  }
+
+  EXPECT_EQ(request_order_[arraysize(reqs) - 1],
+            reqs[arraysize(reqs) - 1].get()) <<
+      "The last request with priority 1 should not have been inserted "
+      "earlier into the queue.";
+}
+
+TEST_F(ClientSocketPoolTest, PendingRequests_NoKeepAlive) {
   int rv;
 
-  scoped_ptr<TestSocketRequest> reqs[kMaxSocketsPerGroup + 10];
+  scoped_ptr<TestSocketRequest> reqs[kMaxSocketsPerGroup + kNumPendingRequests];
   for (size_t i = 0; i < arraysize(reqs); ++i)
-    reqs[i].reset(new TestSocketRequest(pool));
-
-  // Reset
-  MockClientSocket::allocation_count = 0;
-  TestSocketRequest::completion_count = 0;
+    reqs[i].reset(new TestSocketRequest(pool_.get(), &request_order_));
 
   // Create connections or queue up requests.
   for (size_t i = 0; i < arraysize(reqs); ++i) {
-    rv = reqs[i]->handle.Init("a", reqs[i].get());
+    rv = reqs[i]->handle.Init("a", 0, reqs[i].get());
     if (rv != net::ERR_IO_PENDING) {
       EXPECT_EQ(net::OK, rv);
       reqs[i]->EnsureSocket();
@@ -200,31 +226,29 @@ TEST(ClientSocketPoolTest, PendingRequests_NoKeepAlive) {
     }
   } while (released_one);
 
-  EXPECT_EQ(kMaxSocketsPerGroup + 10, MockClientSocket::allocation_count);
-  EXPECT_EQ(10, TestSocketRequest::completion_count);
+  EXPECT_EQ(kMaxSocketsPerGroup + kNumPendingRequests,
+            MockClientSocket::allocation_count);
+  EXPECT_EQ(kNumPendingRequests, TestSocketRequest::completion_count);
 }
 
-TEST(ClientSocketPoolTest, CancelRequest) {
-  scoped_refptr<net::ClientSocketPool> pool =
-      new net::ClientSocketPool(kMaxSocketsPerGroup);
-
+TEST_F(ClientSocketPoolTest, CancelRequest) {
   int rv;
 
-  scoped_ptr<TestSocketRequest> reqs[kMaxSocketsPerGroup + 10];
-  for (size_t i = 0; i < arraysize(reqs); ++i)
-    reqs[i].reset(new TestSocketRequest(pool));
+  scoped_ptr<TestSocketRequest> reqs[kMaxSocketsPerGroup + kNumPendingRequests];
 
-  // Reset
-  MockClientSocket::allocation_count = 0;
-  TestSocketRequest::completion_count = 0;
+  for (size_t i = 0; i < arraysize(reqs); ++i)
+    reqs[i].reset(new TestSocketRequest(pool_.get(), &request_order_));
 
   // Create connections or queue up requests.
-  for (size_t i = 0; i < arraysize(reqs); ++i) {
-    rv = reqs[i]->handle.Init("a", reqs[i].get());
-    if (rv != net::ERR_IO_PENDING) {
-      EXPECT_EQ(net::OK, rv);
-      reqs[i]->EnsureSocket();
-    }
+  for (int i = 0; i < kMaxSocketsPerGroup; ++i) {
+    rv = reqs[i]->handle.Init("a", 5, reqs[i].get());
+    EXPECT_EQ(net::OK, rv);
+    reqs[i]->EnsureSocket();
+  }
+  for (int i = 0; i < kNumPendingRequests; ++i) {
+    rv = reqs[kMaxSocketsPerGroup + i]->handle.Init(
+        "a", kPriorities[i], reqs[kMaxSocketsPerGroup + i].get());
+    EXPECT_EQ(net::ERR_IO_PENDING, rv);
   }
 
   // Cancel a request.
@@ -246,5 +270,26 @@ TEST(ClientSocketPoolTest, CancelRequest) {
   } while (released_one);
 
   EXPECT_EQ(kMaxSocketsPerGroup, MockClientSocket::allocation_count);
-  EXPECT_EQ(9, TestSocketRequest::completion_count);
+  EXPECT_EQ(kNumPendingRequests - 1, TestSocketRequest::completion_count);
+  for (int i = 0; i < kMaxSocketsPerGroup; ++i) {
+    EXPECT_EQ(request_order_[i], reqs[i].get()) <<
+        "Request " << i << " was not in order.";
+  }
+
+  for (int i = 0; i < kNumPendingRequests - 1; ++i) {
+    if (i == 2) continue;
+    int index_in_queue = (kNumPendingRequests - 1) - kPriorities[i];
+    if (kPriorities[i] < kPriorities[index_to_cancel - kMaxSocketsPerGroup])
+      index_in_queue--;
+    EXPECT_EQ(request_order_[kMaxSocketsPerGroup + index_in_queue],
+              reqs[kMaxSocketsPerGroup + i].get()) <<
+        "Request " << kMaxSocketsPerGroup + i << " was not in order.";
+  }
+
+  EXPECT_EQ(request_order_[arraysize(reqs) - 2],
+            reqs[arraysize(reqs) - 1].get()) <<
+      "The last request with priority 1 should not have been inserted "
+      "earlier into the queue.";
 }
+
+}  // namespace