Skip to content

Commit

Permalink
Revert 113300 - Revert of 112134 of Revert 112130 - Close idle connec…
Browse files Browse the repository at this point in the history 
…tions / SPDY sessions when needed.

Due to the idle connection state being held by different socket pools, it's possible for one socket pool to hold an idle socket in a lower layer socket pool. From the lower level socket pool's perspective, the socket is being "actively" used. From the higher socket pool's (including SpdySession, which is more of a connection manager) perspective, the connection is idle and can be closed if we have hit a limit.

Normally this isn't a big deal, except when we have a lot of idle SPDY connections and are connecting via a proxy, so we have low connection limits through the proxy server. We address this problem by allowing lower-level socket pools to tell higher level socket pools to close a socket. 

Fixed ASAN test failures by removing  .Times(1) and .Times(2)  from CloseMultipleIdleSocketsHeldByLayeredPoolWhenNeeded unittest (this removes the tests relying on the order of std::set in CloseOneIdleConnectionInLayeredPool). ASAN is probably causing the memory allocator to allocate the pools differently. The std::set is ordered by LayeredPool* which is the address of the LayeredPool (willchan).


BUG=62364,92244, 105839
TEST=none


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

TBR=willchan@chromium.org
Review URL: http://codereview.chromium.org/8745007

TBR=willchan@chromium.org

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

TBR=rtenneti@chromium.org
Review URL: http://codereview.chromium.org/8825014

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@113305 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
rtenneti@chromium.org committed Dec 7, 2011
1 parent 075a12f commit a4f8f99
Show file tree
Hide file tree
Showing 17 changed files with 48 additions and 663 deletions.
178 changes: 0 additions & 178 deletions net/http/http_network_transaction_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9642,182 +9642,4 @@ TEST_F(HttpNetworkTransactionTest, SendPipelineEvictionFallback) {
EXPECT_EQ("hello world", out.response_data);
}

