Skip to content

Commit

Permalink
Socket Pools Refactor 30: Finish flattening the socket pools.
Browse files Browse the repository at this point in the history 
This CL merges the SSL socket pool that sits on top of the
HTTP proxy socket pool into that socket pool. SSLConnectJobs
that are layered on top of HttpProxyConnectJobs now directly
create those nested jobs, instead of going through another
socket pool.

While this is the final CL needed to fully flatten the socket
pools (though H2 sessions are still layered on top of them),
there are a number of related cleanups that will need to
follow this CL.

This is part of an effort to flatten the socket pools.
https://docs.google.com/document/d/1g0EA4iDqaDhNXA_mq-YK3SlSX-xRkoKvZetAQqdRrxM/edit

Bug: 472729
Change-Id: I98aee1ac0235440902deed1e3d66479efae3af40
Reviewed-on: https://chromium-review.googlesource.com/c/1489428
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Asanka Herath <asanka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#637123}
  • Loading branch information
Matt Menke authored and Commit Bot committed Mar 2, 2019
1 parent c804cc7 commit aade581
Show file tree
Hide file tree
Showing 22 changed files with 301 additions and 345 deletions.
7 changes: 0 additions & 7 deletions net/http/http_network_session.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -331,13 +331,6 @@ TransportClientSocketPool* HttpNetworkSession::GetSocketPoolForHTTPLikeProxy(
http_proxy);
}

TransportClientSocketPool* HttpNetworkSession::GetSocketPoolForSSLWithProxy(
SocketPoolType pool_type,
const ProxyServer& proxy_server) {
return GetSocketPoolManager(pool_type)->GetSocketPoolForSSLWithProxy(
proxy_server);
}

std::unique_ptr<base::Value> HttpNetworkSession::SocketPoolInfoToValue() const {
// TODO(yutak): Should merge values from normal pools and WebSocket pools.
return normal_socket_pool_manager_->SocketPoolInfoToValue();
Expand Down
3 changes: 0 additions & 3 deletions net/http/http_network_session.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -304,9 +304,6 @@ class NET_EXPORT HttpNetworkSession {
TransportClientSocketPool* GetSocketPoolForHTTPLikeProxy(
SocketPoolType pool_type,
const ProxyServer& http_proxy);
TransportClientSocketPool* GetSocketPoolForSSLWithProxy(
SocketPoolType pool_type,
const ProxyServer& proxy_server);

CertVerifier* cert_verifier() { return cert_verifier_; }
ProxyResolutionService* proxy_resolution_service() {
Expand Down
13 changes: 2 additions & 11 deletions net/http/http_network_transaction_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11290,24 +11290,15 @@ TEST_F(HttpNetworkTransactionTest, GroupNameForHTTPProxyConnections) {
HostPortPair("http_proxy", 80));
CaptureGroupNameTransportSocketPool* http_proxy_pool =
new CaptureGroupNameTransportSocketPool(NULL, NULL);
CaptureGroupNameTransportSocketPool* ssl_conn_pool =
new CaptureGroupNameTransportSocketPool(NULL, NULL);
auto mock_pool_manager = std::make_unique<MockClientSocketPoolManager>();
mock_pool_manager->SetSocketPoolForHTTPProxy(
proxy_server, base::WrapUnique(http_proxy_pool));
mock_pool_manager->SetSocketPoolForSSLWithProxy(
proxy_server, base::WrapUnique(ssl_conn_pool));
peer.SetClientSocketPoolManager(std::move(mock_pool_manager));

EXPECT_EQ(ERR_IO_PENDING,
GroupNameTransactionHelper(tests[i].url, session.get()));
if (tests[i].ssl) {
EXPECT_EQ(tests[i].expected_group_name,
ssl_conn_pool->last_group_name_received());
} else {
EXPECT_EQ(tests[i].expected_group_name,
http_proxy_pool->last_group_name_received());
}
EXPECT_EQ(tests[i].expected_group_name,
http_proxy_pool->last_group_name_received());
}
}

Expand Down
3 changes: 1 addition & 2 deletions net/http/http_proxy_client_socket_wrapper.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -577,8 +577,7 @@ int HttpProxyClientSocketWrapper::DoSSLConnect() {
}
next_state_ = STATE_SSL_CONNECT_COMPLETE;
nested_connect_job_ = std::make_unique<SSLConnectJob>(
priority_, common_connect_job_params_, ssl_params_,
nullptr /* http_proxy_pool */, this, &net_log_);
priority_, common_connect_job_params_, ssl_params_, this, &net_log_);
return nested_connect_job_->Connect();
}

