We basically don't do late socket binding when a connect has already

been started for a request, even if another socket frees up earlier.

The reassignment logic was quite complicated, so I reworked it.  Fixing
this bug was easy by changing the way FindTopStalledGroup worked, but
because that function is called in that loop, changing this case
caused the loop to go infinitely in some cases.  This led me to look
into unwinding the loop.

The problem really came down to ReleaseSocket/DoReleaseSocket.  Because
we allow for a pending queue of released sockets, we had to do this
looping (which has been a source of bugs before).  To fix, I
eliminated the pending_releases queue.  I also reworked the routes
through OnAvailableSocketSlot to unify them and always run asynchronously.

The result is that now we no longer have the loop.  So when one
socket is released, we hand out exactly one socket.  Note also that
this logic slightly changes the priority of how we recycle sockets.
Previously, we always consulted the TopStalledGroup.  The TopStalledGroup
is really only interesting in the case where we're at our max global
socket limit, which is rarely the case.  In the new logic, when a
socket is released, first priority goes to any pending socket in the
same group, regardless of that group's priority.  The reason is - why
close a socket we already have open?  Previously, if the released
socket's group was not the highest priority group, the socket would
be marked idle, then closed (to make space for a socket to the 
TopStalledGroup), and finally a new socket created.  I believe the
new algorithm, while not perfectly matching the priorities, is more
efficient (less churn on sockets), and also is more graceful to the
common case.

Finally OnAvailableSocketSlot does two things.  First, it tries to
"give" the now available slot to a particular group, which is dependent
on how OnAvailableSocketSlot was called.  If we're currently
stalled on max sockets, it will also check (after giving the socket
out) to see if we can somehow free something up to satisfy a
stalled group.  If that second step fails for whatever reason,
we don't loop.  In theory, this could mean that we go under the
socket max and didn't dish out some sockets right away.  To make
sure that multiple stalled groups can get unblocked, we'll record
the number of stalled groups, and once in this mode,
OnAvailableSocketSlot will keep checking for stalled groups until the
count finally drops to zero.


BUG=47375
TEST=DelayedSocketBindingWaitingForConnect,CancelStalledSocketAtSocketLimit

Review URL: http://codereview.chromium.org/2861023

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

1 files changed, 8 insertions, 6 deletions

diff --git a/net/socket/socks_client_socket_pool_unittest.cc b/net/socket/socks_client_socket_pool_unittest.cc
index fb83d96..7bfce55 100644
--- a/net/socket/socks_client_socket_pool_unittest.cc
+++ b/net/socket/socks_client_socket_pool_unittest.cc
@@ -319,15 +319,16 @@ TEST_F(SOCKSClientSocketPoolTest, CancelDuringTCPConnect) {
 
   pool_->CancelRequest("a", requests_[0]->handle());
   pool_->CancelRequest("a", requests_[1]->handle());
-  EXPECT_EQ(1, tcp_socket_pool_->cancel_count());
+  // Requests in the connect phase don't actually get cancelled.
+  EXPECT_EQ(0, tcp_socket_pool_->cancel_count());
 
   // Now wait for the TCP sockets to connect.
   MessageLoop::current()->RunAllPending();
 
   EXPECT_EQ(kRequestNotFound, GetOrderOfRequest(1));
   EXPECT_EQ(kRequestNotFound, GetOrderOfRequest(2));
-  EXPECT_EQ(1, tcp_socket_pool_->cancel_count());
-  EXPECT_EQ(1, pool_->IdleSocketCount());
+  EXPECT_EQ(0, tcp_socket_pool_->cancel_count());
+  EXPECT_EQ(2, pool_->IdleSocketCount());
 
   requests_[0]->handle()->Reset();
   requests_[1]->handle()->Reset();
@@ -354,7 +355,8 @@ TEST_F(SOCKSClientSocketPoolTest, CancelDuringSOCKSConnect) {
   pool_->CancelRequest("a", requests_[0]->handle());
   pool_->CancelRequest("a", requests_[1]->handle());
   EXPECT_EQ(0, tcp_socket_pool_->cancel_count());
-  EXPECT_EQ(1, tcp_socket_pool_->release_count());
+  // Requests in the connect phase don't actually get cancelled.
+  EXPECT_EQ(0, tcp_socket_pool_->release_count());
 
   // Now wait for the async data to reach the SOCKS connect jobs.
   MessageLoop::current()->RunAllPending();
@@ -362,8 +364,8 @@ TEST_F(SOCKSClientSocketPoolTest, CancelDuringSOCKSConnect) {
   EXPECT_EQ(kRequestNotFound, GetOrderOfRequest(1));
   EXPECT_EQ(kRequestNotFound, GetOrderOfRequest(2));
   EXPECT_EQ(0, tcp_socket_pool_->cancel_count());
-  EXPECT_EQ(1, tcp_socket_pool_->release_count());
-  EXPECT_EQ(1, pool_->IdleSocketCount());
+  EXPECT_EQ(0, tcp_socket_pool_->release_count());
+  EXPECT_EQ(2, pool_->IdleSocketCount());
 
   requests_[0]->handle()->Reset();
   requests_[1]->handle()->Reset();