TEST_F(HttpNetworkTransactionTest, CloseOldSpdySessionToOpenNewOne) {
HttpStreamFactory::set_next_protos(SpdyNextProtos());
int old_max_sockets_per_group =
ClientSocketPoolManager::max_sockets_per_group();
int old_max_sockets_per_proxy_server =
ClientSocketPoolManager::max_sockets_per_proxy_server();
int old_max_sockets_per_pool =
ClientSocketPoolManager::max_sockets_per_pool();
ClientSocketPoolManager::set_max_sockets_per_group(1);
ClientSocketPoolManager::set_max_sockets_per_proxy_server(1);
ClientSocketPoolManager::set_max_sockets_per_pool(1);

// Use two different hosts with different IPs so they don't get pooled.
SessionDependencies session_deps;
session_deps.host_resolver->rules()->AddRule("a.com", "10.0.0.1");
session_deps.host_resolver->rules()->AddRule("b.com", "10.0.0.2");
scoped_refptr<HttpNetworkSession> session(CreateSession(&session_deps));

SSLSocketDataProvider ssl1(true, OK);
ssl1.next_proto_status = SSLClientSocket::kNextProtoNegotiated;
ssl1.next_proto = "spdy/2";
ssl1.was_npn_negotiated = true;
SSLSocketDataProvider ssl2(true, OK);
ssl2.next_proto_status = SSLClientSocket::kNextProtoNegotiated;
ssl2.next_proto = "spdy/2";
ssl2.was_npn_negotiated = true;
session_deps.socket_factory.AddSSLSocketDataProvider(&ssl1);
session_deps.socket_factory.AddSSLSocketDataProvider(&ssl2);

scoped_ptr<spdy::SpdyFrame> host1_req(ConstructSpdyGet(
"https://www.a.com", false, 1, LOWEST));
MockWrite spdy1_writes[] = {
CreateMockWrite(*host1_req, 1),
};
scoped_ptr<spdy::SpdyFrame> host1_resp(ConstructSpdyGetSynReply(NULL, 0, 1));
scoped_ptr<spdy::SpdyFrame> host1_resp_body(ConstructSpdyBodyFrame(1, true));
MockRead spdy1_reads[] = {
CreateMockRead(*host1_resp, 2),
CreateMockRead(*host1_resp_body, 3),
MockRead(true, ERR_IO_PENDING, 4),
};

scoped_refptr<OrderedSocketData> spdy1_data(
new OrderedSocketData(
spdy1_reads, arraysize(spdy1_reads),
spdy1_writes, arraysize(spdy1_writes)));
session_deps.socket_factory.AddSocketDataProvider(spdy1_data);

scoped_ptr<spdy::SpdyFrame> host2_req(ConstructSpdyGet(
"https://www.b.com", false, 1, LOWEST));
MockWrite spdy2_writes[] = {
CreateMockWrite(*host2_req, 1),
};
scoped_ptr<spdy::SpdyFrame> host2_resp(ConstructSpdyGetSynReply(NULL, 0, 1));
scoped_ptr<spdy::SpdyFrame> host2_resp_body(ConstructSpdyBodyFrame(1, true));
MockRead spdy2_reads[] = {
CreateMockRead(*host2_resp, 2),
CreateMockRead(*host2_resp_body, 3),
MockRead(true, ERR_IO_PENDING, 4),
};

scoped_refptr<OrderedSocketData> spdy2_data(
new OrderedSocketData(
spdy2_reads, arraysize(spdy2_reads),
spdy2_writes, arraysize(spdy2_writes)));
session_deps.socket_factory.AddSocketDataProvider(spdy2_data);

MockWrite http_write[] = {
MockWrite("GET / HTTP/1.1\r\n"
"Host: www.a.com\r\n"
"Connection: keep-alive\r\n\r\n"),
};

MockRead http_read[] = {
MockRead("HTTP/1.1 200 OK\r\n"),
MockRead("Content-Type: text/html; charset=iso-8859-1\r\n"),
MockRead("Content-Length: 6\r\n\r\n"),
MockRead("hello!"),
};

StaticSocketDataProvider http_data(http_read, arraysize(http_read),
http_write, arraysize(http_write));
session_deps.socket_factory.AddSocketDataProvider(&http_data);

HostPortPair host_port_pair_a("www.a.com", 443);
HostPortProxyPair host_port_proxy_pair_a(
host_port_pair_a, ProxyServer::Direct());
EXPECT_FALSE(
session->spdy_session_pool()->HasSession(host_port_proxy_pair_a));

TestOldCompletionCallback callback;
HttpRequestInfo request1;
request1.method = "GET";
request1.url = GURL("https://www.a.com/");
request1.load_flags = 0;
scoped_ptr<HttpNetworkTransaction> trans(new HttpNetworkTransaction(session));

int rv = trans->Start(&request1, &callback, BoundNetLog());
EXPECT_EQ(ERR_IO_PENDING, rv);
EXPECT_EQ(OK, callback.WaitForResult());

const HttpResponseInfo* response = trans->GetResponseInfo();
ASSERT_TRUE(response != NULL);
ASSERT_TRUE(response->headers != NULL);
EXPECT_EQ("HTTP/1.1 200 OK", response->headers->GetStatusLine());
EXPECT_TRUE(response->was_fetched_via_spdy);
EXPECT_TRUE(response->was_npn_negotiated);

std::string response_data;
ASSERT_EQ(OK, ReadTransaction(trans.get(), &response_data));
EXPECT_EQ("hello!", response_data);
trans.reset();
EXPECT_TRUE(
session->spdy_session_pool()->HasSession(host_port_proxy_pair_a));

HostPortPair host_port_pair_b("www.b.com", 443);
HostPortProxyPair host_port_proxy_pair_b(
host_port_pair_b, ProxyServer::Direct());
EXPECT_FALSE(
session->spdy_session_pool()->HasSession(host_port_proxy_pair_b));
HttpRequestInfo request2;
request2.method = "GET";
request2.url = GURL("https://www.b.com/");
request2.load_flags = 0;
trans.reset(new HttpNetworkTransaction(session));

rv = trans->Start(&request2, &callback, BoundNetLog());
EXPECT_EQ(ERR_IO_PENDING, rv);
EXPECT_EQ(OK, callback.WaitForResult());

response = trans->GetResponseInfo();
ASSERT_TRUE(response != NULL);
ASSERT_TRUE(response->headers != NULL);
EXPECT_EQ("HTTP/1.1 200 OK", response->headers->GetStatusLine());
EXPECT_TRUE(response->was_fetched_via_spdy);
EXPECT_TRUE(response->was_npn_negotiated);
ASSERT_EQ(OK, ReadTransaction(trans.get(), &response_data));
EXPECT_EQ("hello!", response_data);
EXPECT_FALSE(
session->spdy_session_pool()->HasSession(host_port_proxy_pair_a));
EXPECT_TRUE(
session->spdy_session_pool()->HasSession(host_port_proxy_pair_b));

HostPortPair host_port_pair_a1("www.a.com", 80);
HostPortProxyPair host_port_proxy_pair_a1(
host_port_pair_a1, ProxyServer::Direct());
EXPECT_FALSE(
session->spdy_session_pool()->HasSession(host_port_proxy_pair_a1));
HttpRequestInfo request3;
request3.method = "GET";
request3.url = GURL("http://www.a.com/");
request3.load_flags = 0;
trans.reset(new HttpNetworkTransaction(session));

rv = trans->Start(&request3, &callback, BoundNetLog());
EXPECT_EQ(ERR_IO_PENDING, rv);
EXPECT_EQ(OK, callback.WaitForResult());

response = trans->GetResponseInfo();
ASSERT_TRUE(response != NULL);
ASSERT_TRUE(response->headers != NULL);
EXPECT_EQ("HTTP/1.1 200 OK", response->headers->GetStatusLine());
EXPECT_FALSE(response->was_fetched_via_spdy);
EXPECT_FALSE(response->was_npn_negotiated);
ASSERT_EQ(OK, ReadTransaction(trans.get(), &response_data));
EXPECT_EQ("hello!", response_data);
EXPECT_FALSE(
session->spdy_session_pool()->HasSession(host_port_proxy_pair_a));
EXPECT_FALSE(
session->spdy_session_pool()->HasSession(host_port_proxy_pair_b));

HttpStreamFactory::set_next_protos(std::vector<std::string>());
ClientSocketPoolManager::set_max_sockets_per_pool(old_max_sockets_per_pool);
ClientSocketPoolManager::set_max_sockets_per_proxy_server(
old_max_sockets_per_proxy_server);
ClientSocketPoolManager::set_max_sockets_per_group(old_max_sockets_per_group);
}

} // namespace net
36 changes: 2 additions & 34 deletions net/http/http_proxy_client_socket_pool.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -391,21 +391,9 @@ HttpProxyClientSocketPool::HttpProxyClientSocketPool(
new HttpProxyConnectJobFactory(transport_pool,
ssl_pool,
host_resolver,
net_log)) {
// We should always have a |transport_pool_| except in unit tests.
if (transport_pool_)
transport_pool_->AddLayeredPool(this);
if (ssl_pool_)
ssl_pool_->AddLayeredPool(this);
}
net_log)) {}