Expand Down
16 changes: 7 additions & 9 deletions net/http/http_stream_factory_job.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -619,14 +619,19 @@ void HttpStreamFactory::Job::RunLoop(int result) {

case ERR_HTTPS_PROXY_TUNNEL_RESPONSE_REDIRECT: {
DCHECK(connection_.get());
DCHECK(connection_->socket());
DCHECK(establishing_tunnel_);

LoadTimingInfo load_timing_info;
bool have_load_timing_info = connection_->GetLoadTimingInfo(
connection_->is_reused(), &load_timing_info);

std::unique_ptr<StreamSocket> socket =
connection_->release_pending_http_proxy_socket();
DCHECK(socket);

connection_.reset();
ProxyClientSocket* proxy_socket =
static_cast<ProxyClientSocket*>(connection_->socket());
static_cast<ProxyClientSocket*>(socket.get());
base::ThreadTaskRunnerHandle::Get()->PostTask(
FROM_HERE,
base::BindOnce(
Expand Down Expand Up @@ -1072,13 +1077,6 @@ int HttpStreamFactory::Job::DoInitConnectionComplete(int result) {

if (result == ERR_HTTPS_PROXY_TUNNEL_RESPONSE_REDIRECT) {
DCHECK(!ssl_started);
// Other state (i.e. |using_ssl_|) suggests that |connection_| will have an
// SSL socket, but there was an error before that could happen. This
// puts the in progress HttpProxy socket into |connection_| in order to
// complete the auth (or read the response body). The tunnel restart code
// is careful to remove it before returning control to the rest of this
// class.
connection_ = connection_->release_pending_http_proxy_connection();
return result;
}

Expand Down
72 changes: 4 additions & 68 deletions net/http/http_stream_factory_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -528,28 +528,14 @@ TEST_F(HttpStreamFactoryTest, PreconnectHttpProxy) {
session_deps.transport_security_state.get(),
session_deps.cert_transparency_verifier.get(),
session_deps.ct_policy_enforcer.get());
CapturePreconnectsTransportSocketPool* ssl_conn_pool =
new CapturePreconnectsTransportSocketPool(
session_deps.host_resolver.get(), session_deps.cert_verifier.get(),
session_deps.transport_security_state.get(),
session_deps.cert_transparency_verifier.get(),
session_deps.ct_policy_enforcer.get());
auto mock_pool_manager = std::make_unique<MockClientSocketPoolManager>();
mock_pool_manager->SetSocketPoolForHTTPProxy(
proxy_server, base::WrapUnique(http_proxy_pool));
mock_pool_manager->SetSocketPoolForSSLWithProxy(
proxy_server, base::WrapUnique(ssl_conn_pool));
peer.SetClientSocketPoolManager(std::move(mock_pool_manager));
PreconnectHelper(kTests[i], session.get());
if (kTests[i].ssl) {
EXPECT_EQ(kTests[i].num_streams, ssl_conn_pool->last_num_streams());
EXPECT_EQ("http_proxy/" + GetGroupName(kTests[i]),
ssl_conn_pool->last_group_name());
} else {
EXPECT_EQ(kTests[i].num_streams, http_proxy_pool->last_num_streams());
EXPECT_EQ("http_proxy/" + GetGroupName(kTests[i]),
http_proxy_pool->last_group_name());
}
EXPECT_EQ(kTests[i].num_streams, http_proxy_pool->last_num_streams());
EXPECT_EQ("http_proxy/" + GetGroupName(kTests[i]),
http_proxy_pool->last_group_name());
}
}

Expand Down Expand Up @@ -1181,20 +1167,12 @@ TEST_F(HttpStreamFactoryTest, UsePreConnectIfNoZeroRTT) {
session_deps.transport_security_state.get(),
session_deps.cert_transparency_verifier.get(),
session_deps.ct_policy_enforcer.get());
CapturePreconnectsTransportSocketPool* ssl_conn_pool =
new CapturePreconnectsTransportSocketPool(
session_deps.host_resolver.get(), session_deps.cert_verifier.get(),
session_deps.transport_security_state.get(),
session_deps.cert_transparency_verifier.get(),
session_deps.ct_policy_enforcer.get());
auto mock_pool_manager = std::make_unique<MockClientSocketPoolManager>();
mock_pool_manager->SetSocketPoolForHTTPProxy(
proxy_server, base::WrapUnique(http_proxy_pool));
mock_pool_manager->SetSocketPoolForSSLWithProxy(
proxy_server, base::WrapUnique(ssl_conn_pool));
peer.SetClientSocketPoolManager(std::move(mock_pool_manager));
PreconnectHelperForURL(num_streams, url, session.get());
EXPECT_EQ(num_streams, ssl_conn_pool->last_num_streams());
EXPECT_EQ(num_streams, http_proxy_pool->last_num_streams());
}
}

Expand Down Expand Up @@ -1242,20 +1220,11 @@ TEST_F(HttpStreamFactoryTest, OnlyOnePreconnectToProxyServer) {
session_deps.transport_security_state.get(),
session_deps.cert_transparency_verifier.get(),
session_deps.ct_policy_enforcer.get());
CapturePreconnectsTransportSocketPool* ssl_conn_pool =
new CapturePreconnectsTransportSocketPool(
session_deps.host_resolver.get(),
session_deps.cert_verifier.get(),
session_deps.transport_security_state.get(),
session_deps.cert_transparency_verifier.get(),
session_deps.ct_policy_enforcer.get());

auto mock_pool_manager =
std::make_unique<MockClientSocketPoolManager>();
mock_pool_manager->SetSocketPoolForHTTPProxy(
proxy_server, base::WrapUnique(http_proxy_pool));
mock_pool_manager->SetSocketPoolForSSLWithProxy(
proxy_server, base::WrapUnique(ssl_conn_pool));
peer.SetClientSocketPoolManager(std::move(mock_pool_manager));

HttpRequestInfo request;
Expand All @@ -1269,13 +1238,11 @@ TEST_F(HttpStreamFactoryTest, OnlyOnePreconnectToProxyServer) {
// First preconnect job should succeed.
session->http_stream_factory()->PreconnectStreams(num_streams,
request);
EXPECT_EQ(-1, ssl_conn_pool->last_num_streams());
EXPECT_EQ(num_streams, http_proxy_pool->last_num_streams());
} else {
// Second preconnect job should not succeed.
session->http_stream_factory()->PreconnectStreams(num_streams,
request);
EXPECT_EQ(-1, ssl_conn_pool->last_num_streams());
if (set_http_server_properties) {
EXPECT_EQ(-1, http_proxy_pool->last_num_streams());
} else {
Expand Down Expand Up @@ -1335,18 +1302,10 @@ TEST_F(HttpStreamFactoryTest, ProxyServerPreconnectDifferentPrivacyModes) {
session_deps.transport_security_state.get(),
session_deps.cert_transparency_verifier.get(),
session_deps.ct_policy_enforcer.get());
CapturePreconnectsTransportSocketPool* ssl_conn_pool =
new CapturePreconnectsTransportSocketPool(
session_deps.host_resolver.get(), session_deps.cert_verifier.get(),
session_deps.transport_security_state.get(),
session_deps.cert_transparency_verifier.get(),
session_deps.ct_policy_enforcer.get());

auto mock_pool_manager = std::make_unique<MockClientSocketPoolManager>();
mock_pool_manager->SetSocketPoolForHTTPProxy(
proxy_server, base::WrapUnique(http_proxy_pool));
mock_pool_manager->SetSocketPoolForSSLWithProxy(
proxy_server, base::WrapUnique(ssl_conn_pool));
peer.SetClientSocketPoolManager(std::move(mock_pool_manager));

HttpRequestInfo request_privacy_mode_disabled;
Expand All @@ -1359,14 +1318,12 @@ TEST_F(HttpStreamFactoryTest, ProxyServerPreconnectDifferentPrivacyModes) {
// First preconnect job should succeed.
session->http_stream_factory()->PreconnectStreams(
num_streams, request_privacy_mode_disabled);
EXPECT_EQ(-1, ssl_conn_pool->last_num_streams());
EXPECT_EQ(num_streams, http_proxy_pool->last_num_streams());
http_proxy_pool->reset();

// Second preconnect job with same privacy mode should not succeed.
session->http_stream_factory()->PreconnectStreams(
num_streams + 1, request_privacy_mode_disabled);
EXPECT_EQ(-1, ssl_conn_pool->last_num_streams());
EXPECT_EQ(-1, http_proxy_pool->last_num_streams());

// Next request with a different privacy mode should succeed.
Expand All @@ -1381,7 +1338,6 @@ TEST_F(HttpStreamFactoryTest, ProxyServerPreconnectDifferentPrivacyModes) {
// Request with a different privacy mode should succeed.
session->http_stream_factory()->PreconnectStreams(
num_streams, request_privacy_mode_enabled);
EXPECT_EQ(-1, ssl_conn_pool->last_num_streams());
EXPECT_EQ(num_streams, http_proxy_pool->last_num_streams());
}

Expand Down Expand Up @@ -1750,22 +1706,10 @@ TEST_F(HttpStreamFactoryTest, RequestHttpStreamOverProxy) {
HttpNetworkSession::NORMAL_SOCKET_POOL,
ProxyServer(ProxyServer::SCHEME_HTTPS,
HostPortPair("myproxy", 8888)))));
EXPECT_EQ(0, GetSocketPoolGroupCount(session->GetSocketPoolForSSLWithProxy(
HttpNetworkSession::NORMAL_SOCKET_POOL,
ProxyServer(ProxyServer::SCHEME_HTTP,
HostPortPair("myproxy", 8888)))));
EXPECT_EQ(0, GetSocketPoolGroupCount(session->GetSocketPoolForSSLWithProxy(
HttpNetworkSession::NORMAL_SOCKET_POOL,
ProxyServer(ProxyServer::SCHEME_HTTPS,
HostPortPair("myproxy", 8888)))));
EXPECT_EQ(0, GetSocketPoolGroupCount(session->GetSocketPoolForHTTPLikeProxy(
HttpNetworkSession::WEBSOCKET_SOCKET_POOL,
ProxyServer(ProxyServer::SCHEME_HTTP,
HostPortPair("myproxy", 8888)))));
EXPECT_EQ(0, GetSocketPoolGroupCount(session->GetSocketPoolForSSLWithProxy(
HttpNetworkSession::WEBSOCKET_SOCKET_POOL,
ProxyServer(ProxyServer::SCHEME_HTTP,
HostPortPair("myproxy", 8888)))));
EXPECT_FALSE(waiter.used_proxy_info().is_direct());
}

Expand Down Expand Up @@ -1978,18 +1922,10 @@ TEST_F(HttpStreamFactoryTest, RequestWebSocketBasicHandshakeStreamOverProxy) {
HttpNetworkSession::NORMAL_SOCKET_POOL,
ProxyServer(ProxyServer::SCHEME_HTTP,
HostPortPair("myproxy", 8888)))));
EXPECT_EQ(0, GetSocketPoolGroupCount(session->GetSocketPoolForSSLWithProxy(
HttpNetworkSession::NORMAL_SOCKET_POOL,
ProxyServer(ProxyServer::SCHEME_HTTP,
HostPortPair("myproxy", 8888)))));
EXPECT_EQ(1, GetSocketPoolGroupCount(session->GetSocketPoolForHTTPLikeProxy(
HttpNetworkSession::WEBSOCKET_SOCKET_POOL,
ProxyServer(ProxyServer::SCHEME_HTTP,
HostPortPair("myproxy", 8888)))));
EXPECT_EQ(0, GetSocketPoolGroupCount(session->GetSocketPoolForSSLWithProxy(
HttpNetworkSession::WEBSOCKET_SOCKET_POOL,
ProxyServer(ProxyServer::SCHEME_HTTP,
HostPortPair("myproxy", 8888)))));
EXPECT_FALSE(waiter.used_proxy_info().is_direct());
}

