Add timeouts for ConnectJobs. Limit ConnectJobs per group to number of Requests per group + 1.

In the histograms for Net.SocketRequestTime, late binding has a much longer tail.  This is presumably because I didn't implement timeouts and CancelRequest() never cancelled jobs.  I'm limiting TCPConnectJobs to 30 seconds and cancelling jobs if there are too many more than there are requests.

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

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

5 files changed, 18 insertions, 154 deletions

diff --git a/net/socket/client_socket_pool_base.cc b/net/socket/client_socket_pool_base.cc
index 00b9753..db3858d 100644
--- a/net/socket/client_socket_pool_base.cc
+++ b/net/socket/client_socket_pool_base.cc
@@ -34,11 +34,9 @@ bool ClientSocketPoolBase::g_late_binding = false;
 
 ConnectJob::ConnectJob(const std::string& group_name,
                        const ClientSocketHandle* key_handle,
-                       base::TimeDelta timeout_duration,
                        Delegate* delegate)
     : group_name_(group_name),
       key_handle_(key_handle),
-      timeout_duration_(timeout_duration),
       delegate_(delegate),
       load_state_(LOAD_STATE_IDLE) {
   DCHECK(!group_name.empty());
@@ -48,19 +46,6 @@ ConnectJob::ConnectJob(const std::string& group_name,
 
 ConnectJob::~ConnectJob() {}
 
-int ConnectJob::Connect() {
-  if (timeout_duration_ != base::TimeDelta())
-    timer_.Start(timeout_duration_, this, &ConnectJob::OnTimeout);
-  return ConnectInternal();
-}
-
-void ConnectJob::OnTimeout() {
-  // The delegate will delete |this|.
-  Delegate *delegate = delegate_;
-  delegate_ = NULL;
-  delegate->OnConnectJobComplete(ERR_TIMED_OUT, this);
-}
-
 ClientSocketPoolBase::ClientSocketPoolBase(
     int max_sockets,
     int max_sockets_per_group,
@@ -179,12 +164,6 @@ void ClientSocketPoolBase::CancelRequest(const std::string& group_name,
   for (; it != group.pending_requests.end(); ++it) {
     if (it->handle == handle) {
       group.pending_requests.erase(it);
-      if (g_late_binding &&
-          group.jobs.size() > group.pending_requests.size() + 1) {
-        // TODO(willchan): Cancel the job in the earliest LoadState.
-        RemoveConnectJob(handle, *group.jobs.begin(), &group);
-        OnAvailableSocketSlot(group_name, &group);
-      }
       return;
     }
   }
@@ -438,7 +417,7 @@ void ClientSocketPoolBase::EnableLateBindingOfSockets(bool enabled) {
 }
 
 void ClientSocketPoolBase::RemoveConnectJob(
-    const ClientSocketHandle* handle, const ConnectJob *job, Group* group) {
+    const ClientSocketHandle* handle, ConnectJob *job, Group* group) {
   CHECK(connecting_socket_count_ > 0);
   connecting_socket_count_--;
 
diff --git a/net/socket/client_socket_pool_base.h b/net/socket/client_socket_pool_base.h
index 51b2139..21cd642 100644
--- a/net/socket/client_socket_pool_base.h
+++ b/net/socket/client_socket_pool_base.h
@@ -43,10 +43,8 @@ class ConnectJob {
     DISALLOW_COPY_AND_ASSIGN(Delegate);
   };
 
-  // A |timeout_duration| of 0 corresponds to no timeout.
   ConnectJob(const std::string& group_name,
              const ClientSocketHandle* key_handle,
-             base::TimeDelta timeout_duration,
              Delegate* delegate);
   virtual ~ConnectJob();
 
@@ -65,7 +63,7 @@ class ConnectJob {
   // |delegate_| via OnConnectJobComplete.  In both asynchronous and synchronous
   // completion, ReleaseSocket() can be called to acquire the connected socket
   // if it succeeded.
-  int Connect();
+  virtual int Connect() = 0;
 
  protected:
   void set_load_state(LoadState load_state) { load_state_ = load_state; }
@@ -74,18 +72,10 @@ class ConnectJob {
   Delegate* delegate() { return delegate_; }
 
  private:
-  virtual int ConnectInternal() = 0;
-
-  // Alerts the delegate that the ConnectJob has timed out.
-  void OnTimeout();
-
   const std::string group_name_;
   // Temporarily needed until we switch to late binding.
   const ClientSocketHandle* const key_handle_;
-  const base::TimeDelta timeout_duration_;
-  // Timer to abort jobs that take too long.
-  base::OneShotTimer<ConnectJob> timer_;
-  Delegate* delegate_;
+  Delegate* const delegate_;
   LoadState load_state_;
   scoped_ptr<ClientSocket> socket_;
 
@@ -178,9 +168,6 @@ class ClientSocketPoolBase
 
   // For testing.
   bool may_have_stalled_group() const { return may_have_stalled_group_; }
-  int NumConnectJobsInGroup(const std::string& group_name) const {
-    return group_map_.find(group_name)->second.jobs.size();
-  }
 
  private:
   // Entry for a persistent socket which became idle at time |start_time|.
@@ -265,7 +252,7 @@ class ClientSocketPoolBase
   // binding is enabled.  |job| must be non-NULL when late binding is
   // enabled.  Also updates |group| if non-NULL.
   void RemoveConnectJob(const ClientSocketHandle* handle,
-                        const ConnectJob* job,
+                        ConnectJob* job,
                         Group* group);
 
   // Same as OnAvailableSocketSlot except it looks up the Group first to see if
diff --git a/net/socket/client_socket_pool_base_unittest.cc b/net/socket/client_socket_pool_base_unittest.cc
index 7d188b9..dcae067b 100644
--- a/net/socket/client_socket_pool_base_unittest.cc
+++ b/net/socket/client_socket_pool_base_unittest.cc
@@ -6,7 +6,6 @@
 
 #include "base/compiler_specific.h"
 #include "base/message_loop.h"
-#include "base/platform_thread.h"
 #include "base/scoped_vector.h"
 #include "net/base/net_errors.h"
 #include "net/base/test_completion_callback.h"
@@ -108,22 +107,16 @@ class TestConnectJob : public ConnectJob {
   TestConnectJob(JobType job_type,
                  const std::string& group_name,
                  const ClientSocketPoolBase::Request& request,
-                 base::TimeDelta timeout_duration,
                  ConnectJob::Delegate* delegate,
                  MockClientSocketFactory* client_socket_factory)
-      : ConnectJob(group_name, request.handle, timeout_duration, delegate),
+      : ConnectJob(group_name, request.handle, delegate),
         job_type_(job_type),
         client_socket_factory_(client_socket_factory),
         method_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {}
 
-  void Signal() {
-    DoConnect(waiting_success_, true /* async */);
-  }
-
- private:
   // ConnectJob methods:
 
-  virtual int ConnectInternal() {
+  virtual int Connect() {
     AddressList ignored;
     client_socket_factory_->CreateTCPClientSocket(ignored);
     switch (job_type_) {
@@ -165,6 +158,11 @@ class TestConnectJob : public ConnectJob {
     }
   }
 
+  void Signal() {
+    DoConnect(waiting_success_, true /* async */);
+  }
+
+ private:
   int DoConnect(bool succeed, bool was_async) {
     int result = ERR_CONNECTION_FAILED;
     if (succeed) {
@@ -209,10 +207,6 @@ class TestConnectJobFactory : public ClientSocketPoolBase::ConnectJobFactory {
 
   void set_job_type(TestConnectJob::JobType job_type) { job_type_ = job_type; }
 
-  void set_timeout_duration(base::TimeDelta timeout_duration) {
-    timeout_duration_ = timeout_duration;
-  }
-
   // ConnectJobFactory methods:
 
   virtual ConnectJob* NewConnectJob(
@@ -222,14 +216,12 @@ class TestConnectJobFactory : public ClientSocketPoolBase::ConnectJobFactory {
     return new TestConnectJob(job_type_,
                               group_name,
                               request,
-                              timeout_duration_,
                               delegate,
                               client_socket_factory_);
   }
 
  private:
   TestConnectJob::JobType job_type_;
-  base::TimeDelta timeout_duration_;
   MockClientSocketFactory* const client_socket_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(TestConnectJobFactory);
@@ -283,10 +275,6 @@ class TestClientSocketPool : public ClientSocketPool {
 
   const ClientSocketPoolBase* base() const { return base_.get(); }
 
-  int NumConnectJobsInGroup(const std::string& group_name) const {
-    return base_->NumConnectJobsInGroup(group_name);
-  }
-
  private:
   const scoped_refptr<ClientSocketPoolBase> base_;
 
@@ -301,37 +289,6 @@ void MockClientSocketFactory::SignalJobs() {
   waiting_jobs_.clear();
 }
 
-class TestConnectJobDelegate : public ConnectJob::Delegate {
- public:
-  TestConnectJobDelegate()
-      : have_result_(false), waiting_for_result_(false), result_(OK) {}
-  virtual ~TestConnectJobDelegate() {}
-
-  virtual void OnConnectJobComplete(int result, ConnectJob* job) {
-    result_ = result;
-    delete job;
-    have_result_ = true;
-    if (waiting_for_result_)
-      MessageLoop::current()->Quit();
-  }
-
-  int WaitForResult() {
-    DCHECK(!waiting_for_result_);
-    while (!have_result_) {
-      waiting_for_result_ = true;
-      MessageLoop::current()->Run();
-      waiting_for_result_ = false;
-    }
-    have_result_ = false;  // auto-reset for next callback
-    return result_;
-  }
-
- private:
-  bool have_result_;
-  bool waiting_for_result_;
-  int result_;
-};
-
 class ClientSocketPoolBaseTest : public ClientSocketPoolTest {
  protected:
   ClientSocketPoolBaseTest()
@@ -367,34 +324,6 @@ class ClientSocketPoolBaseTest : public ClientSocketPoolTest {
   scoped_refptr<TestClientSocketPool> pool_;
 };
 
-// Even though a timeout is specified, it doesn't time out on a synchronous
-// completion.
-TEST(ConnectJobTest, NoTimeoutOnSynchronousCompletion) {
-  TestConnectJobDelegate delegate;
-  MockClientSocketFactory factory;
-  TestConnectJob job(TestConnectJob::kMockJob,
-                     "a", 
-                     ClientSocketPoolBase::Request(),
-                     base::TimeDelta::FromMicroseconds(1),
-                     &delegate,
-                     &factory);
-  EXPECT_EQ(OK, job.Connect());
-}
-
-TEST(ConnectJobTest, JobTimedOut) {
-  TestConnectJobDelegate delegate;
-  MockClientSocketFactory factory;
-  TestConnectJob job(TestConnectJob::kMockPendingJob,
-                     "a", 
-                     ClientSocketPoolBase::Request(),
-                     base::TimeDelta::FromMicroseconds(1),
-                     &delegate,
-                     &factory);
-  ASSERT_EQ(ERR_IO_PENDING, job.Connect());
-  PlatformThread::Sleep(1);
-  EXPECT_EQ(ERR_TIMED_OUT, delegate.WaitForResult());
-}
-
 TEST_F(ClientSocketPoolBaseTest, BasicSynchronous) {
   CreatePool(kDefaultMaxSockets, kDefaultMaxSocketsPerGroup);
 
@@ -1281,28 +1210,6 @@ TEST_F(ClientSocketPoolBaseTest_LateBinding, CancelRequest) {
   EXPECT_EQ(kIndexOutOfBounds, GetOrderOfRequest(8));
 }
 
-TEST_F(ClientSocketPoolBaseTest_LateBinding, CancelRequestLimitsJobs) {
-  connect_job_factory_->set_job_type(TestConnectJob::kMockPendingJob);
-
-  CreatePool(kDefaultMaxSockets, kDefaultMaxSocketsPerGroup);
-
-  EXPECT_EQ(ERR_IO_PENDING, StartRequest("a", 1));
-  EXPECT_EQ(ERR_IO_PENDING, StartRequest("a", 2));
-  EXPECT_EQ(ERR_IO_PENDING, StartRequest("a", 3));
-  EXPECT_EQ(ERR_IO_PENDING, StartRequest("a", 4));
-
-  EXPECT_EQ(kDefaultMaxSocketsPerGroup, pool_->NumConnectJobsInGroup("a"));
-  requests_[2]->handle()->Reset();
-  requests_[3]->handle()->Reset();
-  EXPECT_EQ(kDefaultMaxSocketsPerGroup, pool_->NumConnectJobsInGroup("a"));
-
-  requests_[1]->handle()->Reset();
-  EXPECT_EQ(kDefaultMaxSocketsPerGroup, pool_->NumConnectJobsInGroup("a"));
-
-  requests_[0]->handle()->Reset();
-  EXPECT_EQ(kDefaultMaxSocketsPerGroup - 1, pool_->NumConnectJobsInGroup("a"));
-}
-
 TEST_F(ClientSocketPoolBaseTest_LateBinding, RequestPendingJobTwice) {
   CreatePool(kDefaultMaxSockets, kDefaultMaxSocketsPerGroup);
 
diff --git a/net/socket/tcp_client_socket_pool.cc b/net/socket/tcp_client_socket_pool.cc
index 197ccdb..decc23f1 100644
--- a/net/socket/tcp_client_socket_pool.cc
+++ b/net/socket/tcp_client_socket_pool.cc
@@ -18,19 +18,14 @@ using base::TimeDelta;
 
 namespace net {
 
-// TCPConnectJobs will time out after this many seconds.  Note this is the total
-// time, including both host resolution and TCP connect() times.
-static const int kTCPConnectJobTimeoutInSeconds = 60;
-
 TCPConnectJob::TCPConnectJob(
     const std::string& group_name,
     const HostResolver::RequestInfo& resolve_info,
     const ClientSocketHandle* handle,
-    base::TimeDelta timeout_duration,
     ClientSocketFactory* client_socket_factory,
     HostResolver* host_resolver,
     Delegate* delegate)
-    : ConnectJob(group_name, handle, timeout_duration, delegate),
+    : ConnectJob(group_name, handle, delegate),
       resolve_info_(resolve_info),
       client_socket_factory_(client_socket_factory),
       ALLOW_THIS_IN_INITIALIZER_LIST(
@@ -43,7 +38,7 @@ TCPConnectJob::~TCPConnectJob() {
   // ~SingleRequestHostResolver and ~ClientSocket will take care of it.
 }
 
-int TCPConnectJob::ConnectInternal() {
+int TCPConnectJob::Connect() {
   next_state_ = kStateResolveHost;
   return DoLoop(OK);
 }
@@ -133,7 +128,6 @@ ConnectJob* TCPClientSocketPool::TCPConnectJobFactory::NewConnectJob(
     ConnectJob::Delegate* delegate) const {
   return new TCPConnectJob(
       group_name, request.resolve_info, request.handle,
-      base::TimeDelta::FromSeconds(kTCPConnectJobTimeoutInSeconds),
       client_socket_factory_, host_resolver_, delegate);
 }
 
diff --git a/net/socket/tcp_client_socket_pool.h b/net/socket/tcp_client_socket_pool.h
index b07ea68..1906dcc 100644
--- a/net/socket/tcp_client_socket_pool.h
+++ b/net/socket/tcp_client_socket_pool.h
@@ -10,8 +10,6 @@
 #include "base/basictypes.h"
 #include "base/ref_counted.h"
 #include "base/scoped_ptr.h"
-#include "base/time.h"
-#include "base/timer.h"
 #include "net/socket/client_socket_pool_base.h"
 #include "net/socket/client_socket_pool.h"
 
@@ -26,7 +24,6 @@ class TCPConnectJob : public ConnectJob {
   TCPConnectJob(const std::string& group_name,
                 const HostResolver::RequestInfo& resolve_info,
                 const ClientSocketHandle* handle,
-                base::TimeDelta timeout_duration,
                 ClientSocketFactory* client_socket_factory,
                 HostResolver* host_resolver,
                 Delegate* delegate);
@@ -34,6 +31,11 @@ class TCPConnectJob : public ConnectJob {
 
   // ConnectJob methods.
 
+  // Begins the host resolution and the TCP connect.  Returns OK on success
+  // and ERR_IO_PENDING if it cannot immediately service the request.
+  // Otherwise, it returns a net error code.
+  virtual int Connect();
+
  private:
   enum State {
     kStateResolveHost,
@@ -43,11 +45,6 @@ class TCPConnectJob : public ConnectJob {
     kStateNone,
   };
 
-  // Begins the host resolution and the TCP connect.  Returns OK on success
-  // and ERR_IO_PENDING if it cannot immediately service the request.
-  // Otherwise, it returns a net error code.
-  virtual int ConnectInternal();
-
   void OnIOComplete(int result);
 
   // Runs the state transition loop.