diff options
author | rch@chromium.org <rch@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-04-06 17:26:40 +0000 |
---|---|---|
committer | rch@chromium.org <rch@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-04-06 17:26:40 +0000 |
commit | 2c75400ac98ab4d0caae109f6ea8b1dc979aa24a (patch) | |
tree | dfaf46cd3bc537d084489650e18303e0f8bae731 | |
parent | 7429426bdc631be48434d85191215f5ca6d703f4 (diff) | |
download | chromium_src-2c75400ac98ab4d0caae109f6ea8b1dc979aa24a.zip chromium_src-2c75400ac98ab4d0caae109f6ea8b1dc979aa24a.tar.gz chromium_src-2c75400ac98ab4d0caae109f6ea8b1dc979aa24a.tar.bz2 |
Reverting again ... More crashes, and the instrumentation did not appear to help
Revert 130129 - Revert 129034 - Revert 127893 - Revert 127730 - Revert 127717 - 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, 119847
TEST=
Review URL: http://codereview.chromium.org/9861032
TBR=rch@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10006036
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@131145 0039d316-1c4b-4281-b951-d872f2087c98
22 files changed, 38 insertions, 1425 deletions
diff --git a/net/http/http_network_transaction_spdy21_unittest.cc b/net/http/http_network_transaction_spdy21_unittest.cc index c77e64e..23f1eb3 100644 --- a/net/http/http_network_transaction_spdy21_unittest.cc +++ b/net/http/http_network_transaction_spdy21_unittest.cc @@ -9552,185 +9552,4 @@ 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( - HttpNetworkSession::NORMAL_SOCKET_POOL); - int old_max_sockets_per_proxy_server = - ClientSocketPoolManager::max_sockets_per_proxy_server( - HttpNetworkSession::NORMAL_SOCKET_POOL); - int old_max_sockets_per_pool = - ClientSocketPoolManager::max_sockets_per_pool( - HttpNetworkSession::NORMAL_SOCKET_POOL); - ClientSocketPoolManager::set_max_sockets_per_group( - HttpNetworkSession::NORMAL_SOCKET_POOL, 1); - ClientSocketPoolManager::set_max_sockets_per_proxy_server( - HttpNetworkSession::NORMAL_SOCKET_POOL, 1); - ClientSocketPoolManager::set_max_sockets_per_pool( - HttpNetworkSession::NORMAL_SOCKET_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(kProtoSPDY21); - SSLSocketDataProvider ssl2(ASYNC, OK); - ssl2.SetNextProto(kProtoSPDY21); - session_deps.socket_factory.AddSSLSocketDataProvider(&ssl1); - session_deps.socket_factory.AddSSLSocketDataProvider(&ssl2); - - scoped_ptr<SpdyFrame> host1_req(ConstructSpdyGet( - "https://www.a.com", false, 1, LOWEST)); - MockWrite spdy1_writes[] = { - CreateMockWrite(*host1_req, 1), - }; - scoped_ptr<SpdyFrame> host1_resp(ConstructSpdyGetSynReply(NULL, 0, 1)); - scoped_ptr<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<SpdyFrame> host2_req(ConstructSpdyGet( - "https://www.b.com", false, 1, LOWEST)); - MockWrite spdy2_writes[] = { - CreateMockWrite(*host2_req, 1), - }; - scoped_ptr<SpdyFrame> host2_resp(ConstructSpdyGetSynReply(NULL, 0, 1)); - scoped_ptr<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( - HttpNetworkSession::NORMAL_SOCKET_POOL, old_max_sockets_per_pool); - ClientSocketPoolManager::set_max_sockets_per_proxy_server( - HttpNetworkSession::NORMAL_SOCKET_POOL, old_max_sockets_per_proxy_server); - ClientSocketPoolManager::set_max_sockets_per_group( - HttpNetworkSession::NORMAL_SOCKET_POOL, 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 f52217b..17781f1 100644 --- a/net/http/http_network_transaction_spdy2_unittest.cc +++ b/net/http/http_network_transaction_spdy2_unittest.cc @@ -9551,185 +9551,4 @@ 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( - HttpNetworkSession::NORMAL_SOCKET_POOL); - int old_max_sockets_per_proxy_server = - ClientSocketPoolManager::max_sockets_per_proxy_server( - HttpNetworkSession::NORMAL_SOCKET_POOL); - int old_max_sockets_per_pool = - ClientSocketPoolManager::max_sockets_per_pool( - HttpNetworkSession::NORMAL_SOCKET_POOL); - ClientSocketPoolManager::set_max_sockets_per_group( - HttpNetworkSession::NORMAL_SOCKET_POOL, 1); - ClientSocketPoolManager::set_max_sockets_per_proxy_server( - HttpNetworkSession::NORMAL_SOCKET_POOL, 1); - ClientSocketPoolManager::set_max_sockets_per_pool( - HttpNetworkSession::NORMAL_SOCKET_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(kProtoSPDY2); - SSLSocketDataProvider ssl2(ASYNC, OK); - ssl2.SetNextProto(kProtoSPDY2); - session_deps.socket_factory.AddSSLSocketDataProvider(&ssl1); - session_deps.socket_factory.AddSSLSocketDataProvider(&ssl2); - - scoped_ptr<SpdyFrame> host1_req(ConstructSpdyGet( - "https://www.a.com", false, 1, LOWEST)); - MockWrite spdy1_writes[] = { - CreateMockWrite(*host1_req, 1), - }; - scoped_ptr<SpdyFrame> host1_resp(ConstructSpdyGetSynReply(NULL, 0, 1)); - scoped_ptr<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<SpdyFrame> host2_req(ConstructSpdyGet( - "https://www.b.com", false, 1, LOWEST)); - MockWrite spdy2_writes[] = { - CreateMockWrite(*host2_req, 1), - }; - scoped_ptr<SpdyFrame> host2_resp(ConstructSpdyGetSynReply(NULL, 0, 1)); - scoped_ptr<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( - HttpNetworkSession::NORMAL_SOCKET_POOL, old_max_sockets_per_pool); - ClientSocketPoolManager::set_max_sockets_per_proxy_server( - HttpNetworkSession::NORMAL_SOCKET_POOL, old_max_sockets_per_proxy_server); - ClientSocketPoolManager::set_max_sockets_per_group( - HttpNetworkSession::NORMAL_SOCKET_POOL, 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 efbcf39..5255c4d 100644 --- a/net/http/http_network_transaction_spdy3_unittest.cc +++ b/net/http/http_network_transaction_spdy3_unittest.cc @@ -9550,185 +9550,4 @@ 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( - HttpNetworkSession::NORMAL_SOCKET_POOL); - int old_max_sockets_per_proxy_server = - ClientSocketPoolManager::max_sockets_per_proxy_server( - HttpNetworkSession::NORMAL_SOCKET_POOL); - int old_max_sockets_per_pool = - ClientSocketPoolManager::max_sockets_per_pool( - HttpNetworkSession::NORMAL_SOCKET_POOL); - ClientSocketPoolManager::set_max_sockets_per_group( - HttpNetworkSession::NORMAL_SOCKET_POOL, 1); - ClientSocketPoolManager::set_max_sockets_per_proxy_server( - HttpNetworkSession::NORMAL_SOCKET_POOL, 1); - ClientSocketPoolManager::set_max_sockets_per_pool( - HttpNetworkSession::NORMAL_SOCKET_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(kProtoSPDY3); - SSLSocketDataProvider ssl2(ASYNC, OK); - ssl2.SetNextProto(kProtoSPDY3); - session_deps.socket_factory.AddSSLSocketDataProvider(&ssl1); - session_deps.socket_factory.AddSSLSocketDataProvider(&ssl2); - - scoped_ptr<SpdyFrame> host1_req(ConstructSpdyGet( - "https://www.a.com", false, 1, LOWEST)); - MockWrite spdy1_writes[] = { - CreateMockWrite(*host1_req, 1), - }; - scoped_ptr<SpdyFrame> host1_resp(ConstructSpdyGetSynReply(NULL, 0, 1)); - scoped_ptr<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<SpdyFrame> host2_req(ConstructSpdyGet( - "https://www.b.com", false, 1, LOWEST)); - MockWrite spdy2_writes[] = { - CreateMockWrite(*host2_req, 1), - }; - scoped_ptr<SpdyFrame> host2_resp(ConstructSpdyGetSynReply(NULL, 0, 1)); - scoped_ptr<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( - HttpNetworkSession::NORMAL_SOCKET_POOL, old_max_sockets_per_pool); - ClientSocketPoolManager::set_max_sockets_per_proxy_server( - HttpNetworkSession::NORMAL_SOCKET_POOL, old_max_sockets_per_proxy_server); - ClientSocketPoolManager::set_max_sockets_per_group( - HttpNetworkSession::NORMAL_SOCKET_POOL, 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 67018779..6b53e62 100644 --- a/net/http/http_proxy_client_socket_pool.cc +++ b/net/http/http_proxy_client_socket_pool.cc @@ -397,21 +397,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, @@ -450,12 +438,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(); } @@ -474,14 +456,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, @@ -512,10 +486,4 @@ 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 48a4b27..1b0afb4 100644 --- a/net/http/http_proxy_client_socket_pool.h +++ b/net/http/http_proxy_client_socket_pool.h @@ -170,9 +170,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, @@ -207,8 +205,6 @@ class NET_EXPORT_PRIVATE HttpProxyClientSocketPool virtual void Flush() OVERRIDE; - virtual bool IsStalled() const OVERRIDE; - virtual void CloseIdleSockets() OVERRIDE; virtual int IdleSocketCount() const OVERRIDE; @@ -220,10 +216,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, @@ -233,9 +225,6 @@ class NET_EXPORT_PRIVATE HttpProxyClientSocketPool 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 ba30151..8cc0e90 100644 --- a/net/socket/client_socket_handle.cc +++ b/net/socket/client_socket_handle.cc @@ -17,8 +17,6 @@ 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, @@ -54,10 +52,6 @@ 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(); @@ -81,28 +75,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::RemoveLayeredPool(LayeredPool* layered_pool) { - CHECK(layered_pool); - CHECK(layered_pool_); - if (pool_) { - pool_->RemoveLayeredPool(layered_pool); - layered_pool_ = NULL; - } -} - 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 e2ac23f..0b9a652 100644 --- a/net/socket/client_socket_handle.h +++ b/net/socket/client_socket_handle.h @@ -92,12 +92,6 @@ class NET_EXPORT ClientSocketHandle { // initialized the ClientSocketHandle. LoadState GetLoadState() const; - bool IsPoolStalled() const; - - void AddLayeredPool(LayeredPool* layered_pool); - - void RemoveLayeredPool(LayeredPool* layered_pool); - // Returns true when Init() has completed successfully. bool is_initialized() const { return is_initialized_; } @@ -170,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_; diff --git a/net/socket/client_socket_pool.cc b/net/socket/client_socket_pool.cc index a11feb6..a1967ef 100644 --- a/net/socket/client_socket_pool.cc +++ b/net/socket/client_socket_pool.cc @@ -1,10 +1,9 @@ -// Copyright (c) 2012 The Chromium Authors. All rights reserved. +// Copyright (c) 2011 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. #include "net/socket/client_socket_pool.h" -#include "base/debug/alias.h" #include "base/logging.h" namespace { @@ -22,26 +21,6 @@ int g_used_idle_socket_timeout_s = 300; // 5 minutes namespace net { -LayeredPool::LayeredPool() : magic_value_(ALIVE) {} - -LayeredPool::~LayeredPool() { - CrashIfFreed(); - magic_value_ = DEAD; - stack_trace_ = base::debug::StackTrace(); -} - -void LayeredPool::CrashIfFreed() { - if (magic_value_ != ALIVE) { - MagicValue magic_value = magic_value_; - base::debug::StackTrace deletion_callstack = stack_trace_; - - base::debug::Alias(&magic_value); - base::debug::Alias(&deletion_callstack); - - CHECK(false); - } -} - // static base::TimeDelta ClientSocketPool::unused_idle_socket_timeout() { return base::TimeDelta::FromSeconds(g_unused_idle_socket_timeout_s); diff --git a/net/socket/client_socket_pool.h b/net/socket/client_socket_pool.h index 8645092..92f0c413 100644 --- a/net/socket/client_socket_pool.h +++ b/net/socket/client_socket_pool.h @@ -10,7 +10,6 @@ #include <string> #include "base/basictypes.h" -#include "base/debug/stack_trace.h" #include "base/memory/ref_counted.h" #include "base/time.h" #include "base/template_util.h" @@ -30,32 +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 NET_EXPORT LayeredPool { - public: - LayeredPool(); - virtual ~LayeredPool(); - - // Instructs the LayeredPool to close an idle connection. Return true if one - // was closed. - virtual bool CloseOneIdleConnection() = 0; - - // TODO(rch): remove this once we track down the use-after-free - void CrashIfFreed(); - - private: - // TODO(rch): Remove this once we track down the use-after-free - enum MagicValue { - ALIVE = 0xCA11AB1E, - DEAD = 0xDEADBEEF - }; - - // TODO(rch): Remove this once we track down the use-after-free - MagicValue magic_value_; - base::debug::StackTrace stack_trace_; -}; - // A ClientSocketPool is used to restrict the number of sockets open at a time. // It also maintains a list of idle persistent sockets. // @@ -137,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 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; @@ -154,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 diff --git a/net/socket/client_socket_pool_base.cc b/net/socket/client_socket_pool_base.cc index 22fd29d..d230ad4 100644 --- a/net/socket/client_socket_pool_base.cc +++ b/net/socket/client_socket_pool_base.cc @@ -207,7 +207,6 @@ 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); } @@ -239,18 +238,6 @@ 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) { @@ -349,46 +336,26 @@ 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 { - 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 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; } } - // We couldn't find a socket to reuse, and there's space to allocate one, - // so allocate and connect a new one. + // We couldn't find a socket to reuse, so allocate and connect a new one. scoped_ptr<ConnectJob> connect_job( connect_job_factory_->NewConnectJob(group_name, *request, this)); @@ -650,8 +617,7 @@ DictionaryValue* ClientSocketPoolBaseHelper::GetInfoAsValue( group_dict->Set("connect_jobs", connect_jobs_list); group_dict->SetBoolean("is_stalled", - group->IsStalledOnPoolMaxSockets( - max_sockets_per_group_)); + group->IsStalled(max_sockets_per_group_)); group_dict->SetBoolean("has_backup_job", group->HasBackupJob()); all_groups_dict->SetWithoutPathExpansion(it->first, group_dict); @@ -826,22 +792,18 @@ 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) const { - CHECK((group && group_name) || (!group && !group_name)); +bool ClientSocketPoolBaseHelper::FindTopStalledGroup(Group** group, + std::string* group_name) { Group* top_group = NULL; const std::string* top_group_name = NULL; bool has_stalled_group = false; - for (GroupMap::const_iterator i = group_map_.begin(); + for (GroupMap::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->IsStalledOnPoolMaxSockets(max_sockets_per_group_)) { - if (!group) - return true; + if (curr_group->IsStalled(max_sockets_per_group_)) { has_stalled_group = true; bool has_higher_priority = !top_group || curr_group->TopPendingPriority() < top_group->TopPendingPriority(); @@ -853,11 +815,8 @@ bool ClientSocketPoolBaseHelper::FindTopStalledGroup( } if (top_group) { - CHECK(group); *group = top_group; *group_name = *top_group_name; - } else { - CHECK(!has_stalled_group); } return has_stalled_group; } @@ -930,25 +889,6 @@ 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); @@ -1085,10 +1025,8 @@ bool ClientSocketPoolBaseHelper::ReachedMaxSocketsLimit() const { return true; } -bool ClientSocketPoolBaseHelper::CloseOneIdleSocket() { - if (idle_socket_count() == 0) - return false; - return CloseOneIdleSocketExceptInGroup(NULL); +void ClientSocketPoolBaseHelper::CloseOneIdleSocket() { + CloseOneIdleSocketExceptInGroup(NULL); } bool ClientSocketPoolBaseHelper::CloseOneIdleSocketExceptInGroup( @@ -1112,19 +1050,9 @@ bool ClientSocketPoolBaseHelper::CloseOneIdleSocketExceptInGroup( } } - return false; -} + if (!exception_group) + LOG(DFATAL) << "No idle socket found to close!."; -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) { - (*it)->CrashIfFreed(); - 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 832c166..f550e42 100644 --- a/net/socket/client_socket_pool_base.h +++ b/net/socket/client_socket_pool_base.h @@ -28,7 +28,6 @@ #include <map> #include <set> #include <string> -#include <vector> #include "base/basictypes.h" #include "base/memory/ref_counted.h" @@ -240,11 +239,6 @@ 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. @@ -267,9 +261,6 @@ 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(); @@ -315,16 +306,6 @@ 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; @@ -390,7 +371,7 @@ class NET_EXPORT_PRIVATE ClientSocketPoolBaseHelper static_cast<int>(idle_sockets_.size()); } - bool IsStalledOnPoolMaxSockets(int max_sockets_per_group) const { + bool IsStalled(int max_sockets_per_group) const { return HasAvailableSocketSlot(max_sockets_per_group) && pending_requests_.size() > jobs_.size(); } @@ -476,9 +457,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 (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; + // if so, fills |group| and |group_name| with data of the stalled group + // having highest priority. + bool FindTopStalledGroup(Group** group, std::string* group_name); // Called when timer_ fires. This method scans the idle sockets removing // sockets that timed out or can't be reused. @@ -530,6 +511,13 @@ 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. @@ -594,8 +582,6 @@ 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); @@ -662,13 +648,6 @@ 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(). @@ -713,10 +692,6 @@ 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(); } @@ -765,11 +740,7 @@ class ClientSocketPoolBase { void EnableConnectBackupJobs() { helper_.EnableConnectBackupJobs(); } - bool CloseOneIdleSocket() { return helper_.CloseOneIdleSocket(); } - - bool CloseOneIdleConnectionInLayeredPool() { - return helper_.CloseOneIdleConnectionInLayeredPool(); - } + void Flush() { helper_.Flush(); } 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 d5689c5..20cf1d3 100644 --- a/net/socket/client_socket_pool_base_unittest.cc +++ b/net/socket/client_socket_pool_base_unittest.cc @@ -4,8 +4,6 @@ #include "net/socket/client_socket_pool_base.h" -#include <vector> - #include "base/bind.h" #include "base/bind_helpers.h" #include "base/callback.h" @@ -30,12 +28,8 @@ #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 { @@ -46,18 +40,10 @@ const net::RequestPriority kDefaultPriority = MEDIUM; class TestSocketParams : public base::RefCounted<TestSocketParams> { public: - TestSocketParams() : ignore_limits_(false) {} - - void set_ignore_limits(bool ignore_limits) { - ignore_limits_ = ignore_limits; - } - bool ignore_limits() { return ignore_limits_; } - + bool ignore_limits() { return false; } private: friend class base::RefCounted<TestSocketParams>; ~TestSocketParams() {} - - bool ignore_limits_; }; typedef ClientSocketPoolBase<TestSocketParams> TestClientSocketPoolBase; @@ -372,18 +358,12 @@ 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; } @@ -394,13 +374,7 @@ class TestConnectJobFactory const std::string& group_name, const TestClientSocketPoolBase::Request& request, ConnectJob::Delegate* delegate) const { - 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, + return new TestConnectJob(job_type_, group_name, request, timeout_duration_, @@ -415,7 +389,6 @@ class TestConnectJobFactory private: TestConnectJob::JobType job_type_; - std::list<TestConnectJob::JobType>* job_types_; base::TimeDelta timeout_duration_; MockClientSocketFactory* const client_socket_factory_; @@ -477,10 +450,6 @@ class TestClientSocketPool : public ClientSocketPool { base_.Flush(); } - virtual bool IsStalled() const OVERRIDE { - return base_.IsStalled(); - } - virtual void CloseIdleSockets() OVERRIDE { base_.CloseIdleSockets(); } @@ -500,14 +469,6 @@ 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, @@ -541,10 +502,6 @@ class TestClientSocketPool : public ClientSocketPool { void EnableConnectBackupJobs() { base_.EnableConnectBackupJobs(); } - bool CloseOneIdleConnectionInLayeredPool() { - return base_.CloseOneIdleConnectionInLayeredPool(); - } - private: TestClientSocketPoolBase base_; @@ -1210,7 +1167,6 @@ 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; @@ -1225,7 +1181,6 @@ 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", @@ -1234,7 +1189,6 @@ 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. } @@ -2054,9 +2008,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 100 + // The idle socket timeout value was set to 10 milliseconds. Wait 20 // milliseconds so the sockets timeout. - base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100)); + base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(20)); MessageLoop::current()->RunAllPending(); ASSERT_EQ(2, pool_->IdleSocketCount()); @@ -3088,7 +3042,6 @@ 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")); @@ -3097,7 +3050,6 @@ TEST_F(ClientSocketPoolBaseTest, RequestSocketsHitMaxSocketLimit) { ASSERT_TRUE(pool_->HasGroup("b")); EXPECT_EQ(1, pool_->NumConnectJobsInGroup("b")); - EXPECT_FALSE(pool_->IsStalled()); } TEST_F(ClientSocketPoolBaseTest, RequestSocketsCountIdleSockets) { @@ -3416,173 +3368,6 @@ 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 a7c7ecb..ae963bf 100644 --- a/net/socket/socks_client_socket_pool.cc +++ b/net/socket/socks_client_socket_pool.cc @@ -199,16 +199,9 @@ 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() { - // We should always have a |transport_pool_| except in unit tests. - if (transport_pool_) - transport_pool_->RemoveLayeredPool(this); -} +SOCKSClientSocketPool::~SOCKSClientSocketPool() {} int SOCKSClientSocketPool::RequestSocket( const std::string& group_name, const void* socket_params, @@ -246,10 +239,6 @@ void SOCKSClientSocketPool::Flush() { base_.Flush(); } -bool SOCKSClientSocketPool::IsStalled() const { - return base_.IsStalled() || transport_pool_->IsStalled(); -} - void SOCKSClientSocketPool::CloseIdleSockets() { base_.CloseIdleSockets(); } @@ -268,14 +257,6 @@ 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, @@ -299,10 +280,4 @@ 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 8583d96..3fc7df6 100644 --- a/net/socket/socks_client_socket_pool.h +++ b/net/socket/socks_client_socket_pool.h @@ -105,8 +105,7 @@ class SOCKSConnectJob : public ConnectJob { DISALLOW_COPY_AND_ASSIGN(SOCKSConnectJob); }; -class NET_EXPORT_PRIVATE SOCKSClientSocketPool - : public ClientSocketPool, public LayeredPool { +class NET_EXPORT_PRIVATE SOCKSClientSocketPool : public ClientSocketPool { public: SOCKSClientSocketPool( int max_sockets, @@ -140,8 +139,6 @@ class NET_EXPORT_PRIVATE SOCKSClientSocketPool virtual void Flush() OVERRIDE; - virtual bool IsStalled() const OVERRIDE; - virtual void CloseIdleSockets() OVERRIDE; virtual int IdleSocketCount() const OVERRIDE; @@ -153,10 +150,6 @@ class NET_EXPORT_PRIVATE SOCKSClientSocketPool 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, @@ -166,9 +159,6 @@ class NET_EXPORT_PRIVATE SOCKSClientSocketPool 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 46a3af1..e3af1592 100644 --- a/net/socket/ssl_client_socket_pool.cc +++ b/net/socket/ssl_client_socket_pool.cc @@ -479,21 +479,9 @@ 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); } @@ -546,13 +534,6 @@ 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(); } @@ -571,14 +552,6 @@ 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, @@ -618,10 +591,4 @@ 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 26e5f56..d80ace9 100644 --- a/net/socket/ssl_client_socket_pool.h +++ b/net/socket/ssl_client_socket_pool.h @@ -166,7 +166,6 @@ 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 @@ -212,8 +211,6 @@ class NET_EXPORT_PRIVATE SSLClientSocketPool virtual void Flush() OVERRIDE; - virtual bool IsStalled() const OVERRIDE; - virtual void CloseIdleSockets() OVERRIDE; virtual int IdleSocketCount() const OVERRIDE; @@ -225,10 +222,6 @@ 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, @@ -238,9 +231,6 @@ 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 a00e810..d11099f 100644 --- a/net/socket/transport_client_socket_pool.cc +++ b/net/socket/transport_client_socket_pool.cc @@ -449,10 +449,6 @@ void TransportClientSocketPool::Flush() { base_.Flush(); } -bool TransportClientSocketPool::IsStalled() const { - return base_.IsStalled(); -} - void TransportClientSocketPool::CloseIdleSockets() { base_.CloseIdleSockets(); } @@ -471,14 +467,6 @@ 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 ef3f6fa..a4b4143 100644 --- a/net/socket/transport_client_socket_pool.h +++ b/net/socket/transport_client_socket_pool.h @@ -157,7 +157,6 @@ 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( @@ -165,8 +164,6 @@ 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 cb2aad8..9da8136 100644 --- a/net/spdy/spdy_session.cc +++ b/net/spdy/spdy_session.cc @@ -431,7 +431,6 @@ net::Error SpdySession::InitializeWithSocket( state_ = CONNECTED; connection_.reset(connection); - connection_->AddLayeredPool(this); is_secure_ = is_secure; certificate_error_code_ = certificate_error_code; @@ -1223,18 +1222,6 @@ 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 SpdyStreamId id = stream->stream_id(); DCHECK(!IsStreamActive(id)); diff --git a/net/spdy/spdy_session.h b/net/spdy/spdy_session.h index 5c7b11e..f03436c 100644 --- a/net/spdy/spdy_session.h +++ b/net/spdy/spdy_session.h @@ -50,8 +50,7 @@ class SpdyStream; class SSLInfo; class NET_EXPORT SpdySession : public base::RefCounted<SpdySession>, - public BufferedSpdyFramerVisitorInterface, - public LayeredPool { + public BufferedSpdyFramerVisitorInterface { public: // Create a new SpdySession. // |host_port_proxy_pair| is the host/port that this session connects to, and @@ -273,9 +272,6 @@ class NET_EXPORT SpdySession : public base::RefCounted<SpdySession>, int GetProtocolVersion() const; - // LayeredPool implementation. - virtual bool CloseOneIdleConnection() OVERRIDE; - private: friend class base::RefCounted<SpdySession>; @@ -283,11 +279,9 @@ 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 6d56d6b..3fde2b4 100644 --- a/net/spdy/spdy_session_spdy2_unittest.cc +++ b/net/spdy/spdy_session_spdy2_unittest.cc @@ -1064,144 +1064,4 @@ 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 b34974f..4d78f37 100644 --- a/net/spdy/spdy_session_spdy3_unittest.cc +++ b/net/spdy/spdy_session_spdy3_unittest.cc @@ -1132,144 +1132,4 @@ 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 |