HttpProxyClientSocketPool::~HttpProxyClientSocketPool() {
if (ssl_pool_)
ssl_pool_->RemoveLayeredPool(this);
// We should always have a |transport_pool_| except in unit tests.
if (transport_pool_)
transport_pool_->RemoveLayeredPool(this);
}
HttpProxyClientSocketPool::~HttpProxyClientSocketPool() {}

int HttpProxyClientSocketPool::RequestSocket(const std::string& group_name,
const void* socket_params,
Expand Down Expand Up @@ -446,12 +434,6 @@ void HttpProxyClientSocketPool::Flush() {
base_.Flush();
}

bool HttpProxyClientSocketPool::IsStalled() const {
return base_.IsStalled() ||
(transport_pool_ && transport_pool_->IsStalled()) ||
(ssl_pool_ && ssl_pool_->IsStalled());
}

void HttpProxyClientSocketPool::CloseIdleSockets() {
base_.CloseIdleSockets();
}
Expand All @@ -470,14 +452,6 @@ LoadState HttpProxyClientSocketPool::GetLoadState(
return base_.GetLoadState(group_name, handle);
}

void HttpProxyClientSocketPool::AddLayeredPool(LayeredPool* layered_pool) {
base_.AddLayeredPool(layered_pool);
}

void HttpProxyClientSocketPool::RemoveLayeredPool(LayeredPool* layered_pool) {
base_.RemoveLayeredPool(layered_pool);
}

DictionaryValue* HttpProxyClientSocketPool::GetInfoAsValue(
const std::string& name,
const std::string& type,
Expand Down Expand Up @@ -508,10 +482,4 @@ ClientSocketPoolHistograms* HttpProxyClientSocketPool::histograms() const {
return base_.histograms();
}

bool HttpProxyClientSocketPool::CloseOneIdleConnection() {
if (base_.CloseOneIdleSocket())
return true;
return base_.CloseOneIdleConnectionInLayeredPool();
}

} // namespace net
12 changes: 1 addition & 11 deletions net/http/http_proxy_client_socket_pool.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,7 @@ class HttpProxyConnectJob : public ConnectJob {
DISALLOW_COPY_AND_ASSIGN(HttpProxyConnectJob);
};

class NET_EXPORT_PRIVATE HttpProxyClientSocketPool
: public ClientSocketPool, public LayeredPool {
class NET_EXPORT_PRIVATE HttpProxyClientSocketPool : public ClientSocketPool {
public:
HttpProxyClientSocketPool(
int max_sockets,
Expand Down Expand Up @@ -203,8 +202,6 @@ class NET_EXPORT_PRIVATE HttpProxyClientSocketPool

virtual void Flush() OVERRIDE;

virtual bool IsStalled() const OVERRIDE;

virtual void CloseIdleSockets() OVERRIDE;

virtual int IdleSocketCount() const OVERRIDE;
Expand All @@ -216,10 +213,6 @@ class NET_EXPORT_PRIVATE HttpProxyClientSocketPool
const std::string& group_name,
const ClientSocketHandle* handle) const OVERRIDE;

virtual void AddLayeredPool(LayeredPool* layered_pool) OVERRIDE;

virtual void RemoveLayeredPool(LayeredPool* layered_pool) OVERRIDE;

virtual base::DictionaryValue* GetInfoAsValue(
const std::string& name,
const std::string& type,
Expand All @@ -229,9 +222,6 @@ class NET_EXPORT_PRIVATE HttpProxyClientSocketPool

virtual ClientSocketPoolHistograms* histograms() const OVERRIDE;

// LayeredPool methods:
virtual bool CloseOneIdleConnection() OVERRIDE;

private:
typedef ClientSocketPoolBase<HttpProxySocketParams> PoolBase;

Expand Down
21 changes: 1 addition & 20 deletions net/socket/client_socket_handle.cc
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) 2011 The Chromium Authors. All rights reserved.
// Copyright (c) 2010 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.

Expand All @@ -15,8 +15,6 @@ namespace net {

ClientSocketHandle::ClientSocketHandle()
: is_initialized_(false),
pool_(NULL),
layered_pool_(NULL),
is_reused_(false),
ALLOW_THIS_IN_INITIALIZER_LIST(
callback_(this, &ClientSocketHandle::OnIOComplete)),
Expand Down Expand Up @@ -51,10 +49,6 @@ void ClientSocketHandle::ResetInternal(bool cancel) {
group_name_.clear();
is_reused_ = false;
user_callback_ = NULL;
if (layered_pool_) {
pool_->RemoveLayeredPool(layered_pool_);
layered_pool_ = NULL;
}
pool_ = NULL;
idle_time_ = base::TimeDelta();
init_time_ = base::TimeTicks();
Expand All @@ -78,19 +72,6 @@ LoadState ClientSocketHandle::GetLoadState() const {
return pool_->GetLoadState(group_name_, this);
}

bool ClientSocketHandle::IsPoolStalled() const {
return pool_->IsStalled();
}

void ClientSocketHandle::AddLayeredPool(LayeredPool* layered_pool) {
CHECK(layered_pool);
CHECK(!layered_pool_);
if (pool_) {
pool_->AddLayeredPool(layered_pool);
layered_pool_ = layered_pool;
}
}

void ClientSocketHandle::OnIOComplete(int result) {
OldCompletionCallback* callback = user_callback_;
user_callback_ = NULL;
Expand Down
5 changes: 0 additions & 5 deletions net/socket/client_socket_handle.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,6 @@ class NET_EXPORT ClientSocketHandle {
// initialized the ClientSocketHandle.
LoadState GetLoadState() const;

bool IsPoolStalled() const;

void AddLayeredPool(LayeredPool* layered_pool);

// Returns true when Init() has completed successfully.
bool is_initialized() const { return is_initialized_; }

Expand Down Expand Up @@ -168,7 +164,6 @@ class NET_EXPORT ClientSocketHandle {

bool is_initialized_;
ClientSocketPool* pool_;
LayeredPool* layered_pool_;
scoped_ptr<StreamSocket> socket_;
std::string group_name_;
bool is_reused_;
Expand Down
21 changes: 0 additions & 21 deletions net/socket/client_socket_pool.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,17 +29,6 @@ class ClientSocketHandle;
class ClientSocketPoolHistograms;
class StreamSocket;

// ClientSocketPools are layered. This defines an interface for lower level
// socket pools to communicate with higher layer pools.
class LayeredPool {
public:
virtual ~LayeredPool() {}

// Instructs the LayeredPool to close an idle connection. Return true if one
// was closed.
virtual bool CloseOneIdleConnection() = 0;
};

// A ClientSocketPool is used to restrict the number of sockets open at a time.
// It also maintains a list of idle persistent sockets.
//
Expand Down Expand Up @@ -121,10 +110,6 @@ class NET_EXPORT ClientSocketPool {
// the pool. Does not flush any pools wrapped by |this|.
virtual void Flush() = 0;

// Returns true if a new request may hit a per-pool (not per-host) max socket
// limit.
virtual bool IsStalled() const = 0;

// Called to close any idle connections held by the connection manager.
virtual void CloseIdleSockets() = 0;

Expand All @@ -138,12 +123,6 @@ class NET_EXPORT ClientSocketPool {
virtual LoadState GetLoadState(const std::string& group_name,
const ClientSocketHandle* handle) const = 0;

// Adds a LayeredPool on top of |this|.
virtual void AddLayeredPool(LayeredPool* layered_pool) = 0;

// Removes a LayeredPool from |this|.
virtual void RemoveLayeredPool(LayeredPool* layered_pool) = 0;

// Retrieves information on the current state of the pool as a
// DictionaryValue. Caller takes possession of the returned value.
// If |include_nested_pools| is true, the states of any nested
Expand Down
Loading

0 comments on commit a4f8f99

Please sign in to comment.