Expand Down
24 changes: 19 additions & 5 deletions net/socket/client_socket_handle.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,14 +76,20 @@ void ClientSocketHandle::ResetInternal(bool cancel) {
RemoveHigherLayeredPool(higher_pool_);
pool_ = NULL;
idle_time_ = base::TimeDelta();
connect_timing_ = LoadTimingInfo::ConnectTiming();
// Connection timing is still needed for handling
// ERR_HTTPS_PROXY_TUNNEL_RESPONSE_REDIRECT errors.
//
// TODO(mmenke): Remove once ERR_HTTPS_PROXY_TUNNEL_RESPONSE_REDIRECT no
// longer results in following a redirect.
if (!pending_http_proxy_socket_)
connect_timing_ = LoadTimingInfo::ConnectTiming();
pool_id_ = -1;
}

void ClientSocketHandle::ResetErrorState() {
is_ssl_error_ = false;
ssl_error_response_info_ = HttpResponseInfo();
pending_http_proxy_connection_.reset();
pending_http_proxy_socket_.reset();
}

LoadState ClientSocketHandle::GetLoadState() const {
Expand Down Expand Up @@ -132,11 +138,19 @@ void ClientSocketHandle::CloseIdleSocketsInGroup() {
bool ClientSocketHandle::GetLoadTimingInfo(
bool is_reused,
LoadTimingInfo* load_timing_info) const {
// Only return load timing information when there's a socket.
if (!socket_)
if (socket_) {
load_timing_info->socket_log_id = socket_->NetLog().source().id;
} else if (pending_http_proxy_socket_) {
// TODO(mmenke): This case is only needed for timing for redirects in the
// case of ERR_HTTPS_PROXY_TUNNEL_RESPONSE_REDIRECT. Remove this code once
// we no longer follow those redirects.
load_timing_info->socket_log_id =
pending_http_proxy_socket_->NetLog().source().id;
} else {
// Only return load timing information when there's a socket.
return false;
}

load_timing_info->socket_log_id = socket_->NetLog().source().id;
load_timing_info->socket_reused = is_reused;

// No times if the socket is reused.
Expand Down
11 changes: 5 additions & 6 deletions net/socket/client_socket_handle.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,9 +172,8 @@ class NET_EXPORT ClientSocketHandle {
void set_ssl_error_response_info(const HttpResponseInfo& ssl_error_state) {
ssl_error_response_info_ = ssl_error_state;
}
void set_pending_http_proxy_connection(
std::unique_ptr<ClientSocketHandle> connection) {
pending_http_proxy_connection_ = std::move(connection);
void set_pending_http_proxy_socket(std::unique_ptr<StreamSocket> socket) {
pending_http_proxy_socket_ = std::move(socket);
}
void set_connection_attempts(const ConnectionAttempts& attempts) {
connection_attempts_ = attempts;
Expand All @@ -191,8 +190,8 @@ class NET_EXPORT ClientSocketHandle {
const HttpResponseInfo& ssl_error_response_info() const {
return ssl_error_response_info_;
}
std::unique_ptr<ClientSocketHandle> release_pending_http_proxy_connection() {
return std::move(pending_http_proxy_connection_);
std::unique_ptr<StreamSocket> release_pending_http_proxy_socket() {
return std::move(pending_http_proxy_socket_);
}
// If the connection failed, returns the connection attempts made. (If it
// succeeded, they will be returned through the socket instead; see
Expand Down Expand Up @@ -247,7 +246,7 @@ class NET_EXPORT ClientSocketHandle {
int pool_id_; // See ClientSocketPool::ReleaseSocket() for an explanation.
bool is_ssl_error_;
HttpResponseInfo ssl_error_response_info_;
std::unique_ptr<ClientSocketHandle> pending_http_proxy_connection_;
std::unique_ptr<StreamSocket> pending_http_proxy_socket_;
std::vector<ConnectionAttempt> connection_attempts_;

NetLogSource requesting_source_;
Expand Down
2 changes: 1 addition & 1 deletion net/socket/client_socket_pool_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ int InitSocketPoolHelper(
ssl_pool = session->GetSocketPoolForSOCKSProxy(socket_pool_type,
proxy_info.proxy_server());
} else {
ssl_pool = session->GetSocketPoolForSSLWithProxy(
ssl_pool = session->GetSocketPoolForHTTPLikeProxy(
socket_pool_type, proxy_info.proxy_server());
}

Expand Down
2 changes: 0 additions & 2 deletions net/socket/client_socket_pool_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,6 @@ class NET_EXPORT_PRIVATE ClientSocketPoolManager {
// "HTTP-like" scheme, as defined by ProxyServer::is_http_like().
virtual TransportClientSocketPool* GetSocketPoolForHTTPLikeProxy(
const ProxyServer& http_proxy) = 0;
virtual TransportClientSocketPool* GetSocketPoolForSSLWithProxy(
const ProxyServer& proxy_server) = 0;
// Creates a Value summary of the state of the socket pools.
virtual std::unique_ptr<base::Value> SocketPoolInfoToValue() const = 0;

Expand Down
Loading

0 comments on commit aade581

Please sign in to comment.