Close idle sockets next time we are about to send data.

net/socket/client_socket_pool_base.cc

 // Copyright (c) 2011 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/socket/client_socket_pool_base.h"
 
 #include <math.h>
 #include "base/compiler_specific.h"
 #include "base/format_macros.h"
 #include "base/logging.h"
@@ skipping 164 matching lines @@
     int max_sockets,
     int max_sockets_per_group,
     base::TimeDelta unused_idle_socket_timeout,
     base::TimeDelta used_idle_socket_timeout,
     ConnectJobFactory* connect_job_factory)
     : idle_socket_count_(0),
       connecting_socket_count_(0),
       handed_out_socket_count_(0),
       max_sockets_(max_sockets),
       max_sockets_per_group_(max_sockets_per_group),
+      use_cleanup_timer_(true),
       unused_idle_socket_timeout_(unused_idle_socket_timeout),
       used_idle_socket_timeout_(used_idle_socket_timeout),
       connect_job_factory_(connect_job_factory),
       connect_backup_jobs_enabled_(false),
       pool_generation_number_(0),
       method_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
   DCHECK_LE(0, max_sockets_per_group);
   DCHECK_LE(max_sockets_per_group, max_sockets);
 
+#if defined(OS_ANDROID)
+  SetCleanupTimerEnabled(false);

[willchan no longer on Chromium, 2011/11/11 23:47:50]

I think you should do this differently. The problem with this is there's no way
to change this for OS_ANDROID then. There's no public API
(ClientSocketPoolBaseHelper is an internal class, not exposed to net consumers)
to tweak this. Either we should be passing this in as a parameter to the
constructor, or we should be initializing the member variable from a global
(this is a pattern we've done elsewhere) in the constructor.

I think the "right" thing to do is pass it in as a constructor argument. That
might require a bunch of changes to existing ClientSocketPool classes. I'm find
deferring that change due to time constraints, but would like it to be done
eventually. The expedient change would be to just use a global variable (see
g_connect_backup_jobs_enabled).

+#endif
   NetworkChangeNotifier::AddIPAddressObserver(this);
 }
 
 ClientSocketPoolBaseHelper::~ClientSocketPoolBaseHelper() {
   // Clean up any idle sockets and pending connect jobs.  Assert that we have no
   // remaining active sockets or pending requests.  They should have all been
   // cleaned up prior to |this| being destroyed.
   Flush();
   DCHECK(group_map_.empty());
   DCHECK(pending_callback_map_.empty());
@@ skipping 26 matching lines @@
     group->CleanupBackupJob();
   return req;
 }
 
 int ClientSocketPoolBaseHelper::RequestSocket(
     const std::string& group_name,
     const Request* request) {
   CHECK(request->callback());
   CHECK(request->handle());
 
+  // Cleanup any timed-out idle sockets if no timer is used.
+  if (!use_cleanup_timer_)
+    CleanupIdleSockets(false);

[willchan no longer on Chromium, 2011/11/11 23:47:50]

Move this after the BeginEvent().

+
   request->net_log().BeginEvent(NetLog::TYPE_SOCKET_POOL, NULL);
   Group* group = GetOrCreateGroup(group_name);
 
   int rv = RequestSocketInternal(group_name, request);
   if (rv != ERR_IO_PENDING) {
     request->net_log().EndEventWithNetErrorCode(NetLog::TYPE_SOCKET_POOL, rv);
     CHECK(!request->handle()->is_initialized());
     delete request;
   } else {
     InsertRequestIntoQueue(request, group->mutable_pending_requests());
   }
   return rv;
 }
 
 void ClientSocketPoolBaseHelper::RequestSockets(
     const std::string& group_name,
     const Request& request,
     int num_sockets) {
   DCHECK(!request.callback());
   DCHECK(!request.handle());
 
+  // Cleanup any timed out idle sockets if no timer is used.
+  if (!use_cleanup_timer_)
+    CleanupIdleSockets(false);
+
   if (num_sockets > max_sockets_per_group_) {
     num_sockets = max_sockets_per_group_;
   }
 
   request.net_log().BeginEvent(
       NetLog::TYPE_SOCKET_POOL_CONNECTING_N_SOCKETS,
       make_scoped_refptr(new NetLogIntegerParameter(
           "num_sockets", num_sockets)));
 
   Group* group = GetOrCreateGroup(group_name);
@@ skipping 418 matching lines @@
   bool old_value = g_connect_backup_jobs_enabled;
   g_connect_backup_jobs_enabled = enabled;
   return old_value;
 }
 
 void ClientSocketPoolBaseHelper::EnableConnectBackupJobs() {
   connect_backup_jobs_enabled_ = g_connect_backup_jobs_enabled;
 }
 
 void ClientSocketPoolBaseHelper::IncrementIdleCount() {
-  if (++idle_socket_count_ == 1)
-    timer_.Start(FROM_HERE, TimeDelta::FromSeconds(kCleanupInterval), this,
-                 &ClientSocketPoolBaseHelper::OnCleanupTimerFired);
+  if (++idle_socket_count_ == 1 && use_cleanup_timer_)
+    StartIdleSocketTimer();
 }
 
 void ClientSocketPoolBaseHelper::DecrementIdleCount() {
   if (--idle_socket_count_ == 0)
     timer_.Stop();
 }
 
