diff options
author | rch@chromium.org <rch@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-03-20 17:37:28 +0000 |
---|---|---|
committer | rch@chromium.org <rch@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-03-20 17:37:28 +0000 |
commit | 691ee166556fdd0804b0651fd0e219e9bc3789bf (patch) | |
tree | 86ccd78ccfc2eca36ba991210adcb89b3f2858be | |
parent | 775f19f371aa19e7c1d947aaeb8eca43cb281c71 (diff) | |
download | chromium_src-691ee166556fdd0804b0651fd0e219e9bc3789bf.zip chromium_src-691ee166556fdd0804b0651fd0e219e9bc3789bf.tar.gz chromium_src-691ee166556fdd0804b0651fd0e219e9bc3789bf.tar.bz2 |
Revert 118788 - Revert 113405 - Revert 113305 - Revert 113300 - Revert 112134 - Revert 112130 - Close idle connections / 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 prob
ably causing the memory allocator to allocate the pools differently. The std::set is ordered by LayeredPool* which is the address of the LayeredPool (willchan).
Added NET_EXPORT for layered_pool class defintion to fix windows shared compile.
BUG=62364, 92244, 109876, 110368
TEST=
Review URL: http://codereview.chromium.org/9667016
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@127717 0039d316-1c4b-4281-b951-d872f2087c98
21 files changed, 1351 insertions, 37 deletions
diff --git a/net/http/http_network_transaction_spdy21_unittest.cc b/net/http/http_network_transaction_spdy21_unittest.cc index 2998bf0..d528065 100644 --- a/net/http/http_network_transaction_spdy21_unittest.cc +++ b/net/http/http_network_transaction_spdy21_unittest.cc @@ -9387,4 +9387,177 @@ TEST_F(HttpNetworkTransactionSpdy21Test, SendPipelineEvictionFallback) { EXPECT_EQ("hello world", out.response_data); } +TEST_F(HttpNetworkTransactionSpdy21Test, CloseIdleSpdySessionToOpenNewOne) { + HttpStreamFactory::SetNextProtos(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(ASYNC, OK); + ssl1.SetNextProto(SSLClientSocket::kProtoSPDY21); + SSLSocketDataProvider ssl2(ASYNC, OK); + ssl2.SetNextProto(SSLClientSocket::kProtoSPDY21); + 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(ASYNC, ERR_IO_PENDING, 4), + }; + + scoped_ptr<OrderedSocketData> spdy1_data( + new OrderedSocketData( + spdy1_reads, arraysize(spdy1_reads), + spdy1_writes, arraysize(spdy1_writes))); + session_deps.socket_factory.AddSocketDataProvider(spdy1_data.get()); + + 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(ASYNC, ERR_IO_PENDING, 4), + }; + + scoped_ptr<OrderedSocketData> spdy2_data( + new OrderedSocketData( + spdy2_reads, arraysize(spdy2_reads), + spdy2_writes, arraysize(spdy2_writes))); + session_deps.socket_factory.AddSocketDataProvider(spdy2_data.get()); + + 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)); + + TestCompletionCallback 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.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.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.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::SetNextProtos(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 diff --git a/net/http/http_network_transaction_spdy2_unittest.cc b/net/http/http_network_transaction_spdy2_unittest.cc index 7764467..0912dee 100644 --- a/net/http/http_network_transaction_spdy2_unittest.cc +++ b/net/http/http_network_transaction_spdy2_unittest.cc @@ -9386,4 +9386,177 @@ TEST_F(HttpNetworkTransactionSpdy2Test, SendPipelineEvictionFallback) { EXPECT_EQ("hello world", out.response_data); } +TEST_F(HttpNetworkTransactionSpdy2Test, CloseIdleSpdySessionToOpenNewOne) { + HttpStreamFactory::SetNextProtos(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(ASYNC, OK); + ssl1.SetNextProto(SSLClientSocket::kProtoSPDY2); + SSLSocketDataProvider ssl2(ASYNC, OK); + ssl2.SetNextProto(SSLClientSocket::kProtoSPDY2); + 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(ASYNC, ERR_IO_PENDING, 4), + }; + + scoped_ptr<OrderedSocketData> spdy1_data( + new OrderedSocketData( + spdy1_reads, arraysize(spdy1_reads), + spdy1_writes, arraysize(spdy1_writes))); + session_deps.socket_factory.AddSocketDataProvider(spdy1_data.get()); + + 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(ASYNC, ERR_IO_PENDING, 4), + }; + + scoped_ptr<OrderedSocketData> spdy2_data( + new OrderedSocketData( + spdy2_reads, arraysize(spdy2_reads), + spdy2_writes, arraysize(spdy2_writes))); + session_deps.socket_factory.AddSocketDataProvider(spdy2_data.get()); + + 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)); + + TestCompletionCallback 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.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.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.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::SetNextProtos(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 diff --git a/net/http/http_network_transaction_spdy3_unittest.cc b/net/http/http_network_transaction_spdy3_unittest.cc index dd18ee8..a34158a 100644 --- a/net/http/http_network_transaction_spdy3_unittest.cc +++ b/net/http/http_network_transaction_spdy3_unittest.cc @@ -9384,4 +9384,177 @@ TEST_F(HttpNetworkTransactionSpdy3Test, SendPipelineEvictionFallback) { EXPECT_EQ("hello world", out.response_data); } +TEST_F(HttpNetworkTransactionSpdy3Test, CloseIdleSpdySessionToOpenNewOne) { + HttpStreamFactory::SetNextProtos(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(ASYNC, OK); + ssl1.SetNextProto(SSLClientSocket::kProtoSPDY3); + SSLSocketDataProvider ssl2(ASYNC, OK); + ssl2.SetNextProto(SSLClientSocket::kProtoSPDY3); + 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(ASYNC, ERR_IO_PENDING, 4), + }; + + scoped_ptr<OrderedSocketData> spdy1_data( + new OrderedSocketData( + spdy1_reads, arraysize(spdy1_reads), + spdy1_writes, arraysize(spdy1_writes))); + session_deps.socket_factory.AddSocketDataProvider(spdy1_data.get()); + + 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(ASYNC, ERR_IO_PENDING, 4), + }; + + scoped_ptr<OrderedSocketData> spdy2_data( + new OrderedSocketData( + spdy2_reads, arraysize(spdy2_reads), + spdy2_writes, arraysize(spdy2_writes))); + session_deps.socket_factory.AddSocketDataProvider(spdy2_data.get()); + + 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)); + + TestCompletionCallback 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.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.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.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::SetNextProtos(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 diff --git a/net/http/http_proxy_client_socket_pool.cc b/net/http/http_proxy_client_socket_pool.cc index ebea776..bbd2fc9 100644 --- a/net/http/http_proxy_client_socket_pool.cc +++ b/net/http/http_proxy_client_socket_pool.cc @@ -397,9 +397,21 @@ HttpProxyClientSocketPool::HttpProxyClientSocketPool( new HttpProxyConnectJobFactory(transport_pool, ssl_pool, host_resolver, - net_log)) {} + 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); +} -HttpProxyClientSocketPool::~HttpProxyClientSocketPool() {} +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); +} int HttpProxyClientSocketPool::RequestSocket( const std::string& group_name, const void* socket_params, @@ -438,6 +450,12 @@ 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(); } @@ -456,6 +474,14 @@ 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, @@ -486,4 +512,10 @@ ClientSocketPoolHistograms* HttpProxyClientSocketPool::histograms() const { return base_.histograms(); } +bool HttpProxyClientSocketPool::CloseOneIdleConnection() { + if (base_.CloseOneIdleSocket()) + return true; + return base_.CloseOneIdleConnectionInLayeredPool(); +} + } // namespace net diff --git a/net/http/http_proxy_client_socket_pool.h b/net/http/http_proxy_client_socket_pool.h index 483520b..1bebdda 100644 --- a/net/http/http_proxy_client_socket_pool.h +++ b/net/http/http_proxy_client_socket_pool.h @@ -170,7 +170,9 @@ class HttpProxyConnectJob : public ConnectJob { DISALLOW_COPY_AND_ASSIGN(HttpProxyConnectJob); }; -class NET_EXPORT_PRIVATE HttpProxyClientSocketPool : public ClientSocketPool { +class NET_EXPORT_PRIVATE HttpProxyClientSocketPool + : public ClientSocketPool, + public LayeredPool { public: HttpProxyClientSocketPool( int max_sockets, @@ -205,6 +207,8 @@ class NET_EXPORT_PRIVATE HttpProxyClientSocketPool : public ClientSocketPool { virtual void Flush() OVERRIDE; + virtual bool IsStalled() const OVERRIDE; + virtual void CloseIdleSockets() OVERRIDE; virtual int IdleSocketCount() const OVERRIDE; @@ -216,6 +220,10 @@ class NET_EXPORT_PRIVATE HttpProxyClientSocketPool : public ClientSocketPool { 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, @@ -225,6 +233,9 @@ class NET_EXPORT_PRIVATE HttpProxyClientSocketPool : public ClientSocketPool { virtual ClientSocketPoolHistograms* histograms() const OVERRIDE; + // LayeredPool implementation. + virtual bool CloseOneIdleConnection() OVERRIDE; + private: typedef ClientSocketPoolBase<HttpProxySocketParams> PoolBase; diff --git a/net/socket/client_socket_handle.cc b/net/socket/client_socket_handle.cc index 8cc0e90..af3c78a 100644 --- a/net/socket/client_socket_handle.cc +++ b/net/socket/client_socket_handle.cc @@ -17,6 +17,8 @@ namespace net { ClientSocketHandle::ClientSocketHandle() : is_initialized_(false), + pool_(NULL), + layered_pool_(NULL), is_reused_(false), ALLOW_THIS_IN_INITIALIZER_LIST(callback_( base::Bind(&ClientSocketHandle::OnIOComplete, @@ -52,6 +54,10 @@ void ClientSocketHandle::ResetInternal(bool cancel) { group_name_.clear(); is_reused_ = false; user_callback_.Reset(); + if (layered_pool_) { + pool_->RemoveLayeredPool(layered_pool_); + layered_pool_ = NULL; + } pool_ = NULL; idle_time_ = base::TimeDelta(); init_time_ = base::TimeTicks(); @@ -75,6 +81,19 @@ 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) { CompletionCallback callback = user_callback_; user_callback_.Reset(); diff --git a/net/socket/client_socket_handle.h b/net/socket/client_socket_handle.h index 0b9a652..0bdb8ac 100644 --- a/net/socket/client_socket_handle.h +++ b/net/socket/client_socket_handle.h @@ -92,6 +92,10 @@ 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_; } @@ -164,6 +168,7 @@ class NET_EXPORT ClientSocketHandle { bool is_initialized_; ClientSocketPool* pool_; + LayeredPool* layered_pool_; scoped_ptr<StreamSocket> socket_; std::string group_name_; bool is_reused_; diff --git a/net/socket/client_socket_pool.h b/net/socket/client_socket_pool.h index 92f0c413..a13534a 100644 --- a/net/socket/client_socket_pool.h +++ b/net/socket/client_socket_pool.h @@ -29,6 +29,17 @@ 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 NET_EXPORT 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. // @@ -110,6 +121,10 @@ class NET_EXPORT ClientSocketPool { // the pool. Does not flush any pools wrapped by |this|. virtual void Flush() = 0; + // Returns true if a there is currently a request blocked on the + // 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; @@ -123,6 +138,12 @@ 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 diff --git a/net/socket/client_socket_pool_base.cc b/net/socket/client_socket_pool_base.cc index d230ad4..b3417bf 100644 --- a/net/socket/client_socket_pool_base.cc +++ b/net/socket/client_socket_pool_base.cc @@ -207,6 +207,7 @@ ClientSocketPoolBaseHelper::~ClientSocketPoolBaseHelper() { DCHECK(group_map_.empty()); DCHECK(pending_callback_map_.empty()); DCHECK_EQ(0, connecting_socket_count_); + CHECK(higher_layer_pools_.empty()); NetworkChangeNotifier::RemoveIPAddressObserver(this); } @@ -238,6 +239,18 @@ ClientSocketPoolBaseHelper::RemoveRequestFromQueue( return req; } +void ClientSocketPoolBaseHelper::AddLayeredPool(LayeredPool* pool) { + CHECK(pool); + CHECK(!ContainsKey(higher_layer_pools_, pool)); + higher_layer_pools_.insert(pool); +} + +void ClientSocketPoolBaseHelper::RemoveLayeredPool(LayeredPool* pool) { + CHECK(pool); + CHECK(ContainsKey(higher_layer_pools_, pool)); + higher_layer_pools_.erase(pool); +} + int ClientSocketPoolBaseHelper::RequestSocket( const std::string& group_name, const Request* request) { @@ -336,26 +349,46 @@ int ClientSocketPoolBaseHelper::RequestSocketInternal( // Can we make another active socket now? if (!group->HasAvailableSocketSlot(max_sockets_per_group_) && !request->ignore_limits()) { + // TODO(willchan): Consider whether or not we need to close a socket in a + // higher layered group. I don't think this makes sense since we would just + // reuse that socket then if we needed one and wouldn't make it down to this + // layer. request->net_log().AddEvent( NetLog::TYPE_SOCKET_POOL_STALLED_MAX_SOCKETS_PER_GROUP, NULL); return ERR_IO_PENDING; } if (ReachedMaxSocketsLimit() && !request->ignore_limits()) { + // NOTE(mmenke): Wonder if we really need different code for each case + // here. Only reason for them now seems to be preconnects. if (idle_socket_count() > 0) { + // There's an idle socket in this pool. Either that's because there's + // still one in this group, but we got here due to preconnecting bypassing + // idle sockets, or because there's an idle socket in another group. bool closed = CloseOneIdleSocketExceptInGroup(group); if (preconnecting && !closed) return ERR_PRECONNECT_MAX_SOCKET_LIMIT; } else { - // We could check if we really have a stalled group here, but it requires - // a scan of all groups, so just flip a flag here, and do the check later. - request->net_log().AddEvent( - NetLog::TYPE_SOCKET_POOL_STALLED_MAX_SOCKETS, NULL); - return ERR_IO_PENDING; + do { + if (!CloseOneIdleConnectionInLayeredPool()) { + // We could check if we really have a stalled group here, but it + // requires a scan of all groups, so just flip a flag here, and do + // the check later. + request->net_log().AddEvent( + NetLog::TYPE_SOCKET_POOL_STALLED_MAX_SOCKETS, NULL); + return ERR_IO_PENDING; + } + } while (ReachedMaxSocketsLimit()); + + // It is possible that CloseOneIdleConnectionInLayeredPool() has deleted + // our Group (see http://crbug.com/109876), so look it up again + // to be safe. + group = GetOrCreateGroup(group_name); } } - // We couldn't find a socket to reuse, so allocate and connect a new one. + // We couldn't find a socket to reuse, and there's space to allocate one, + // so allocate and connect a new one. scoped_ptr<ConnectJob> connect_job( connect_job_factory_->NewConnectJob(group_name, *request, this)); @@ -617,7 +650,8 @@ DictionaryValue* ClientSocketPoolBaseHelper::GetInfoAsValue( group_dict->Set("connect_jobs", connect_jobs_list); group_dict->SetBoolean("is_stalled", - group->IsStalled(max_sockets_per_group_)); + group->IsStalledOnPoolMaxSockets( + max_sockets_per_group_)); group_dict->SetBoolean("has_backup_job", group->HasBackupJob()); all_groups_dict->SetWithoutPathExpansion(it->first, group_dict); @@ -792,18 +826,22 @@ void ClientSocketPoolBaseHelper::CheckForStalledSocketGroups() { // are not at the |max_sockets_per_group_| limit. Note: for requests with // the same priority, the winner is based on group hash ordering (and not // insertion order). -bool ClientSocketPoolBaseHelper::FindTopStalledGroup(Group** group, - std::string* group_name) { +bool ClientSocketPoolBaseHelper::FindTopStalledGroup( + Group** group, + std::string* group_name) const { + CHECK((group && group_name) || (!group && !group_name)); Group* top_group = NULL; const std::string* top_group_name = NULL; bool has_stalled_group = false; - for (GroupMap::iterator i = group_map_.begin(); + for (GroupMap::const_iterator i = group_map_.begin(); i != group_map_.end(); ++i) { Group* curr_group = i->second; const RequestQueue& queue = curr_group->pending_requests(); if (queue.empty()) continue; - if (curr_group->IsStalled(max_sockets_per_group_)) { + if (curr_group->IsStalledOnPoolMaxSockets(max_sockets_per_group_)) { + if (!group) + return true; has_stalled_group = true; bool has_higher_priority = !top_group || curr_group->TopPendingPriority() < top_group->TopPendingPriority(); @@ -815,8 +853,11 @@ bool ClientSocketPoolBaseHelper::FindTopStalledGroup(Group** group, } if (top_group) { + CHECK(group); *group = top_group; *group_name = *top_group_name; + } else { + CHECK(!has_stalled_group); } return has_stalled_group; } @@ -889,6 +930,25 @@ void ClientSocketPoolBaseHelper::Flush() { AbortAllRequests(); } +bool ClientSocketPoolBaseHelper::IsStalled() const { + // If we are not using |max_sockets_|, then clearly we are not stalled + if ((handed_out_socket_count_ + connecting_socket_count_) < max_sockets_) + return false; + // So in order to be stalled we need to be using |max_sockets_| AND + // we need to have a request that is actually stalled on the global + // socket limit. To find such a request, we look for a group that + // a has more requests that jobs AND where the number of jobs is less + // than |max_sockets_per_group_|. (If the number of jobs is equal to + // |max_sockets_per_group_|, then the request is stalled on the group, + // which does not count.) + for (GroupMap::const_iterator it = group_map_.begin(); + it != group_map_.end(); it++) { + if (it->second->IsStalledOnPoolMaxSockets(max_sockets_per_group_)) + return true; + } + return false; +} + void ClientSocketPoolBaseHelper::RemoveConnectJob(ConnectJob* job, Group* group) { CHECK_GT(connecting_socket_count_, 0); @@ -1025,8 +1085,10 @@ bool ClientSocketPoolBaseHelper::ReachedMaxSocketsLimit() const { return true; } -void ClientSocketPoolBaseHelper::CloseOneIdleSocket() { - CloseOneIdleSocketExceptInGroup(NULL); +bool ClientSocketPoolBaseHelper::CloseOneIdleSocket() { + if (idle_socket_count() == 0) + return false; + return CloseOneIdleSocketExceptInGroup(NULL); } bool ClientSocketPoolBaseHelper::CloseOneIdleSocketExceptInGroup( @@ -1050,9 +1112,18 @@ bool ClientSocketPoolBaseHelper::CloseOneIdleSocketExceptInGroup( } } - if (!exception_group) - LOG(DFATAL) << "No idle socket found to close!."; + return false; +} +bool ClientSocketPoolBaseHelper::CloseOneIdleConnectionInLayeredPool() { + // This pool doesn't have any idle sockets. It's possible that a pool at a + // higher layer is holding one of this sockets active, but it's actually idle. + // Query the higher layers. + for (std::set<LayeredPool*>::const_iterator it = higher_layer_pools_.begin(); + it != higher_layer_pools_.end(); ++it) { + if ((*it)->CloseOneIdleConnection()) + return true; + } return false; } diff --git a/net/socket/client_socket_pool_base.h b/net/socket/client_socket_pool_base.h index f550e42..832c166 100644 --- a/net/socket/client_socket_pool_base.h +++ b/net/socket/client_socket_pool_base.h @@ -28,6 +28,7 @@ #include <map> #include <set> #include <string> +#include <vector> #include "base/basictypes.h" #include "base/memory/ref_counted.h" @@ -239,6 +240,11 @@ class NET_EXPORT_PRIVATE ClientSocketPoolBaseHelper virtual ~ClientSocketPoolBaseHelper(); + // Adds/Removes layered pools. It is expected in the destructor that no + // layered pools remain. + void AddLayeredPool(LayeredPool* pool); + void RemoveLayeredPool(LayeredPool* pool); + // See ClientSocketPool::RequestSocket for documentation on this function. // ClientSocketPoolBaseHelper takes ownership of |request|, which must be // heap allocated. @@ -261,6 +267,9 @@ class NET_EXPORT_PRIVATE ClientSocketPoolBaseHelper // See ClientSocketPool::Flush for documentation on this function. void Flush(); + // See ClientSocketPool::IsStalled for documentation on this function. + bool IsStalled() const; + // See ClientSocketPool::CloseIdleSockets for documentation on this function. void CloseIdleSockets(); @@ -306,6 +315,16 @@ class NET_EXPORT_PRIVATE ClientSocketPoolBaseHelper // sockets that timed out or can't be reused. Made public for testing. void CleanupIdleSockets(bool force); + // Closes one idle socket. Picks the first one encountered. + // TODO(willchan): Consider a better algorithm for doing this. Perhaps we + // should keep an ordered list of idle sockets, and close them in order. + // Requires maintaining more state. It's not clear if it's worth it since + // I'm not sure if we hit this situation often. + bool CloseOneIdleSocket(); + + // Checks layered pools to see if they can close an idle connection. + bool CloseOneIdleConnectionInLayeredPool(); + // See ClientSocketPool::GetInfoAsValue for documentation on this function. base::DictionaryValue* GetInfoAsValue(const std::string& name, const std::string& type) const; @@ -371,7 +390,7 @@ class NET_EXPORT_PRIVATE ClientSocketPoolBaseHelper static_cast<int>(idle_sockets_.size()); } - bool IsStalled(int max_sockets_per_group) const { + bool IsStalledOnPoolMaxSockets(int max_sockets_per_group) const { return HasAvailableSocketSlot(max_sockets_per_group) && pending_requests_.size() > jobs_.size(); } @@ -457,9 +476,9 @@ class NET_EXPORT_PRIVATE ClientSocketPoolBaseHelper // Scans the group map for groups which have an available socket slot and // at least one pending request. Returns true if any groups are stalled, and - // if so, fills |group| and |group_name| with data of the stalled group - // having highest priority. - bool FindTopStalledGroup(Group** group, std::string* group_name); + // if so (and if both |group| and |group_name| are not NULL), fills |group| + // and |group_name| with data of the stalled group having highest priority. + bool FindTopStalledGroup(Group** group, std::string* group_name) const; // Called when timer_ fires. This method scans the idle sockets removing // sockets that timed out or can't be reused. @@ -511,13 +530,6 @@ class NET_EXPORT_PRIVATE ClientSocketPoolBaseHelper static void LogBoundConnectJobToRequest( const NetLog::Source& connect_job_source, const Request* request); - // Closes one idle socket. Picks the first one encountered. - // TODO(willchan): Consider a better algorithm for doing this. Perhaps we - // should keep an ordered list of idle sockets, and close them in order. - // Requires maintaining more state. It's not clear if it's worth it since - // I'm not sure if we hit this situation often. - void CloseOneIdleSocket(); - // Same as CloseOneIdleSocket() except it won't close an idle socket in // |group|. If |group| is NULL, it is ignored. Returns true if it closed a // socket. @@ -582,6 +594,8 @@ class NET_EXPORT_PRIVATE ClientSocketPoolBaseHelper // make sure that they are discarded rather than reused. int pool_generation_number_; + std::set<LayeredPool*> higher_layer_pools_; + base::WeakPtrFactory<ClientSocketPoolBaseHelper> weak_factory_; DISALLOW_COPY_AND_ASSIGN(ClientSocketPoolBaseHelper); @@ -648,6 +662,13 @@ class ClientSocketPoolBase { virtual ~ClientSocketPoolBase() {} // These member functions simply forward to ClientSocketPoolBaseHelper. + void AddLayeredPool(LayeredPool* pool) { + helper_.AddLayeredPool(pool); + } + + void RemoveLayeredPool(LayeredPool* pool) { + helper_.RemoveLayeredPool(pool); + } // RequestSocket bundles up the parameters into a Request and then forwards to // ClientSocketPoolBaseHelper::RequestSocket(). @@ -692,6 +713,10 @@ class ClientSocketPoolBase { return helper_.ReleaseSocket(group_name, socket, id); } + void Flush() { helper_.Flush(); } + + bool IsStalled() const { return helper_.IsStalled(); } + void CloseIdleSockets() { return helper_.CloseIdleSockets(); } int idle_socket_count() const { return helper_.idle_socket_count(); } @@ -740,7 +765,11 @@ class ClientSocketPoolBase { void EnableConnectBackupJobs() { helper_.EnableConnectBackupJobs(); } - void Flush() { helper_.Flush(); } + bool CloseOneIdleSocket() { return helper_.CloseOneIdleSocket(); } + + bool CloseOneIdleConnectionInLayeredPool() { + return helper_.CloseOneIdleConnectionInLayeredPool(); + } private: // This adaptor class exists to bridge the diff --git a/net/socket/client_socket_pool_base_unittest.cc b/net/socket/client_socket_pool_base_unittest.cc index b1c1a99..6c692fe 100644 --- a/net/socket/client_socket_pool_base_unittest.cc +++ b/net/socket/client_socket_pool_base_unittest.cc @@ -4,6 +4,8 @@ #include "net/socket/client_socket_pool_base.h" +#include <vector> + #include "base/bind.h" #include "base/bind_helpers.h" #include "base/callback.h" @@ -28,8 +30,12 @@ #include "net/socket/socket_test_util.h" #include "net/socket/ssl_host_info.h" #include "net/socket/stream_socket.h" +#include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" +using ::testing::Invoke; +using ::testing::Return; + namespace net { namespace { @@ -40,10 +46,18 @@ const net::RequestPriority kDefaultPriority = MEDIUM; class TestSocketParams : public base::RefCounted<TestSocketParams> { public: - bool ignore_limits() { return false; } + TestSocketParams() : ignore_limits_(false) {} + + void set_ignore_limits(bool ignore_limits) { + ignore_limits_ = ignore_limits; + } + bool ignore_limits() { return ignore_limits_; } + private: friend class base::RefCounted<TestSocketParams>; ~TestSocketParams() {} + + bool ignore_limits_; }; typedef ClientSocketPoolBase<TestSocketParams> TestClientSocketPoolBase; @@ -355,12 +369,18 @@ class TestConnectJobFactory public: explicit TestConnectJobFactory(MockClientSocketFactory* client_socket_factory) : job_type_(TestConnectJob::kMockJob), + job_types_(NULL), client_socket_factory_(client_socket_factory) {} virtual ~TestConnectJobFactory() {} void set_job_type(TestConnectJob::JobType job_type) { job_type_ = job_type; } + void set_job_types(std::list<TestConnectJob::JobType>* job_types) { + job_types_ = job_types; + CHECK(!job_types_->empty()); + } + void set_timeout_duration(base::TimeDelta timeout_duration) { timeout_duration_ = timeout_duration; } @@ -371,7 +391,13 @@ class TestConnectJobFactory const std::string& group_name, const TestClientSocketPoolBase::Request& request, ConnectJob::Delegate* delegate) const { - return new TestConnectJob(job_type_, + EXPECT_TRUE(!job_types_ || !job_types_->empty()); + TestConnectJob::JobType job_type = job_type_; + if (job_types_ && !job_types_->empty()) { + job_type = job_types_->front(); + job_types_->pop_front(); + } + return new TestConnectJob(job_type, group_name, request, timeout_duration_, @@ -386,6 +412,7 @@ class TestConnectJobFactory private: TestConnectJob::JobType job_type_; + std::list<TestConnectJob::JobType>* job_types_; base::TimeDelta timeout_duration_; MockClientSocketFactory* const client_socket_factory_; @@ -447,6 +474,10 @@ class TestClientSocketPool : public ClientSocketPool { base_.Flush(); } + virtual bool IsStalled() const OVERRIDE { + return base_.IsStalled(); + } + virtual void CloseIdleSockets() OVERRIDE { base_.CloseIdleSockets(); } @@ -466,6 +497,14 @@ class TestClientSocketPool : public ClientSocketPool { return base_.GetLoadState(group_name, handle); } + virtual void AddLayeredPool(LayeredPool* pool) OVERRIDE { + base_.AddLayeredPool(pool); + } + + virtual void RemoveLayeredPool(LayeredPool* pool) OVERRIDE { + base_.RemoveLayeredPool(pool); + } + virtual DictionaryValue* GetInfoAsValue( const std::string& name, const std::string& type, @@ -499,6 +538,10 @@ class TestClientSocketPool : public ClientSocketPool { void EnableConnectBackupJobs() { base_.EnableConnectBackupJobs(); } + bool CloseOneIdleConnectionInLayeredPool() { + return base_.CloseOneIdleConnectionInLayeredPool(); + } + private: TestClientSocketPoolBase base_; @@ -1164,6 +1207,7 @@ TEST_F(ClientSocketPoolBaseTest, WaitForStalledSocketAtSocketLimit) { ClientSocketHandle stalled_handle; TestCompletionCallback callback; { + EXPECT_FALSE(pool_->IsStalled()); ClientSocketHandle handles[kDefaultMaxSockets]; for (int i = 0; i < kDefaultMaxSockets; ++i) { TestCompletionCallback callback; @@ -1178,6 +1222,7 @@ TEST_F(ClientSocketPoolBaseTest, WaitForStalledSocketAtSocketLimit) { EXPECT_EQ(kDefaultMaxSockets, client_socket_factory_.allocation_count()); EXPECT_EQ(0, pool_->IdleSocketCount()); + EXPECT_FALSE(pool_->IsStalled()); // Now we will hit the socket limit. EXPECT_EQ(ERR_IO_PENDING, stalled_handle.Init("foo", @@ -1186,6 +1231,7 @@ TEST_F(ClientSocketPoolBaseTest, WaitForStalledSocketAtSocketLimit) { callback.callback(), pool_.get(), BoundNetLog())); + EXPECT_TRUE(pool_->IsStalled()); // Dropping out of scope will close all handles and return them to idle. } @@ -2005,9 +2051,9 @@ TEST_F(ClientSocketPoolBaseTest, DisableCleanupTimer) { EXPECT_EQ(1, handle2.socket()->Write(NULL, 1, CompletionCallback())); handle2.Reset(); - // The idle socket timeout value was set to 10 milliseconds. Wait 20 + // The idle socket timeout value was set to 10 milliseconds. Wait 100 // milliseconds so the sockets timeout. - base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(20)); + base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100)); MessageLoop::current()->RunAllPending(); ASSERT_EQ(2, pool_->IdleSocketCount()); @@ -3039,6 +3085,7 @@ TEST_F(ClientSocketPoolBaseTest, RequestSocketsHitMaxSocketLimit) { ASSERT_TRUE(pool_->HasGroup("a")); EXPECT_EQ(kDefaultMaxSockets - 1, pool_->NumConnectJobsInGroup("a")); + EXPECT_FALSE(pool_->IsStalled()); ASSERT_FALSE(pool_->HasGroup("b")); @@ -3047,6 +3094,7 @@ TEST_F(ClientSocketPoolBaseTest, RequestSocketsHitMaxSocketLimit) { ASSERT_TRUE(pool_->HasGroup("b")); EXPECT_EQ(1, pool_->NumConnectJobsInGroup("b")); + EXPECT_FALSE(pool_->IsStalled()); } TEST_F(ClientSocketPoolBaseTest, RequestSocketsCountIdleSockets) { @@ -3365,6 +3413,173 @@ TEST_F(ClientSocketPoolBaseTest, PreconnectWithBackupJob) { EXPECT_EQ(1, pool_->NumActiveSocketsInGroup("a")); } +class MockLayeredPool : public LayeredPool { + public: + MockLayeredPool(TestClientSocketPool* pool, + const std::string& group_name) + : pool_(pool), + params_(new TestSocketParams), + group_name_(group_name), + can_release_connection_(true) { + pool_->AddLayeredPool(this); + } + + ~MockLayeredPool() { + pool_->RemoveLayeredPool(this); + } + + int RequestSocket(TestClientSocketPool* pool) { + return handle_.Init(group_name_, params_, kDefaultPriority, + callback_.callback(), pool, BoundNetLog()); + } + + int RequestSocketWithoutLimits(TestClientSocketPool* pool) { + params_->set_ignore_limits(true); + return handle_.Init(group_name_, params_, kDefaultPriority, + callback_.callback(), pool, BoundNetLog()); + } + + bool ReleaseOneConnection() { + if (!handle_.is_initialized() || !can_release_connection_) { + return false; + } + handle_.socket()->Disconnect(); + handle_.Reset(); + return true; + } + + void set_can_release_connection(bool can_release_connection) { + can_release_connection_ = can_release_connection; + } + + MOCK_METHOD0(CloseOneIdleConnection, bool()); + + private: + TestClientSocketPool* const pool_; + scoped_refptr<TestSocketParams> params_; + ClientSocketHandle handle_; + TestCompletionCallback callback_; + const std::string group_name_; + bool can_release_connection_; +}; + +TEST_F(ClientSocketPoolBaseTest, FailToCloseIdleSocketsNotHeldByLayeredPool) { + CreatePool(kDefaultMaxSockets, kDefaultMaxSocketsPerGroup); + connect_job_factory_->set_job_type(TestConnectJob::kMockJob); + + MockLayeredPool mock_layered_pool(pool_.get(), "foo"); + EXPECT_EQ(OK, mock_layered_pool.RequestSocket(pool_.get())); + EXPECT_CALL(mock_layered_pool, CloseOneIdleConnection()) + .WillOnce(Return(false)); + EXPECT_FALSE(pool_->CloseOneIdleConnectionInLayeredPool()); +} + +TEST_F(ClientSocketPoolBaseTest, ForciblyCloseIdleSocketsHeldByLayeredPool) { + CreatePool(kDefaultMaxSockets, kDefaultMaxSocketsPerGroup); + connect_job_factory_->set_job_type(TestConnectJob::kMockJob); + + MockLayeredPool mock_layered_pool(pool_.get(), "foo"); + EXPECT_EQ(OK, mock_layered_pool.RequestSocket(pool_.get())); + EXPECT_CALL(mock_layered_pool, CloseOneIdleConnection()) + .WillOnce(Invoke(&mock_layered_pool, + &MockLayeredPool::ReleaseOneConnection)); + EXPECT_TRUE(pool_->CloseOneIdleConnectionInLayeredPool()); +} + +// This test exercises the codepath which caused http://crbug.com/109876 +TEST_F(ClientSocketPoolBaseTest, + CloseIdleSocketsHeldByLayeredPoolInSameGroupWhenNeeded) { + CreatePool(2, 2); + std::list<TestConnectJob::JobType> job_types; + job_types.push_back(TestConnectJob::kMockJob); + job_types.push_back(TestConnectJob::kMockJob); + job_types.push_back(TestConnectJob::kMockFailingJob); + job_types.push_back(TestConnectJob::kMockJob); + connect_job_factory_->set_job_types(&job_types); + + ClientSocketHandle handle1; + TestCompletionCallback callback1; + EXPECT_EQ(OK, handle1.Init("group1", + params_, + kDefaultPriority, + callback1.callback(), + pool_.get(), + BoundNetLog())); + + MockLayeredPool mock_layered_pool(pool_.get(), "group2"); + EXPECT_EQ(OK, mock_layered_pool.RequestSocket(pool_.get())); + EXPECT_CALL(mock_layered_pool, CloseOneIdleConnection()) + .WillRepeatedly(Invoke(&mock_layered_pool, + &MockLayeredPool::ReleaseOneConnection)); + mock_layered_pool.set_can_release_connection(false); + + // This connection attempt will fail when the next request causes the + // MockLayeredPool to delete the socket it's holding. This request is + // needed to trigger the destruction of the "group2" Group. + ClientSocketHandle handle3; + TestCompletionCallback callback3; + EXPECT_EQ(ERR_IO_PENDING, handle3.Init("group2", + params_, + kDefaultPriority, + callback3.callback(), + pool_.get(), + BoundNetLog())); + + mock_layered_pool.set_can_release_connection(true); + ClientSocketHandle handle4; + TestCompletionCallback callback4; + EXPECT_EQ(OK, handle4.Init("group2", + params_, + kDefaultPriority, + callback4.callback(), + pool_.get(), + BoundNetLog())); +} + +TEST_F(ClientSocketPoolBaseTest, CloseIdleSocketsHeldByLayeredPoolWhenNeeded) { + CreatePool(1, 1); + connect_job_factory_->set_job_type(TestConnectJob::kMockJob); + + MockLayeredPool mock_layered_pool(pool_.get(), "foo"); + EXPECT_EQ(OK, mock_layered_pool.RequestSocket(pool_.get())); + EXPECT_CALL(mock_layered_pool, CloseOneIdleConnection()) + .WillOnce(Invoke(&mock_layered_pool, + &MockLayeredPool::ReleaseOneConnection)); + ClientSocketHandle handle; + TestCompletionCallback callback; + EXPECT_EQ(OK, handle.Init("a", + params_, + kDefaultPriority, + callback.callback(), + pool_.get(), + BoundNetLog())); +} + +TEST_F(ClientSocketPoolBaseTest, + CloseMultipleIdleSocketsHeldByLayeredPoolWhenNeeded) { + CreatePool(1, 1); + connect_job_factory_->set_job_type(TestConnectJob::kMockJob); + + MockLayeredPool mock_layered_pool1(pool_.get(), "foo"); + EXPECT_EQ(OK, mock_layered_pool1.RequestSocket(pool_.get())); + EXPECT_CALL(mock_layered_pool1, CloseOneIdleConnection()) + .WillRepeatedly(Invoke(&mock_layered_pool1, + &MockLayeredPool::ReleaseOneConnection)); + MockLayeredPool mock_layered_pool2(pool_.get(), "bar"); + EXPECT_EQ(OK, mock_layered_pool2.RequestSocketWithoutLimits(pool_.get())); + EXPECT_CALL(mock_layered_pool2, CloseOneIdleConnection()) + .WillRepeatedly(Invoke(&mock_layered_pool2, + &MockLayeredPool::ReleaseOneConnection)); + ClientSocketHandle handle; + TestCompletionCallback callback; + EXPECT_EQ(OK, handle.Init("a", + params_, + kDefaultPriority, + callback.callback(), + pool_.get(), + BoundNetLog())); +} + } // namespace } // namespace net diff --git a/net/socket/socks_client_socket_pool.cc b/net/socket/socks_client_socket_pool.cc index ae963bf..a7c7ecb 100644 --- a/net/socket/socks_client_socket_pool.cc +++ b/net/socket/socks_client_socket_pool.cc @@ -199,9 +199,16 @@ SOCKSClientSocketPool::SOCKSClientSocketPool( new SOCKSConnectJobFactory(transport_pool, host_resolver, net_log)) { + // We should always have a |transport_pool_| except in unit tests. + if (transport_pool_) + transport_pool_->AddLayeredPool(this); } -SOCKSClientSocketPool::~SOCKSClientSocketPool() {} +SOCKSClientSocketPool::~SOCKSClientSocketPool() { + // We should always have a |transport_pool_| except in unit tests. + if (transport_pool_) + transport_pool_->RemoveLayeredPool(this); +} int SOCKSClientSocketPool::RequestSocket( const std::string& group_name, const void* socket_params, @@ -239,6 +246,10 @@ void SOCKSClientSocketPool::Flush() { base_.Flush(); } +bool SOCKSClientSocketPool::IsStalled() const { + return base_.IsStalled() || transport_pool_->IsStalled(); +} + void SOCKSClientSocketPool::CloseIdleSockets() { base_.CloseIdleSockets(); } @@ -257,6 +268,14 @@ LoadState SOCKSClientSocketPool::GetLoadState( return base_.GetLoadState(group_name, handle); } +void SOCKSClientSocketPool::AddLayeredPool(LayeredPool* layered_pool) { + base_.AddLayeredPool(layered_pool); +} + +void SOCKSClientSocketPool::RemoveLayeredPool(LayeredPool* layered_pool) { + base_.RemoveLayeredPool(layered_pool); +} + DictionaryValue* SOCKSClientSocketPool::GetInfoAsValue( const std::string& name, const std::string& type, @@ -280,4 +299,10 @@ ClientSocketPoolHistograms* SOCKSClientSocketPool::histograms() const { return base_.histograms(); }; +bool SOCKSClientSocketPool::CloseOneIdleConnection() { + if (base_.CloseOneIdleSocket()) + return true; + return base_.CloseOneIdleConnectionInLayeredPool(); +} + } // namespace net diff --git a/net/socket/socks_client_socket_pool.h b/net/socket/socks_client_socket_pool.h index 3fc7df6..8583d96 100644 --- a/net/socket/socks_client_socket_pool.h +++ b/net/socket/socks_client_socket_pool.h @@ -105,7 +105,8 @@ class SOCKSConnectJob : public ConnectJob { DISALLOW_COPY_AND_ASSIGN(SOCKSConnectJob); }; -class NET_EXPORT_PRIVATE SOCKSClientSocketPool : public ClientSocketPool { +class NET_EXPORT_PRIVATE SOCKSClientSocketPool + : public ClientSocketPool, public LayeredPool { public: SOCKSClientSocketPool( int max_sockets, @@ -139,6 +140,8 @@ class NET_EXPORT_PRIVATE SOCKSClientSocketPool : public ClientSocketPool { virtual void Flush() OVERRIDE; + virtual bool IsStalled() const OVERRIDE; + virtual void CloseIdleSockets() OVERRIDE; virtual int IdleSocketCount() const OVERRIDE; @@ -150,6 +153,10 @@ class NET_EXPORT_PRIVATE SOCKSClientSocketPool : public ClientSocketPool { 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, @@ -159,6 +166,9 @@ class NET_EXPORT_PRIVATE SOCKSClientSocketPool : public ClientSocketPool { virtual ClientSocketPoolHistograms* histograms() const OVERRIDE; + // LayeredPool implementation. + virtual bool CloseOneIdleConnection() OVERRIDE; + private: typedef ClientSocketPoolBase<SOCKSSocketParams> PoolBase; diff --git a/net/socket/ssl_client_socket_pool.cc b/net/socket/ssl_client_socket_pool.cc index 0c96546..53a84c0 100644 --- a/net/socket/ssl_client_socket_pool.cc +++ b/net/socket/ssl_client_socket_pool.cc @@ -479,9 +479,21 @@ SSLClientSocketPool::SSLClientSocketPool( ssl_config_service_(ssl_config_service) { if (ssl_config_service_) ssl_config_service_->AddObserver(this); + if (transport_pool_) + transport_pool_->AddLayeredPool(this); + if (socks_pool_) + socks_pool_->AddLayeredPool(this); + if (http_proxy_pool_) + http_proxy_pool_->AddLayeredPool(this); } SSLClientSocketPool::~SSLClientSocketPool() { + if (http_proxy_pool_) + http_proxy_pool_->RemoveLayeredPool(this); + if (socks_pool_) + socks_pool_->RemoveLayeredPool(this); + if (transport_pool_) + transport_pool_->RemoveLayeredPool(this); if (ssl_config_service_) ssl_config_service_->RemoveObserver(this); } @@ -534,6 +546,13 @@ void SSLClientSocketPool::Flush() { base_.Flush(); } +bool SSLClientSocketPool::IsStalled() const { + return base_.IsStalled() || + (transport_pool_ && transport_pool_->IsStalled()) || + (socks_pool_ && socks_pool_->IsStalled()) || + (http_proxy_pool_ && http_proxy_pool_->IsStalled()); +} + void SSLClientSocketPool::CloseIdleSockets() { base_.CloseIdleSockets(); } @@ -552,6 +571,14 @@ LoadState SSLClientSocketPool::GetLoadState( return base_.GetLoadState(group_name, handle); } +void SSLClientSocketPool::AddLayeredPool(LayeredPool* layered_pool) { + base_.AddLayeredPool(layered_pool); +} + +void SSLClientSocketPool::RemoveLayeredPool(LayeredPool* layered_pool) { + base_.RemoveLayeredPool(layered_pool); +} + DictionaryValue* SSLClientSocketPool::GetInfoAsValue( const std::string& name, const std::string& type, @@ -591,4 +618,10 @@ void SSLClientSocketPool::OnSSLConfigChanged() { Flush(); } +bool SSLClientSocketPool::CloseOneIdleConnection() { + if (base_.CloseOneIdleSocket()) + return true; + return base_.CloseOneIdleConnectionInLayeredPool(); +} + } // namespace net diff --git a/net/socket/ssl_client_socket_pool.h b/net/socket/ssl_client_socket_pool.h index bd667ff..730f0e3 100644 --- a/net/socket/ssl_client_socket_pool.h +++ b/net/socket/ssl_client_socket_pool.h @@ -166,6 +166,7 @@ class SSLConnectJob : public ConnectJob { class NET_EXPORT_PRIVATE SSLClientSocketPool : public ClientSocketPool, + public LayeredPool, public SSLConfigService::Observer { public: // Only the pools that will be used are required. i.e. if you never @@ -211,6 +212,8 @@ class NET_EXPORT_PRIVATE SSLClientSocketPool virtual void Flush() OVERRIDE; + virtual bool IsStalled() const OVERRIDE; + virtual void CloseIdleSockets() OVERRIDE; virtual int IdleSocketCount() const OVERRIDE; @@ -222,6 +225,10 @@ class NET_EXPORT_PRIVATE SSLClientSocketPool 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, @@ -231,6 +238,9 @@ class NET_EXPORT_PRIVATE SSLClientSocketPool virtual ClientSocketPoolHistograms* histograms() const OVERRIDE; + // LayeredPool implementation. + virtual bool CloseOneIdleConnection() OVERRIDE; + private: typedef ClientSocketPoolBase<SSLSocketParams> PoolBase; diff --git a/net/socket/transport_client_socket_pool.cc b/net/socket/transport_client_socket_pool.cc index d11099f..a00e810 100644 --- a/net/socket/transport_client_socket_pool.cc +++ b/net/socket/transport_client_socket_pool.cc @@ -449,6 +449,10 @@ void TransportClientSocketPool::Flush() { base_.Flush(); } +bool TransportClientSocketPool::IsStalled() const { + return base_.IsStalled(); +} + void TransportClientSocketPool::CloseIdleSockets() { base_.CloseIdleSockets(); } @@ -467,6 +471,14 @@ LoadState TransportClientSocketPool::GetLoadState( return base_.GetLoadState(group_name, handle); } +void TransportClientSocketPool::AddLayeredPool(LayeredPool* layered_pool) { + base_.AddLayeredPool(layered_pool); +} + +void TransportClientSocketPool::RemoveLayeredPool(LayeredPool* layered_pool) { + base_.RemoveLayeredPool(layered_pool); +} + DictionaryValue* TransportClientSocketPool::GetInfoAsValue( const std::string& name, const std::string& type, diff --git a/net/socket/transport_client_socket_pool.h b/net/socket/transport_client_socket_pool.h index a4b4143..ef3f6fa 100644 --- a/net/socket/transport_client_socket_pool.h +++ b/net/socket/transport_client_socket_pool.h @@ -157,6 +157,7 @@ class NET_EXPORT_PRIVATE TransportClientSocketPool : public ClientSocketPool { StreamSocket* socket, int id) OVERRIDE; virtual void Flush() OVERRIDE; + virtual bool IsStalled() const OVERRIDE; virtual void CloseIdleSockets() OVERRIDE; virtual int IdleSocketCount() const OVERRIDE; virtual int IdleSocketCountInGroup( @@ -164,6 +165,8 @@ class NET_EXPORT_PRIVATE TransportClientSocketPool : public ClientSocketPool { virtual LoadState GetLoadState( 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, diff --git a/net/spdy/spdy_session.cc b/net/spdy/spdy_session.cc index ddfa2a2..181d32f 100644 --- a/net/spdy/spdy_session.cc +++ b/net/spdy/spdy_session.cc @@ -394,6 +394,7 @@ net::Error SpdySession::InitializeWithSocket( state_ = CONNECTED; connection_.reset(connection); + connection_->AddLayeredPool(this); is_secure_ = is_secure; certificate_error_code_ = certificate_error_code; @@ -1191,6 +1192,18 @@ int SpdySession::GetLocalAddress(IPEndPoint* address) const { return connection_->socket()->GetLocalAddress(address); } +bool SpdySession::CloseOneIdleConnection() { + if (spdy_session_pool_ && num_active_streams() == 0) { + bool ret = HasOneRef(); + // Will remove a reference to this. + RemoveFromPool(); + // Since the underlying socket is only returned when |this| is destroyed + // we should only return true if RemoveFromPool() removed the last ref. + return ret; + } + return false; +} + void SpdySession::ActivateStream(SpdyStream* stream) { const spdy::SpdyStreamId id = stream->stream_id(); DCHECK(!IsStreamActive(id)); diff --git a/net/spdy/spdy_session.h b/net/spdy/spdy_session.h index f56187b..4a161c7 100644 --- a/net/spdy/spdy_session.h +++ b/net/spdy/spdy_session.h @@ -50,7 +50,8 @@ class SpdyStream; class SSLInfo; class NET_EXPORT SpdySession : public base::RefCounted<SpdySession>, - public spdy::BufferedSpdyFramerVisitorInterface { + public spdy::BufferedSpdyFramerVisitorInterface, + public LayeredPool { public: // Create a new SpdySession. // |host_port_proxy_pair| is the host/port that this session connects to, and @@ -266,6 +267,9 @@ class NET_EXPORT SpdySession : public base::RefCounted<SpdySession>, int GetProtocolVersion() const; + // LayeredPool implementation. + virtual bool CloseOneIdleConnection() OVERRIDE; + private: friend class base::RefCounted<SpdySession>; @@ -273,9 +277,11 @@ class NET_EXPORT SpdySession : public base::RefCounted<SpdySession>, FRIEND_TEST_ALL_PREFIXES(SpdySessionSpdy2Test, Ping); FRIEND_TEST_ALL_PREFIXES(SpdySessionSpdy2Test, FailedPing); FRIEND_TEST_ALL_PREFIXES(SpdySessionSpdy2Test, GetActivePushStream); + FRIEND_TEST_ALL_PREFIXES(SpdySessionSpdy2Test, CloseOneIdleConnection); FRIEND_TEST_ALL_PREFIXES(SpdySessionSpdy3Test, Ping); FRIEND_TEST_ALL_PREFIXES(SpdySessionSpdy3Test, FailedPing); FRIEND_TEST_ALL_PREFIXES(SpdySessionSpdy3Test, GetActivePushStream); + FRIEND_TEST_ALL_PREFIXES(SpdySessionSpdy3Test, CloseOneIdleConnection); struct PendingCreateStream { PendingCreateStream(const GURL& url, RequestPriority priority, diff --git a/net/spdy/spdy_session_spdy2_unittest.cc b/net/spdy/spdy_session_spdy2_unittest.cc index 346c057..4e0c3b8 100644 --- a/net/spdy/spdy_session_spdy2_unittest.cc +++ b/net/spdy/spdy_session_spdy2_unittest.cc @@ -1076,4 +1076,144 @@ TEST_F(SpdySessionSpdy2Test, CloseSessionOnError) { EXPECT_EQ(ERR_CONNECTION_CLOSED, request_params->status()); } +TEST_F(SpdySessionSpdy2Test, CloseOneIdleConnection) { + MockHostResolver host_resolver; + CapturingBoundNetLog log(CapturingNetLog::kUnbounded); + ClientSocketPoolHistograms tcp_histograms(""); + MockClientSocketFactory socket_factory; + MockConnect connect_data(SYNCHRONOUS, OK); + MockRead reads[] = { + MockRead(SYNCHRONOUS, ERR_IO_PENDING) // Stall forever. + }; + StaticSocketDataProvider data(reads, arraysize(reads), NULL, 0); + data.set_connect_data(connect_data); + socket_factory.AddSocketDataProvider(&data); + socket_factory.AddSocketDataProvider(&data); + socket_factory.AddSocketDataProvider(&data); + socket_factory.AddSocketDataProvider(&data); + socket_factory.AddSocketDataProvider(&data); + socket_factory.AddSocketDataProvider(&data); + TransportClientSocketPool pool( + 3, 2, + &tcp_histograms, + &host_resolver, + &socket_factory, NULL); + // Now if I check out 1 socket from 3 different groups, the next request + // will leave us stalled. + + TestCompletionCallback callback1; + HostPortPair host_port1("1.com", 80); + scoped_refptr<TransportSocketParams> params1( + new TransportSocketParams(host_port1, MEDIUM, false, false)); + scoped_ptr<ClientSocketHandle> connection1(new ClientSocketHandle); + EXPECT_EQ(ERR_IO_PENDING, + connection1->Init(host_port1.ToString(), params1, MEDIUM, + callback1.callback(), &pool, log.bound())); + EXPECT_EQ(OK, callback1.WaitForResult()); + EXPECT_FALSE(pool.IsStalled()); + EXPECT_TRUE(connection1->is_initialized()); + EXPECT_TRUE(connection1->socket()); + + TestCompletionCallback callback2; + HostPortPair host_port2("2.com", 80); + scoped_refptr<TransportSocketParams> params2( + new TransportSocketParams(host_port2, MEDIUM, false, false)); + scoped_ptr<ClientSocketHandle> connection2(new ClientSocketHandle); + EXPECT_EQ(ERR_IO_PENDING, + connection2->Init(host_port2.ToString(), params2, MEDIUM, + callback2.callback(), &pool, log.bound())); + EXPECT_EQ(OK, callback2.WaitForResult()); + EXPECT_FALSE(pool.IsStalled()); + + TestCompletionCallback callback3; + HostPortPair host_port3("3.com", 80); + scoped_refptr<TransportSocketParams> params3( + new TransportSocketParams(host_port3, MEDIUM, false, false)); + scoped_ptr<ClientSocketHandle> connection3(new ClientSocketHandle); + EXPECT_EQ(ERR_IO_PENDING, + connection3->Init(host_port3.ToString(), params3, MEDIUM, + callback3.callback(), &pool, log.bound())); + EXPECT_EQ(OK, callback3.WaitForResult()); + EXPECT_FALSE(pool.IsStalled()); + + TestCompletionCallback callback4; + HostPortPair host_port4("4.com", 80); + scoped_refptr<TransportSocketParams> params4( + new TransportSocketParams(host_port4, MEDIUM, false, false)); + scoped_ptr<ClientSocketHandle> connection4(new ClientSocketHandle); + EXPECT_EQ(ERR_IO_PENDING, + connection4->Init(host_port4.ToString(), params4, MEDIUM, + callback4.callback(), &pool, log.bound())); + EXPECT_TRUE(pool.IsStalled()); + + // Return 1 socket to the pool so that we are no longer stalled + connection3->socket()->Disconnect(); + connection3->Reset(); + EXPECT_EQ(OK, callback4.WaitForResult()); + EXPECT_FALSE(pool.IsStalled()); + + // Now, wrap one of the sockets in a SpdySession + HttpServerPropertiesImpl props; + SpdySessionPool spdy_session_pool(&host_resolver, NULL, &props); + HostPortProxyPair pair1(host_port1, ProxyServer::Direct()); + EXPECT_FALSE(spdy_session_pool.HasSession(pair1)); + scoped_refptr<SpdySession> session1 = + spdy_session_pool.Get(pair1, log.bound()); + EXPECT_TRUE(spdy_session_pool.HasSession(pair1)); + EXPECT_EQ(OK, + session1->InitializeWithSocket(connection1.release(), false, OK)); + session1 = NULL; + EXPECT_TRUE(spdy_session_pool.HasSession(pair1)); + + // The SpdySession is now idle. When we request the next socket from the + // transport pool, the session will be closed via CloseOneIdleConnection(). + TestCompletionCallback callback5; + HostPortPair host_port5("5.com", 80); + scoped_refptr<TransportSocketParams> params5( + new TransportSocketParams(host_port5, MEDIUM, false, false)); + scoped_ptr<ClientSocketHandle> connection5(new ClientSocketHandle); + EXPECT_EQ(ERR_IO_PENDING, + connection5->Init(host_port5.ToString(), params5, MEDIUM, + callback5.callback(), &pool, log.bound())); + EXPECT_FALSE(pool.IsStalled()); + EXPECT_EQ(OK, callback5.WaitForResult()); + EXPECT_FALSE(spdy_session_pool.HasSession(pair1)); + EXPECT_FALSE(pool.IsStalled()); + + // Now, wrap one of the sockets in a SpdySession + HostPortProxyPair pair2(host_port2, ProxyServer::Direct()); + EXPECT_FALSE(spdy_session_pool.HasSession(pair2)); + scoped_refptr<SpdySession> session2 = + spdy_session_pool.Get(pair2, log.bound()); + EXPECT_TRUE(spdy_session_pool.HasSession(pair2)); + EXPECT_EQ(OK, + session2->InitializeWithSocket(connection2.release(), false, OK)); + + // Manually remove the socket from the pool. This does *not* return the + // transport socket. It will be returned only when the SpdySession is + // destructed. + session2->RemoveFromPool(); + EXPECT_FALSE(spdy_session_pool.HasSession(pair2)); + + // Although there are no active streams on the session, the pool does not + // hold a reference. This means that CloseOneIdleConnection should not + // return true, and this request should stall. + TestCompletionCallback callback6; + HostPortPair host_port6("6.com", 80); + scoped_refptr<TransportSocketParams> params6( + new TransportSocketParams(host_port5, MEDIUM, false, false)); + scoped_ptr<ClientSocketHandle> connection6(new ClientSocketHandle); + EXPECT_EQ(ERR_IO_PENDING, + connection6->Init(host_port6.ToString(), params6, MEDIUM, + callback6.callback(), &pool, log.bound())); + EXPECT_TRUE(pool.IsStalled()); + + // But now if we drop our reference to the session, it will be destructed + // and the transport socket will return to the pool, unblocking this + // request. + session2 = NULL; + EXPECT_EQ(OK, callback6.WaitForResult()); + EXPECT_FALSE(pool.IsStalled()); +} + } // namespace net diff --git a/net/spdy/spdy_session_spdy3_unittest.cc b/net/spdy/spdy_session_spdy3_unittest.cc index f516ed2..a40834a 100644 --- a/net/spdy/spdy_session_spdy3_unittest.cc +++ b/net/spdy/spdy_session_spdy3_unittest.cc @@ -1152,4 +1152,144 @@ TEST_F(SpdySessionSpdy3Test, CloseSessionOnError) { EXPECT_EQ(ERR_CONNECTION_CLOSED, request_params->status()); } +TEST_F(SpdySessionSpdy3Test, CloseOneIdleConnection) { + MockHostResolver host_resolver; + CapturingBoundNetLog log(CapturingNetLog::kUnbounded); + ClientSocketPoolHistograms tcp_histograms(""); + MockClientSocketFactory socket_factory; + MockConnect connect_data(SYNCHRONOUS, OK); + MockRead reads[] = { + MockRead(SYNCHRONOUS, ERR_IO_PENDING) // Stall forever. + }; + StaticSocketDataProvider data(reads, arraysize(reads), NULL, 0); + data.set_connect_data(connect_data); + socket_factory.AddSocketDataProvider(&data); + socket_factory.AddSocketDataProvider(&data); + socket_factory.AddSocketDataProvider(&data); + socket_factory.AddSocketDataProvider(&data); + socket_factory.AddSocketDataProvider(&data); + socket_factory.AddSocketDataProvider(&data); + TransportClientSocketPool pool( + 3, 2, + &tcp_histograms, + &host_resolver, + &socket_factory, NULL); + // Now if I check out 1 socket from 3 different groups, the next request + // will leave us stalled. + + TestCompletionCallback callback1; + HostPortPair host_port1("1.com", 80); + scoped_refptr<TransportSocketParams> params1( + new TransportSocketParams(host_port1, MEDIUM, false, false)); + scoped_ptr<ClientSocketHandle> connection1(new ClientSocketHandle); + EXPECT_EQ(ERR_IO_PENDING, + connection1->Init(host_port1.ToString(), params1, MEDIUM, + callback1.callback(), &pool, log.bound())); + EXPECT_EQ(OK, callback1.WaitForResult()); + EXPECT_FALSE(pool.IsStalled()); + EXPECT_TRUE(connection1->is_initialized()); + EXPECT_TRUE(connection1->socket()); + + TestCompletionCallback callback2; + HostPortPair host_port2("2.com", 80); + scoped_refptr<TransportSocketParams> params2( + new TransportSocketParams(host_port2, MEDIUM, false, false)); + scoped_ptr<ClientSocketHandle> connection2(new ClientSocketHandle); + EXPECT_EQ(ERR_IO_PENDING, + connection2->Init(host_port2.ToString(), params2, MEDIUM, + callback2.callback(), &pool, log.bound())); + EXPECT_EQ(OK, callback2.WaitForResult()); + EXPECT_FALSE(pool.IsStalled()); + + TestCompletionCallback callback3; + HostPortPair host_port3("3.com", 80); + scoped_refptr<TransportSocketParams> params3( + new TransportSocketParams(host_port3, MEDIUM, false, false)); + scoped_ptr<ClientSocketHandle> connection3(new ClientSocketHandle); + EXPECT_EQ(ERR_IO_PENDING, + connection3->Init(host_port3.ToString(), params3, MEDIUM, + callback3.callback(), &pool, log.bound())); + EXPECT_EQ(OK, callback3.WaitForResult()); + EXPECT_FALSE(pool.IsStalled()); + + TestCompletionCallback callback4; + HostPortPair host_port4("4.com", 80); + scoped_refptr<TransportSocketParams> params4( + new TransportSocketParams(host_port4, MEDIUM, false, false)); + scoped_ptr<ClientSocketHandle> connection4(new ClientSocketHandle); + EXPECT_EQ(ERR_IO_PENDING, + connection4->Init(host_port4.ToString(), params4, MEDIUM, + callback4.callback(), &pool, log.bound())); + EXPECT_TRUE(pool.IsStalled()); + + // Return 1 socket to the pool so that we are no longer stalled + connection3->socket()->Disconnect(); + connection3->Reset(); + EXPECT_EQ(OK, callback4.WaitForResult()); + EXPECT_FALSE(pool.IsStalled()); + + // Now, wrap one of the sockets in a SpdySession + HttpServerPropertiesImpl props; + SpdySessionPool spdy_session_pool(&host_resolver, NULL, &props); + HostPortProxyPair pair1(host_port1, ProxyServer::Direct()); + EXPECT_FALSE(spdy_session_pool.HasSession(pair1)); + scoped_refptr<SpdySession> session1 = + spdy_session_pool.Get(pair1, log.bound()); + EXPECT_TRUE(spdy_session_pool.HasSession(pair1)); + EXPECT_EQ(OK, + session1->InitializeWithSocket(connection1.release(), false, OK)); + session1 = NULL; + EXPECT_TRUE(spdy_session_pool.HasSession(pair1)); + + // The SpdySession is now idle. When we request the next socket from the + // transport pool, the session will be closed via CloseOneIdleConnection(). + TestCompletionCallback callback5; + HostPortPair host_port5("5.com", 80); + scoped_refptr<TransportSocketParams> params5( + new TransportSocketParams(host_port5, MEDIUM, false, false)); + scoped_ptr<ClientSocketHandle> connection5(new ClientSocketHandle); + EXPECT_EQ(ERR_IO_PENDING, + connection5->Init(host_port5.ToString(), params5, MEDIUM, + callback5.callback(), &pool, log.bound())); + EXPECT_FALSE(pool.IsStalled()); + EXPECT_EQ(OK, callback5.WaitForResult()); + EXPECT_FALSE(spdy_session_pool.HasSession(pair1)); + EXPECT_FALSE(pool.IsStalled()); + + // Now, wrap one of the sockets in a SpdySession + HostPortProxyPair pair2(host_port2, ProxyServer::Direct()); + EXPECT_FALSE(spdy_session_pool.HasSession(pair2)); + scoped_refptr<SpdySession> session2 = + spdy_session_pool.Get(pair2, log.bound()); + EXPECT_TRUE(spdy_session_pool.HasSession(pair2)); + EXPECT_EQ(OK, + session2->InitializeWithSocket(connection2.release(), false, OK)); + + // Manually remove the socket from the pool. This does *not* return the + // transport socket. It will be returned only when the SpdySession is + // destructed. + session2->RemoveFromPool(); + EXPECT_FALSE(spdy_session_pool.HasSession(pair2)); + + // Although there are no active streams on the session, the pool does not + // hold a reference. This means that CloseOneIdleConnection should not + // return true, and this request should stall. + TestCompletionCallback callback6; + HostPortPair host_port6("6.com", 80); + scoped_refptr<TransportSocketParams> params6( + new TransportSocketParams(host_port5, MEDIUM, false, false)); + scoped_ptr<ClientSocketHandle> connection6(new ClientSocketHandle); + EXPECT_EQ(ERR_IO_PENDING, + connection6->Init(host_port6.ToString(), params6, MEDIUM, + callback6.callback(), &pool, log.bound())); + EXPECT_TRUE(pool.IsStalled()); + + // But now if we drop our reference to the session, it will be destructed + // and the transport socket will return to the pool, unblocking this + // request. + session2 = NULL; + EXPECT_EQ(OK, callback6.WaitForResult()); + EXPECT_FALSE(pool.IsStalled()); +} + } // namespace net |