Refactor WebSocketTransportClientSocketPool's socket handing out code

net/socket/websocket_transport_client_socket_pool.cc

 // Copyright 2014 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/websocket_transport_client_socket_pool.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/compiler_specific.h"
@@ skipping 344 matching lines @@
         StalledRequestMap::value_type(handle, iterator));
     return ERR_IO_PENDING;
   }
 
   std::unique_ptr<WebSocketTransportConnectJob> connect_job(
       new WebSocketTransportConnectJob(
           group_name, priority, respect_limits, casted_params,
           ConnectionTimeout(), callback, client_socket_factory_, host_resolver_,
           handle, &connect_job_delegate_, pool_net_log_, request_net_log));
 
-  int rv = connect_job->Connect();
+  int result = connect_job->Connect();
+
   // Regardless of the outcome of |connect_job|, it will always be bound to
   // |handle|, since this pool uses early-binding. So the binding is logged
   // here, without waiting for the result.
   request_net_log.AddEvent(
       NetLogEventType::SOCKET_POOL_BOUND_TO_CONNECT_JOB,
       connect_job->net_log().source().ToEventParametersCallback());
-  if (rv == OK) {
-    HandOutSocket(connect_job->PassSocket(),
-                  connect_job->connect_timing(),
-                  handle,
-                  request_net_log);
-    request_net_log.EndEvent(NetLogEventType::SOCKET_POOL);
-  } else if (rv == ERR_IO_PENDING) {
+
+  if (result == ERR_IO_PENDING) {
     // TODO(ricea): Implement backup job timer?
     AddJob(handle, std::move(connect_job));
   } else {
-    std::unique_ptr<StreamSocket> error_socket;
-    connect_job->GetAdditionalErrorState(handle);
-    error_socket = connect_job->PassSocket();
-    if (error_socket) {
-      HandOutSocket(std::move(error_socket), connect_job->connect_timing(),
-                    handle, request_net_log);
-    }
+    TryHandOutSocket(result, connect_job.get());
   }
 
-  if (rv != ERR_IO_PENDING) {
-    request_net_log.EndEventWithNetErrorCode(NetLogEventType::SOCKET_POOL, rv);
-  }
-
-  return rv;
+  return result;
 }
 
 void WebSocketTransportClientSocketPool::RequestSockets(
     const std::string& group_name,
     const void* params,
     int num_sockets,
     const BoundNetLog& net_log) {
   NOTIMPLEMENTED();
 }
 
 void WebSocketTransportClientSocketPool::CancelRequest(
     const std::string& group_name,
     ClientSocketHandle* handle) {
   DCHECK(!handle->is_initialized());
   if (DeleteStalledRequest(handle))
     return;
   std::unique_ptr<StreamSocket> socket = handle->PassSocket();
   if (socket)
     ReleaseSocket(handle->group_name(), std::move(socket), handle->id());
   if (!DeleteJob(handle))
     pending_callbacks_.erase(handle);
-  if (!ReachedMaxSocketsLimit() && !stalled_request_queue_.empty())
-    ActivateStalledRequest();
+
+  ActivateStalledRequest();
 }
 
 void WebSocketTransportClientSocketPool::ReleaseSocket(
     const std::string& group_name,
     std::unique_ptr<StreamSocket> socket,
     int id) {
   WebSocketEndpointLockManager::GetInstance()->UnlockSocket(socket.get());
   CHECK_GT(handed_out_socket_count_, 0);
   --handed_out_socket_count_;
-  if (!ReachedMaxSocketsLimit() && !stalled_request_queue_.empty())
-    ActivateStalledRequest();
+
+  ActivateStalledRequest();
 }
 
 void WebSocketTransportClientSocketPool::FlushWithError(int error) {
+  DCHECK_NE(error, OK);
+
   // Sockets which are in LOAD_STATE_CONNECTING are in danger of unlocking
   // sockets waiting for the endpoint lock. If they connected synchronously,
   // then OnConnectJobComplete(). The |flushing_| flag tells this object to
   // ignore spurious calls to OnConnectJobComplete(). It is safe to ignore those
   // calls because this method will delete the jobs and call their callbacks
   // anyway.
   flushing_ = true;
   for (PendingConnectsMap::iterator it = pending_connects_.begin();
        it != pending_connects_.end();
        ++it) {
@@ skipping 56 matching lines @@
   // TODO(ricea): For now, we implement a global timeout for compatibility with
   // TransportConnectJob. Since WebSocketTransportConnectJob controls the
   // address selection process more tightly, it could do something smarter here.
   return base::TimeDelta::FromSeconds(TransportConnectJob::kTimeoutInSeconds);
 }
 
 bool WebSocketTransportClientSocketPool::IsStalled() const {
   return !stalled_request_queue_.empty();
 }
 
+bool WebSocketTransportClientSocketPool::TryHandOutSocket(
+    int result,
+    WebSocketTransportConnectJob* job) {
+  DCHECK_NE(result, ERR_IO_PENDING);
+
+  std::unique_ptr<StreamSocket> socket = job->PassSocket();
+  ClientSocketHandle* const handle = job->handle();
+  BoundNetLog request_net_log = job->request_net_log();
+  LoadTimingInfo::ConnectTiming connect_timing = job->connect_timing();
+
+  if (result == OK) {
+    DCHECK(socket);
+
+    HandOutSocket(std::move(socket), connect_timing, handle, request_net_log);
+
+    request_net_log.EndEvent(NetLogEventType::SOCKET_POOL);
+
+    return true;
+  }
+
+  bool handed_out_socket = false;
+
+  // If we got a socket, it must contain error information so pass that
+  // up so that the caller can retrieve it.
+  job->GetAdditionalErrorState(handle);
+  if (socket) {
+    HandOutSocket(std::move(socket), connect_timing, handle, request_net_log);
+    handed_out_socket = true;
+  }
+
+  request_net_log.EndEventWithNetErrorCode(NetLogEventType::SOCKET_POOL,
+                                           result);
+
+  return handed_out_socket;
+}
+
 void WebSocketTransportClientSocketPool::OnConnectJobComplete(
     int result,
     WebSocketTransportConnectJob* job) {
   DCHECK_NE(ERR_IO_PENDING, result);
 
-  std::unique_ptr<StreamSocket> socket = job->PassSocket();
-
   // See comment in FlushWithError.
   if (flushing_) {
+    std::unique_ptr<StreamSocket> socket = job->PassSocket();
     WebSocketEndpointLockManager::GetInstance()->UnlockSocket(socket.get());
     return;
   }
 
-  BoundNetLog request_net_log = job->request_net_log();
+  bool handed_out_socket = TryHandOutSocket(result, job);
+
   CompletionCallback callback = job->callback();
-  LoadTimingInfo::ConnectTiming connect_timing = job->connect_timing();
 
   ClientSocketHandle* const handle = job->handle();
-  bool handed_out_socket = false;
 
-  if (result == OK) {
-    DCHECK(socket.get());
-    handed_out_socket = true;
-    HandOutSocket(std::move(socket), connect_timing, handle, request_net_log);
-    request_net_log.EndEvent(NetLogEventType::SOCKET_POOL);
-  } else {
-    // If we got a socket, it must contain error information so pass that
-    // up so that the caller can retrieve it.
-    job->GetAdditionalErrorState(handle);
-    if (socket.get()) {
-      handed_out_socket = true;
-      HandOutSocket(std::move(socket), connect_timing, handle, request_net_log);
-    }
-    request_net_log.EndEventWithNetErrorCode(NetLogEventType::SOCKET_POOL,
-                                             result);
-  }
   bool delete_succeeded = DeleteJob(handle);
   DCHECK(delete_succeeded);
-  if (!handed_out_socket && !stalled_request_queue_.empty() &&
-      !ReachedMaxSocketsLimit())
+
+  job = NULL;

[yhirano, 2016/09/16 03:41:04]

[optional] nullptr

[tyoshino (SeeGerritForStatus), 2016/09/16 06:55:12]

Created another CL
https://codereview.chromium.org/2341233002/

+
+  if (!handed_out_socket)
     ActivateStalledRequest();
+
   InvokeUserCallbackLater(handle, callback, result);
 }
 
 void WebSocketTransportClientSocketPool::InvokeUserCallbackLater(
     ClientSocketHandle* handle,
     const CompletionCallback& callback,
     int rv) {
   DCHECK(!pending_callbacks_.count(handle));
   pending_callbacks_.insert(handle);
   base::ThreadTaskRunnerHandle::Get()->PostTask(
@@ skipping 15 matching lines @@
          base::checked_cast<int>(pending_connects_.size()) >=
              max_sockets_ - handed_out_socket_count_;
 }
 
 void WebSocketTransportClientSocketPool::HandOutSocket(
     std::unique_ptr<StreamSocket> socket,
     const LoadTimingInfo::ConnectTiming& connect_timing,
     ClientSocketHandle* handle,
     const BoundNetLog& net_log) {
   DCHECK(socket);
-  handle->SetSocket(std::move(socket));
   DCHECK_EQ(ClientSocketHandle::UNUSED, handle->reuse_type());
   DCHECK_EQ(0, handle->idle_time().InMicroseconds());
+
+  handle->SetSocket(std::move(socket));
   handle->set_pool_id(0);
   handle->set_connect_timing(connect_timing);
 
   net_log.AddEvent(
       NetLogEventType::SOCKET_POOL_BOUND_TO_SOCKET,
       handle->socket()->NetLog().source().ToEventParametersCallback());
 
   ++handed_out_socket_count_;
 }
 
@@ skipping 23 matching lines @@
 
 const WebSocketTransportConnectJob*
 WebSocketTransportClientSocketPool::LookupConnectJob(
     const ClientSocketHandle* handle) const {
   PendingConnectsMap::const_iterator it = pending_connects_.find(handle);
   CHECK(it != pending_connects_.end());
   return it->second;
 }
 
 void WebSocketTransportClientSocketPool::ActivateStalledRequest() {
-  DCHECK(!stalled_request_queue_.empty());
-  DCHECK(!ReachedMaxSocketsLimit());
   // Usually we will only be able to activate one stalled request at a time,
   // however if all the connects fail synchronously for some reason, we may be
   // able to clear the whole queue at once.
   while (!stalled_request_queue_.empty() && !ReachedMaxSocketsLimit()) {
     StalledRequest request(stalled_request_queue_.front());
     stalled_request_queue_.pop_front();
     stalled_request_map_.erase(request.handle);
+
     int rv = RequestSocket("ignored", &request.params, request.priority,
                            // Stalled requests can't have |respect_limits|
                            // DISABLED.
                            RespectLimits::ENABLED, request.handle,
                            request.callback, request.net_log);
+
     // ActivateStalledRequest() never returns synchronously, so it is never
     // called re-entrantly.
     if (rv != ERR_IO_PENDING)
       InvokeUserCallbackLater(request.handle, request.callback, rv);
   }
 }
 
 bool WebSocketTransportClientSocketPool::DeleteStalledRequest(
     ClientSocketHandle* handle) {
   StalledRequestMap::iterator it = stalled_request_map_.find(handle);
@@ skipping 29 matching lines @@
       handle(handle),
       callback(callback),
       net_log(net_log) {}
 
 WebSocketTransportClientSocketPool::StalledRequest::StalledRequest(
     const StalledRequest& other) = default;
 
 WebSocketTransportClientSocketPool::StalledRequest::~StalledRequest() {}
 
 }  // namespace net