+void ClientSocketPoolBaseHelper::SetCleanupTimerEnabled(bool enabled) {

[willchan no longer on Chromium, 2011/11/11 23:47:50]

I think it's not necessary to have this code where you can dynamically turn
on/off cleanup timers. I think it's OK for us to just assume it never changes
after construction, and furthermore, I'd rather you do that because it makes
this already complicated code easier to reason about.

+  use_cleanup_timer_ = enabled;
+  if (enabled) {
+    if (idle_socket_count_ > 0)
+      StartIdleSocketTimer();
+  } else {
+    timer_.Stop();
+  }
+}
+
+void ClientSocketPoolBaseHelper::StartIdleSocketTimer() {
+  timer_.Start(FROM_HERE, TimeDelta::FromSeconds(kCleanupInterval), this,
+               &ClientSocketPoolBaseHelper::OnCleanupTimerFired);
+}
+
 void ClientSocketPoolBaseHelper::ReleaseSocket(const std::string& group_name,
                                                StreamSocket* socket,
                                                int id) {
   GroupMap::iterator i = group_map_.find(group_name);
   CHECK(i != group_map_.end());
 
   Group* group = i->second;
 
   CHECK_GT(handed_out_socket_count_, 0);
   handed_out_socket_count_--;
@@ skipping 170 matching lines @@
     const std::string& group_name, Group* group) {
   int rv = RequestSocketInternal(group_name,
                                  *group->pending_requests().begin());
   if (rv != ERR_IO_PENDING) {
     scoped_ptr<const Request> request(RemoveRequestFromQueue(
           group->mutable_pending_requests()->begin(), group));
     if (group->IsEmpty())
       RemoveGroup(group_name);
 
     request->net_log().EndEventWithNetErrorCode(NetLog::TYPE_SOCKET_POOL, rv);
-    InvokeUserCallbackLater(
-        request->handle(), request->callback(), rv);
+    InvokeUserCallbackLater(request->handle(), request->callback(), rv);
   }
 }
 
 void ClientSocketPoolBaseHelper::HandOutSocket(
     StreamSocket* socket,
     bool reused,
     ClientSocketHandle* handle,
     base::TimeDelta idle_time,
     Group* group,
     const BoundNetLog& net_log) {
@@ skipping 212 matching lines @@
   // Delete active jobs.
   STLDeleteElements(&jobs_);
 
   // Cancel pending backup job.
   method_factory_.RevokeAll();
 }
 
 }  // namespace internal
 
 }  // namespace net