diff options
author | simonjam@chromium.org <simonjam@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-12-01 22:20:06 +0000 |
---|---|---|
committer | simonjam@chromium.org <simonjam@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-12-01 22:20:06 +0000 |
commit | 4b9f729a0b70ef2d916e718e48e9552dcc3d3e7d (patch) | |
tree | b413ca358feda9c6a7799634163a61e9ac65ee56 | |
parent | 9d977272049fc90034052f0e0a930584a0a42e84 (diff) | |
download | chromium_src-4b9f729a0b70ef2d916e718e48e9552dcc3d3e7d.zip chromium_src-4b9f729a0b70ef2d916e718e48e9552dcc3d3e7d.tar.gz chromium_src-4b9f729a0b70ef2d916e718e48e9552dcc3d3e7d.tar.bz2 |
Slow start pipelining.
We need to wait for an HTTP/1.1 keep-alive response before we try to pipeline. Notably, this fixes wordpress.com and techcrunch.com.
Remember which hosts clearly support, or don't support pipelining. If pipelining is supported, skip the slow start. If it's not, fall back to HttpBasicStreams.
A site is judged not to support pipelining if we see an old HTTP version or encounter a socket error. A site does support pipelining if it successfully handles 3 requests. There's obviously room for improvement here, but this is a
start.
Related changes:
- In the spirit of CHECK() failing. Use CHECK(false) instead of NOTREACHED().
- HttpPipelinedHost is now an interface with a corresponding Impl. This is to help unit test HttpPipelinedHostPool.
BUG=None
TEST=net_unittests
Review URL: http://codereview.chromium.org/8586015
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@112557 0039d316-1c4b-4281-b951-d872f2087c98
-rw-r--r-- | net/http/http_pipelined_connection.h | 12 | ||||
-rw-r--r-- | net/http/http_pipelined_connection_impl.cc | 44 | ||||
-rw-r--r-- | net/http/http_pipelined_connection_impl.h | 4 | ||||
-rw-r--r-- | net/http/http_pipelined_connection_impl_unittest.cc | 140 | ||||
-rw-r--r-- | net/http/http_pipelined_host.cc | 108 | ||||
-rw-r--r-- | net/http/http_pipelined_host.h | 70 | ||||
-rw-r--r-- | net/http/http_pipelined_host_impl.cc | 183 | ||||
-rw-r--r-- | net/http/http_pipelined_host_impl.h | 99 | ||||
-rw-r--r-- | net/http/http_pipelined_host_impl_unittest.cc | 301 | ||||
-rw-r--r-- | net/http/http_pipelined_host_pool.cc | 78 | ||||
-rw-r--r-- | net/http/http_pipelined_host_pool.h | 36 | ||||
-rw-r--r-- | net/http/http_pipelined_host_pool_unittest.cc | 185 | ||||
-rw-r--r-- | net/http/http_pipelined_host_unittest.cc | 201 | ||||
-rw-r--r-- | net/http/http_pipelined_network_transaction_unittest.cc | 224 | ||||
-rw-r--r-- | net/http/http_stream_factory_impl.cc | 2 | ||||
-rw-r--r-- | net/http/http_stream_factory_impl.h | 11 | ||||
-rw-r--r-- | net/http/http_stream_factory_impl_job.cc | 8 | ||||
-rw-r--r-- | net/http/http_stream_factory_impl_job.h | 2 | ||||
-rw-r--r-- | net/net.gyp | 6 |
19 files changed, 1288 insertions, 426 deletions
diff --git a/net/http/http_pipelined_connection.h b/net/http/http_pipelined_connection.h index 59e61d6..e8c332b 100644 --- a/net/http/http_pipelined_connection.h +++ b/net/http/http_pipelined_connection.h @@ -19,12 +19,24 @@ struct SSLConfig; class NET_EXPORT_PRIVATE HttpPipelinedConnection { public: + enum Feedback { + OK, + PIPELINE_SOCKET_ERROR, + OLD_HTTP_VERSION, + MUST_CLOSE_CONNECTION, + }; + class Delegate { public: // Called when a pipeline has newly available capacity. This may be because // the first request has been sent and the pipeline is now active. Or, it // may be because a request successfully completed. virtual void OnPipelineHasCapacity(HttpPipelinedConnection* pipeline) = 0; + + // Called every time a pipeline receives headers. Lets the delegate know if + // the headers indicate that pipelining can be used. + virtual void OnPipelineFeedback(HttpPipelinedConnection* pipeline, + Feedback feedback) = 0; }; class Factory { diff --git a/net/http/http_pipelined_connection_impl.cc b/net/http/http_pipelined_connection_impl.cc index 7792970..95d27a6 100644 --- a/net/http/http_pipelined_connection_impl.cc +++ b/net/http/http_pipelined_connection_impl.cc @@ -12,6 +12,7 @@ #include "net/http/http_response_body_drainer.h" #include "net/http/http_response_headers.h" #include "net/http/http_stream_parser.h" +#include "net/http/http_version.h" #include "net/socket/client_socket_handle.h" namespace net { @@ -167,7 +168,7 @@ int HttpPipelinedConnectionImpl::DoSendRequestLoop(int result) { rv = DoEvictPendingSendRequests(rv); break; default: - NOTREACHED() << "bad send state: " << state; + CHECK(false) << "bad send state: " << state; rv = ERR_FAILED; break; } @@ -337,7 +338,7 @@ int HttpPipelinedConnectionImpl::DoReadHeadersLoop(int result) { case READ_STATE_NONE: break; default: - NOTREACHED() << "bad read state"; + CHECK(false) << "bad read state"; rv = ERR_FAILED; break; } @@ -394,7 +395,7 @@ int HttpPipelinedConnectionImpl::DoStartNextDeferredRead(int result) { break; default: - NOTREACHED() << "Unexpected read state: " + CHECK(false) << "Unexpected read state: " << stream_info_map_[next_id].state; } @@ -425,6 +426,8 @@ int HttpPipelinedConnectionImpl::DoReadHeadersComplete(int result) { usable_ = false; } + CheckHeadersForPipelineCompatibility(result, active_read_id_); + if (!read_still_on_call_stack_) { QueueUserCallback(active_read_id_, stream_info_map_[active_read_id_].read_headers_callback, @@ -445,6 +448,7 @@ int HttpPipelinedConnectionImpl::DoReadStreamClosed() { CHECK_EQ(stream_info_map_[active_read_id_].state, STREAM_CLOSED); active_read_id_ = 0; if (!usable_) { + // TODO(simonjam): Don't wait this long to evict. read_next_state_ = READ_STATE_EVICT_PENDING_READS; return OK; } @@ -527,7 +531,7 @@ void HttpPipelinedConnectionImpl::Close(int pipeline_id, break; default: - NOTREACHED(); + CHECK(false); break; } } @@ -624,6 +628,38 @@ void HttpPipelinedConnectionImpl::Drain(HttpPipelinedStream* stream, // |drainer| will delete itself when done. } +void HttpPipelinedConnectionImpl::CheckHeadersForPipelineCompatibility( + int result, + int pipeline_id) { + if (result < OK) { + switch (result) { + // TODO(simonjam): Ignoring specific errors like this may not work. + // Collect metrics to see if this code is useful. + case ERR_ABORTED: + case ERR_INTERNET_DISCONNECTED: + // These errors are no fault of the server. + break; + + default: + delegate_->OnPipelineFeedback(this, PIPELINE_SOCKET_ERROR); + return; + } + } + HttpResponseInfo* info = GetResponseInfo(pipeline_id); + const HttpVersion required_version(1, 1); + if (info->headers->GetParsedHttpVersion() < required_version) { + delegate_->OnPipelineFeedback(this, OLD_HTTP_VERSION); + return; + } + if (!info->headers->IsKeepAlive() || !CanFindEndOfResponse(pipeline_id)) { + usable_ = false; + delegate_->OnPipelineFeedback(this, MUST_CLOSE_CONNECTION); + return; + } + // TODO(simonjam): We should also check for, and work around, authentication. + delegate_->OnPipelineFeedback(this, OK); +} + void HttpPipelinedConnectionImpl::QueueUserCallback( int pipeline_id, OldCompletionCallback* callback, diff --git a/net/http/http_pipelined_connection_impl.h b/net/http/http_pipelined_connection_impl.h index a63aabd..64577b7 100644 --- a/net/http/http_pipelined_connection_impl.h +++ b/net/http/http_pipelined_connection_impl.h @@ -256,6 +256,10 @@ class NET_EXPORT_PRIVATE HttpPipelinedConnectionImpl // HttpPipelinedSockets indicates the connection was suddenly closed. int DoEvictPendingReadHeaders(int result); + // Reports back to |delegate_| whether pipelining will work. This is called + // every time we receive headers. + void CheckHeadersForPipelineCompatibility(int result, int pipeline_id); + // Posts a task to fire the user's callback in response to SendRequest() or // ReadResponseHeaders() completing on an underlying parser. This might be // invoked in response to our own IO callbacks, or it may be invoked if the diff --git a/net/http/http_pipelined_connection_impl_unittest.cc b/net/http/http_pipelined_connection_impl_unittest.cc index 2c9beec..2c0d2df 100644 --- a/net/http/http_pipelined_connection_impl_unittest.cc +++ b/net/http/http_pipelined_connection_impl_unittest.cc @@ -31,9 +31,14 @@ class DummySocketParams : public base::RefCounted<DummySocketParams> { REGISTER_SOCKET_PARAMS_FOR_POOL(MockTransportClientSocketPool, DummySocketParams); -class MockPipelineDelegate : public HttpPipelinedConnectionImpl::Delegate { +namespace { + +class MockPipelineDelegate : public HttpPipelinedConnection::Delegate { public: MOCK_METHOD1(OnPipelineHasCapacity, void(HttpPipelinedConnection* pipeline)); + MOCK_METHOD2(OnPipelineFeedback, void( + HttpPipelinedConnection* pipeline, + HttpPipelinedConnection::Feedback feedback)); }; class SuddenCloseObserver : public MessageLoop::TaskObserver { @@ -1211,6 +1216,137 @@ TEST_F(HttpPipelinedConnectionImplTest, EvictIfDrainingChunkedEncoding) { stream2->Close(false); } +TEST_F(HttpPipelinedConnectionImplTest, EvictionDueToMissingContentLength) { + MockWrite writes[] = { + MockWrite(false, 0, "GET /ok.html HTTP/1.1\r\n\r\n"), + MockWrite(false, 1, "GET /evicted.html HTTP/1.1\r\n\r\n"), + MockWrite(false, 2, "GET /rejected.html HTTP/1.1\r\n\r\n"), + }; + MockRead reads[] = { + MockRead(true, 3, "HTTP/1.1 200 OK\r\n\r\n"), + MockRead(false, 4, "ok.html"), + MockRead(false, OK, 5), + }; + Initialize(reads, arraysize(reads), writes, arraysize(writes)); + + scoped_ptr<HttpStream> ok_stream(NewTestStream("ok.html")); + scoped_ptr<HttpStream> evicted_stream(NewTestStream("evicted.html")); + scoped_ptr<HttpStream> rejected_stream(NewTestStream("rejected.html")); + + HttpRequestHeaders headers; + HttpResponseInfo response; + EXPECT_EQ(OK, ok_stream->SendRequest(headers, NULL, &response, &callback_)); + EXPECT_EQ(OK, evicted_stream->SendRequest(headers, NULL, &response, + &callback_)); + EXPECT_EQ(OK, rejected_stream->SendRequest(headers, NULL, &response, + &callback_)); + + TestOldCompletionCallback ok_callback; + EXPECT_EQ(ERR_IO_PENDING, ok_stream->ReadResponseHeaders(&ok_callback)); + + TestOldCompletionCallback evicted_callback; + EXPECT_EQ(ERR_IO_PENDING, + evicted_stream->ReadResponseHeaders(&evicted_callback)); + + data_->RunFor(1); + EXPECT_LE(OK, ok_callback.WaitForResult()); + data_->StopAfter(10); + + ExpectResponse("ok.html", ok_stream, false); + ok_stream->Close(false); + + EXPECT_EQ(ERR_PIPELINE_EVICTION, + rejected_stream->ReadResponseHeaders(&callback_)); + rejected_stream->Close(true); + EXPECT_EQ(ERR_PIPELINE_EVICTION, evicted_callback.WaitForResult()); + evicted_stream->Close(true); +} + +TEST_F(HttpPipelinedConnectionImplTest, FeedbackOnSocketError) { + MockWrite writes[] = { + MockWrite(false, 0, "GET /ok.html HTTP/1.1\r\n\r\n"), + }; + MockRead reads[] = { + MockRead(false, ERR_FAILED, 1), + }; + Initialize(reads, arraysize(reads), writes, arraysize(writes)); + + EXPECT_CALL(delegate_, + OnPipelineFeedback( + pipeline_.get(), + HttpPipelinedConnection::PIPELINE_SOCKET_ERROR)) + .Times(1); + + scoped_ptr<HttpStream> stream(NewTestStream("ok.html")); + HttpRequestHeaders headers; + HttpResponseInfo response; + EXPECT_EQ(OK, stream->SendRequest(headers, NULL, &response, &callback_)); + EXPECT_EQ(ERR_FAILED, stream->ReadResponseHeaders(&callback_)); +} + +TEST_F(HttpPipelinedConnectionImplTest, FeedbackOnHttp10) { + MockWrite writes[] = { + MockWrite(false, 0, "GET /ok.html HTTP/1.1\r\n\r\n"), + }; + MockRead reads[] = { + MockRead(false, 1, "HTTP/1.0 200 OK\r\n"), + MockRead(false, 2, "Content-Length: 7\r\n"), + MockRead(false, 3, "Connection: keep-alive\r\n\r\n"), + MockRead(false, 4, "ok.html"), + }; + Initialize(reads, arraysize(reads), writes, arraysize(writes)); + + EXPECT_CALL(delegate_, + OnPipelineFeedback(pipeline_.get(), + HttpPipelinedConnection::OLD_HTTP_VERSION)) + .Times(1); + + scoped_ptr<HttpStream> stream(NewTestStream("ok.html")); + TestSyncRequest(stream, "ok.html"); +} + +TEST_F(HttpPipelinedConnectionImplTest, FeedbackOnMustClose) { + MockWrite writes[] = { + MockWrite(false, 0, "GET /ok.html HTTP/1.1\r\n\r\n"), + }; + MockRead reads[] = { + MockRead(false, 1, "HTTP/1.1 200 OK\r\n"), + MockRead(false, 2, "Content-Length: 7\r\n"), + MockRead(false, 3, "Connection: close\r\n\r\n"), + MockRead(false, 4, "ok.html"), + }; + Initialize(reads, arraysize(reads), writes, arraysize(writes)); + + EXPECT_CALL(delegate_, + OnPipelineFeedback( + pipeline_.get(), + HttpPipelinedConnection::MUST_CLOSE_CONNECTION)) + .Times(1); + + scoped_ptr<HttpStream> stream(NewTestStream("ok.html")); + TestSyncRequest(stream, "ok.html"); +} + +TEST_F(HttpPipelinedConnectionImplTest, FeedbackOnNoContentLength) { + MockWrite writes[] = { + MockWrite(false, 0, "GET /ok.html HTTP/1.1\r\n\r\n"), + }; + MockRead reads[] = { + MockRead(false, 1, "HTTP/1.1 200 OK\r\n\r\n"), + MockRead(false, 2, "ok.html"), + }; + Initialize(reads, arraysize(reads), writes, arraysize(writes)); + + EXPECT_CALL(delegate_, + OnPipelineFeedback( + pipeline_.get(), + HttpPipelinedConnection::MUST_CLOSE_CONNECTION)) + .Times(1); + + scoped_ptr<HttpStream> stream(NewTestStream("ok.html")); + TestSyncRequest(stream, "ok.html"); +} + TEST_F(HttpPipelinedConnectionImplTest, OnPipelineHasCapacity) { MockWrite writes[] = { MockWrite(false, 0, "GET /ok.html HTTP/1.1\r\n\r\n"), @@ -1250,4 +1386,6 @@ TEST_F(HttpPipelinedConnectionImplTest, OnPipelineHasCapacityWithoutSend) { stream.reset(NULL); } +} // anonymous namespace + } // namespace net diff --git a/net/http/http_pipelined_host.cc b/net/http/http_pipelined_host.cc deleted file mode 100644 index 32d023d..0000000 --- a/net/http/http_pipelined_host.cc +++ /dev/null @@ -1,108 +0,0 @@ -// 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/http/http_pipelined_host.h" - -#include "base/stl_util.h" -#include "net/http/http_pipelined_connection_impl.h" -#include "net/http/http_pipelined_stream.h" - -namespace net { - -class HttpPipelinedConnectionImplFactory : - public HttpPipelinedConnection::Factory { - public: - HttpPipelinedConnection* CreateNewPipeline( - ClientSocketHandle* connection, - HttpPipelinedConnection::Delegate* delegate, - const SSLConfig& used_ssl_config, - const ProxyInfo& used_proxy_info, - const BoundNetLog& net_log, - bool was_npn_negotiated) OVERRIDE { - return new HttpPipelinedConnectionImpl(connection, delegate, - used_ssl_config, used_proxy_info, - net_log, was_npn_negotiated); - } -}; - -HttpPipelinedHost::HttpPipelinedHost( - HttpPipelinedHost::Delegate* delegate, - const HostPortPair& origin, - HttpPipelinedConnection::Factory* factory) - : delegate_(delegate), - origin_(origin), - factory_(factory) { - if (!factory) { - factory_.reset(new HttpPipelinedConnectionImplFactory()); - } -} - -HttpPipelinedHost::~HttpPipelinedHost() { - CHECK(pipelines_.empty()); -} - -HttpPipelinedStream* HttpPipelinedHost::CreateStreamOnNewPipeline( - ClientSocketHandle* connection, - const SSLConfig& used_ssl_config, - const ProxyInfo& used_proxy_info, - const BoundNetLog& net_log, - bool was_npn_negotiated) { - HttpPipelinedConnection* pipeline = factory_->CreateNewPipeline( - connection, this, used_ssl_config, used_proxy_info, net_log, - was_npn_negotiated); - pipelines_.insert(pipeline); - return pipeline->CreateNewStream(); -} - -HttpPipelinedStream* HttpPipelinedHost::CreateStreamOnExistingPipeline() { - HttpPipelinedConnection* available_pipeline = NULL; - for (std::set<HttpPipelinedConnection*>::iterator it = pipelines_.begin(); - it != pipelines_.end(); ++it) { - if ((*it)->usable() && - (*it)->active() && - (*it)->depth() < max_pipeline_depth() && - (!available_pipeline || (*it)->depth() < available_pipeline->depth())) { - available_pipeline = *it; - } - } - if (!available_pipeline) { - return NULL; - } - return available_pipeline->CreateNewStream(); -} - -bool HttpPipelinedHost::IsExistingPipelineAvailable() { - for (std::set<HttpPipelinedConnection*>::iterator it = pipelines_.begin(); - it != pipelines_.end(); ++it) { - if ((*it)->usable() && - (*it)->active() && - (*it)->depth() < max_pipeline_depth()) { - return true; - } - } - return false; -} - -void HttpPipelinedHost::OnPipelineEmpty(HttpPipelinedConnection* pipeline) { - CHECK(ContainsKey(pipelines_, pipeline)); - pipelines_.erase(pipeline); - delete pipeline; - if (pipelines_.empty()) { - delegate_->OnHostIdle(this); - // WARNING: We'll probably be deleted here. - } -} - -void HttpPipelinedHost::OnPipelineHasCapacity( - HttpPipelinedConnection* pipeline) { - if (pipeline->usable() && pipeline->depth() < max_pipeline_depth()) { - delegate_->OnHostHasAdditionalCapacity(this); - } - if (!pipeline->depth()) { - OnPipelineEmpty(pipeline); - // WARNING: We might be deleted here. - } -} - -} // namespace net diff --git a/net/http/http_pipelined_host.h b/net/http/http_pipelined_host.h index ae3128d..c368313 100644 --- a/net/http/http_pipelined_host.h +++ b/net/http/http_pipelined_host.h @@ -6,11 +6,6 @@ #define NET_HTTP_HTTP_PIPELINED_HOST_H_ #pragma once -#include <set> -#include <string> - -#include "base/basictypes.h" -#include "base/memory/scoped_ptr.h" #include "net/base/host_port_pair.h" #include "net/base/net_export.h" #include "net/http/http_pipelined_connection.h" @@ -26,9 +21,16 @@ struct SSLConfig; // Manages all of the pipelining state for specific host with active pipelined // HTTP requests. Manages connection jobs, constructs pipelined streams, and // assigns requests to the least loaded pipelined connection. -class NET_EXPORT_PRIVATE HttpPipelinedHost - : public HttpPipelinedConnection::Delegate { +class NET_EXPORT_PRIVATE HttpPipelinedHost { public: + enum Capability { + UNKNOWN, + INCAPABLE, + CAPABLE, + PROBABLY_CAPABLE, // We are using pipelining, but haven't processed enough + // requests to record this host as known to be capable. + }; + class Delegate { public: // Called when a pipelined host has no outstanding requests on any of its @@ -38,54 +40,44 @@ class NET_EXPORT_PRIVATE HttpPipelinedHost // Called when a pipelined host has newly available pipeline capacity, like // when a request completes. virtual void OnHostHasAdditionalCapacity(HttpPipelinedHost* host) = 0; + + // Called when a host determines if pipelining can be used. + virtual void OnHostDeterminedCapability(HttpPipelinedHost* host, + Capability capability) = 0; + }; + + class Factory { + public: + virtual ~Factory() {} + + // Returns a new HttpPipelinedHost. + virtual HttpPipelinedHost* CreateNewHost( + Delegate* delegate, const HostPortPair& origin, + HttpPipelinedConnection::Factory* factory, + Capability capability) = 0; }; - HttpPipelinedHost(Delegate* delegate, const HostPortPair& origin, - HttpPipelinedConnection::Factory* factory); - virtual ~HttpPipelinedHost(); + virtual ~HttpPipelinedHost() {} // Constructs a new pipeline on |connection| and returns a new // HttpPipelinedStream that uses it. - HttpPipelinedStream* CreateStreamOnNewPipeline( + virtual HttpPipelinedStream* CreateStreamOnNewPipeline( ClientSocketHandle* connection, const SSLConfig& used_ssl_config, const ProxyInfo& used_proxy_info, const BoundNetLog& net_log, - bool was_npn_negotiated); + bool was_npn_negotiated) = 0; // Tries to find an existing pipeline with capacity for a new request. If // successful, returns a new stream on that pipeline. Otherwise, returns NULL. - HttpPipelinedStream* CreateStreamOnExistingPipeline(); + virtual HttpPipelinedStream* CreateStreamOnExistingPipeline() = 0; // Returns true if we have a pipelined connection that can accept new // requests. - bool IsExistingPipelineAvailable(); - - // Callbacks for HttpPipelinedConnection. - - // Called when a pipelined connection completes a request. Adds a pending - // request to the pipeline if the pipeline is still usable. - virtual void OnPipelineHasCapacity( - HttpPipelinedConnection* pipeline) OVERRIDE; - - const HostPortPair& origin() const { return origin_; } - - private: - // Called when a pipeline is empty and there are no pending requests. Closes - // the connection. - void OnPipelineEmpty(HttpPipelinedConnection* pipeline); - - // Adds the next pending request to the pipeline if it's still usuable. - void AddRequestToPipeline(HttpPipelinedConnection* connection); - - int max_pipeline_depth() const { return 3; } - - Delegate* delegate_; - const HostPortPair origin_; - std::set<HttpPipelinedConnection*> pipelines_; - scoped_ptr<HttpPipelinedConnection::Factory> factory_; + virtual bool IsExistingPipelineAvailable() const = 0; - DISALLOW_COPY_AND_ASSIGN(HttpPipelinedHost); + // Returns the host and port associated with this class. + virtual const HostPortPair& origin() const = 0; }; } // namespace net diff --git a/net/http/http_pipelined_host_impl.cc b/net/http/http_pipelined_host_impl.cc new file mode 100644 index 0000000..25014763 --- /dev/null +++ b/net/http/http_pipelined_host_impl.cc @@ -0,0 +1,183 @@ +// 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/http/http_pipelined_host_impl.h" + +#include "base/stl_util.h" +#include "net/http/http_pipelined_connection_impl.h" +#include "net/http/http_pipelined_stream.h" + +namespace net { + +// TODO(simonjam): Run experiments to see what value minimizes evictions without +// costing too much performance. Until then, this is just a bad guess. +static const int kNumKnownSuccessesThreshold = 3; + +class HttpPipelinedConnectionImplFactory : + public HttpPipelinedConnection::Factory { + public: + HttpPipelinedConnection* CreateNewPipeline( + ClientSocketHandle* connection, + HttpPipelinedConnection::Delegate* delegate, + const SSLConfig& used_ssl_config, + const ProxyInfo& used_proxy_info, + const BoundNetLog& net_log, + bool was_npn_negotiated) OVERRIDE { + return new HttpPipelinedConnectionImpl(connection, delegate, + used_ssl_config, used_proxy_info, + net_log, was_npn_negotiated); + } +}; + +HttpPipelinedHostImpl::HttpPipelinedHostImpl( + HttpPipelinedHost::Delegate* delegate, + const HostPortPair& origin, + HttpPipelinedConnection::Factory* factory, + Capability capability) + : delegate_(delegate), + origin_(origin), + factory_(factory), + capability_(capability) { + if (!factory) { + factory_.reset(new HttpPipelinedConnectionImplFactory()); + } +} + +HttpPipelinedHostImpl::~HttpPipelinedHostImpl() { + CHECK(pipelines_.empty()); +} + +HttpPipelinedStream* HttpPipelinedHostImpl::CreateStreamOnNewPipeline( + ClientSocketHandle* connection, + const SSLConfig& used_ssl_config, + const ProxyInfo& used_proxy_info, + const BoundNetLog& net_log, + bool was_npn_negotiated) { + if (capability_ == INCAPABLE) { + return NULL; + } + HttpPipelinedConnection* pipeline = factory_->CreateNewPipeline( + connection, this, used_ssl_config, used_proxy_info, net_log, + was_npn_negotiated); + PipelineInfo info; + pipelines_.insert(std::make_pair(pipeline, info)); + return pipeline->CreateNewStream(); +} + +HttpPipelinedStream* HttpPipelinedHostImpl::CreateStreamOnExistingPipeline() { + HttpPipelinedConnection* available_pipeline = NULL; + for (PipelineInfoMap::iterator it = pipelines_.begin(); + it != pipelines_.end(); ++it) { + if (it->first->usable() && + it->first->active() && + it->first->depth() < GetPipelineCapacity() && + (!available_pipeline || + it->first->depth() < available_pipeline->depth())) { + available_pipeline = it->first; + } + } + if (!available_pipeline) { + return NULL; + } + return available_pipeline->CreateNewStream(); +} + +bool HttpPipelinedHostImpl::IsExistingPipelineAvailable() const { + for (PipelineInfoMap::const_iterator it = pipelines_.begin(); + it != pipelines_.end(); ++it) { + if (it->first->usable() && + it->first->active() && + it->first->depth() < GetPipelineCapacity()) { + return true; + } + } + return false; +} + +const HostPortPair& HttpPipelinedHostImpl::origin() const { + return origin_; +} + +void HttpPipelinedHostImpl::OnPipelineEmpty(HttpPipelinedConnection* pipeline) { + CHECK(ContainsKey(pipelines_, pipeline)); + pipelines_.erase(pipeline); + delete pipeline; + if (pipelines_.empty()) { + delegate_->OnHostIdle(this); + // WARNING: We'll probably be deleted here. + } +} + +void HttpPipelinedHostImpl::OnPipelineHasCapacity( + HttpPipelinedConnection* pipeline) { + CHECK(ContainsKey(pipelines_, pipeline)); + if (pipeline->usable() && + capability_ != INCAPABLE && + pipeline->depth() < GetPipelineCapacity()) { + delegate_->OnHostHasAdditionalCapacity(this); + } + if (!pipeline->depth()) { + OnPipelineEmpty(pipeline); + // WARNING: We might be deleted here. + } +} + +void HttpPipelinedHostImpl::OnPipelineFeedback( + HttpPipelinedConnection* pipeline, + HttpPipelinedConnection::Feedback feedback) { + CHECK(ContainsKey(pipelines_, pipeline)); + switch (feedback) { + case HttpPipelinedConnection::OK: + ++pipelines_[pipeline].num_successes; + if (capability_ == UNKNOWN) { + capability_ = PROBABLY_CAPABLE; + for (PipelineInfoMap::iterator it = pipelines_.begin(); + it != pipelines_.end(); ++it) { + OnPipelineHasCapacity(it->first); + } + } else if (capability_ == PROBABLY_CAPABLE && + pipelines_[pipeline].num_successes >= + kNumKnownSuccessesThreshold) { + capability_ = CAPABLE; + delegate_->OnHostDeterminedCapability(this, CAPABLE); + } + break; + + case HttpPipelinedConnection::PIPELINE_SOCKET_ERROR: + case HttpPipelinedConnection::OLD_HTTP_VERSION: + capability_ = INCAPABLE; + delegate_->OnHostDeterminedCapability(this, INCAPABLE); + break; + + case HttpPipelinedConnection::MUST_CLOSE_CONNECTION: + break; + } +} + +int HttpPipelinedHostImpl::GetPipelineCapacity() const { + int capacity = 0; + switch (capability_) { + case CAPABLE: + case PROBABLY_CAPABLE: + capacity = max_pipeline_depth(); + break; + + case INCAPABLE: + CHECK(false); + + case UNKNOWN: + capacity = 1; + break; + + default: + CHECK(false) << "Unkown pipeline capability: " << capability_; + } + return capacity; +} + +HttpPipelinedHostImpl::PipelineInfo::PipelineInfo() + : num_successes(0) { +} + +} // namespace net diff --git a/net/http/http_pipelined_host_impl.h b/net/http/http_pipelined_host_impl.h new file mode 100644 index 0000000..af78005 --- /dev/null +++ b/net/http/http_pipelined_host_impl.h @@ -0,0 +1,99 @@ +// 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. + +#ifndef NET_HTTP_HTTP_PIPELINED_HOST_IMPL_H_ +#define NET_HTTP_HTTP_PIPELINED_HOST_IMPL_H_ +#pragma once + +#include <map> +#include <string> + +#include "base/basictypes.h" +#include "base/memory/scoped_ptr.h" +#include "net/base/host_port_pair.h" +#include "net/base/net_export.h" +#include "net/http/http_pipelined_connection.h" +#include "net/http/http_pipelined_host.h" + +namespace net { + +class BoundNetLog; +class ClientSocketHandle; +class HttpPipelinedStream; +class ProxyInfo; +struct SSLConfig; + +// Manages all of the pipelining state for specific host with active pipelined +// HTTP requests. Manages connection jobs, constructs pipelined streams, and +// assigns requests to the least loaded pipelined connection. +class NET_EXPORT_PRIVATE HttpPipelinedHostImpl + : public HttpPipelinedHost, + public HttpPipelinedConnection::Delegate { + public: + HttpPipelinedHostImpl(HttpPipelinedHost::Delegate* delegate, + const HostPortPair& origin, + HttpPipelinedConnection::Factory* factory, + Capability capability); + virtual ~HttpPipelinedHostImpl(); + + // HttpPipelinedHost interface + virtual HttpPipelinedStream* CreateStreamOnNewPipeline( + ClientSocketHandle* connection, + const SSLConfig& used_ssl_config, + const ProxyInfo& used_proxy_info, + const BoundNetLog& net_log, + bool was_npn_negotiated) OVERRIDE; + + virtual HttpPipelinedStream* CreateStreamOnExistingPipeline() OVERRIDE; + + virtual bool IsExistingPipelineAvailable() const OVERRIDE; + + // HttpPipelinedConnection::Delegate interface + + // Called when a pipelined connection completes a request. Adds a pending + // request to the pipeline if the pipeline is still usable. + virtual void OnPipelineHasCapacity( + HttpPipelinedConnection* pipeline) OVERRIDE; + + virtual void OnPipelineFeedback( + HttpPipelinedConnection* pipeline, + HttpPipelinedConnection::Feedback feedback) OVERRIDE; + + virtual const HostPortPair& origin() const OVERRIDE; + + // Returns the maximum number of in-flight pipelined requests we'll allow on a + // single connection. + NET_EXPORT_PRIVATE static int max_pipeline_depth() { return 3; } + + private: + struct PipelineInfo { + PipelineInfo(); + + int num_successes; + }; + typedef std::map<HttpPipelinedConnection*, PipelineInfo> PipelineInfoMap; + + // Called when a pipeline is empty and there are no pending requests. Closes + // the connection. + void OnPipelineEmpty(HttpPipelinedConnection* pipeline); + + // Adds the next pending request to the pipeline if it's still usuable. + void AddRequestToPipeline(HttpPipelinedConnection* connection); + + // Returns the current pipeline capacity based on |capability_|. This should + // not be called if |capability_| is INCAPABLE. + int GetPipelineCapacity() const; + + HttpPipelinedHost::Delegate* delegate_; + const HostPortPair origin_; + PipelineInfoMap pipelines_; + scoped_ptr<HttpPipelinedConnection::Factory> factory_; + Capability capability_; + + DISALLOW_COPY_AND_ASSIGN(HttpPipelinedHostImpl); +}; + +} // namespace net + +#endif // NET_HTTP_HTTP_PIPELINED_HOST_IMPL_H_ diff --git a/net/http/http_pipelined_host_impl_unittest.cc b/net/http/http_pipelined_host_impl_unittest.cc new file mode 100644 index 0000000..ed7d722 --- /dev/null +++ b/net/http/http_pipelined_host_impl_unittest.cc @@ -0,0 +1,301 @@ +// 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/http/http_pipelined_host_impl.h" + +#include "base/memory/scoped_ptr.h" +#include "net/base/ssl_config_service.h" +#include "net/http/http_pipelined_connection.h" +#include "net/proxy/proxy_info.h" +#include "testing/gmock/include/gmock/gmock.h" +#include "testing/gtest/include/gtest/gtest.h" + +using testing::_; +using testing::NiceMock; +using testing::Ref; +using testing::Return; +using testing::ReturnNull; + +namespace net { + +namespace { + +ClientSocketHandle* kDummyConnection = + reinterpret_cast<ClientSocketHandle*>(84); +HttpPipelinedStream* kDummyStream = + reinterpret_cast<HttpPipelinedStream*>(42); + +class MockHostDelegate : public HttpPipelinedHost::Delegate { + public: + MOCK_METHOD1(OnHostIdle, void(HttpPipelinedHost* host)); + MOCK_METHOD1(OnHostHasAdditionalCapacity, void(HttpPipelinedHost* host)); + MOCK_METHOD2(OnHostDeterminedCapability, + void(HttpPipelinedHost* host, + HttpPipelinedHost::Capability capability)); +}; + +class MockPipelineFactory : public HttpPipelinedConnection::Factory { + public: + MOCK_METHOD6(CreateNewPipeline, HttpPipelinedConnection*( + ClientSocketHandle* connection, + HttpPipelinedConnection::Delegate* delegate, + const SSLConfig& used_ssl_config, + const ProxyInfo& used_proxy_info, + const BoundNetLog& net_log, + bool was_npn_negotiated)); +}; + +class MockPipeline : public HttpPipelinedConnection { + public: + MockPipeline(int depth, bool usable, bool active) + : depth_(depth), + usable_(usable), + active_(active) { + } + + void SetState(int depth, bool usable, bool active) { + depth_ = depth; + usable_ = usable; + active_ = active; + } + + virtual int depth() const OVERRIDE { return depth_; } + virtual bool usable() const OVERRIDE { return usable_; } + virtual bool active() const OVERRIDE { return active_; } + + MOCK_METHOD0(CreateNewStream, HttpPipelinedStream*()); + MOCK_METHOD1(OnStreamDeleted, void(int pipeline_id)); + MOCK_CONST_METHOD0(used_ssl_config, const SSLConfig&()); + MOCK_CONST_METHOD0(used_proxy_info, const ProxyInfo&()); + MOCK_CONST_METHOD0(source, const NetLog::Source&()); + MOCK_CONST_METHOD0(was_npn_negotiated, bool()); + + private: + int depth_; + bool usable_; + bool active_; +}; + +class HttpPipelinedHostImplTest : public testing::Test { + public: + HttpPipelinedHostImplTest() + : origin_("host", 123), + factory_(new MockPipelineFactory), // Owned by host_. + host_(new HttpPipelinedHostImpl(&delegate_, origin_, factory_, + HttpPipelinedHost::CAPABLE)) { + } + + void SetCapability(HttpPipelinedHost::Capability capability) { + factory_ = new MockPipelineFactory; + host_.reset(new HttpPipelinedHostImpl( + &delegate_, origin_, factory_, capability)); + } + + MockPipeline* AddTestPipeline(int depth, bool usable, bool active) { + MockPipeline* pipeline = new MockPipeline(depth, usable, active); + EXPECT_CALL(*factory_, CreateNewPipeline(kDummyConnection, host_.get(), + Ref(ssl_config_), Ref(proxy_info_), + Ref(net_log_), true)) + .Times(1) + .WillOnce(Return(pipeline)); + EXPECT_CALL(*pipeline, CreateNewStream()) + .Times(1) + .WillOnce(Return(kDummyStream)); + EXPECT_EQ(kDummyStream, host_->CreateStreamOnNewPipeline( + kDummyConnection, ssl_config_, proxy_info_, net_log_, true)); + return pipeline; + } + + void ClearTestPipeline(MockPipeline* pipeline) { + pipeline->SetState(0, true, true); + host_->OnPipelineHasCapacity(pipeline); + } + + NiceMock<MockHostDelegate> delegate_; + HostPortPair origin_; + MockPipelineFactory* factory_; + scoped_ptr<HttpPipelinedHostImpl> host_; + + SSLConfig ssl_config_; + ProxyInfo proxy_info_; + BoundNetLog net_log_; +}; + +TEST_F(HttpPipelinedHostImplTest, Delegate) { + EXPECT_TRUE(origin_.Equals(host_->origin())); +} + +TEST_F(HttpPipelinedHostImplTest, OnUnusablePipelineHasCapacity) { + MockPipeline* pipeline = AddTestPipeline(0, false, true); + + EXPECT_CALL(delegate_, OnHostHasAdditionalCapacity(host_.get())) + .Times(0); + EXPECT_CALL(delegate_, OnHostIdle(host_.get())) + .Times(1); + host_->OnPipelineHasCapacity(pipeline); +} + +TEST_F(HttpPipelinedHostImplTest, OnUsablePipelineHasCapacity) { + MockPipeline* pipeline = AddTestPipeline(1, true, true); + + EXPECT_CALL(delegate_, OnHostHasAdditionalCapacity(host_.get())) + .Times(1); + EXPECT_CALL(delegate_, OnHostIdle(host_.get())) + .Times(0); + + host_->OnPipelineHasCapacity(pipeline); + + EXPECT_CALL(delegate_, OnHostHasAdditionalCapacity(host_.get())) + .Times(1); + EXPECT_CALL(delegate_, OnHostIdle(host_.get())) + .Times(1); + ClearTestPipeline(pipeline); +} + +TEST_F(HttpPipelinedHostImplTest, IgnoresUnusablePipeline) { + MockPipeline* pipeline = AddTestPipeline(1, false, true); + + EXPECT_FALSE(host_->IsExistingPipelineAvailable()); + EXPECT_EQ(NULL, host_->CreateStreamOnExistingPipeline()); + + ClearTestPipeline(pipeline); +} + +TEST_F(HttpPipelinedHostImplTest, IgnoresInactivePipeline) { + MockPipeline* pipeline = AddTestPipeline(1, true, false); + + EXPECT_FALSE(host_->IsExistingPipelineAvailable()); + EXPECT_EQ(NULL, host_->CreateStreamOnExistingPipeline()); + + ClearTestPipeline(pipeline); +} + +TEST_F(HttpPipelinedHostImplTest, IgnoresFullPipeline) { + MockPipeline* pipeline = AddTestPipeline( + HttpPipelinedHostImpl::max_pipeline_depth(), true, true); + + EXPECT_FALSE(host_->IsExistingPipelineAvailable()); + EXPECT_EQ(NULL, host_->CreateStreamOnExistingPipeline()); + + ClearTestPipeline(pipeline); +} + +TEST_F(HttpPipelinedHostImplTest, PicksLeastLoadedPipeline) { + MockPipeline* full_pipeline = AddTestPipeline( + HttpPipelinedHostImpl::max_pipeline_depth(), true, true); + MockPipeline* usable_pipeline = AddTestPipeline( + HttpPipelinedHostImpl::max_pipeline_depth() - 1, true, true); + MockPipeline* empty_pipeline = AddTestPipeline(0, true, true); + + EXPECT_TRUE(host_->IsExistingPipelineAvailable()); + EXPECT_CALL(*empty_pipeline, CreateNewStream()) + .Times(1) + .WillOnce(ReturnNull()); + EXPECT_EQ(NULL, host_->CreateStreamOnExistingPipeline()); + + ClearTestPipeline(full_pipeline); + ClearTestPipeline(usable_pipeline); + ClearTestPipeline(empty_pipeline); +} + +TEST_F(HttpPipelinedHostImplTest, OpensUpOnPipelineSuccess) { + SetCapability(HttpPipelinedHost::UNKNOWN); + MockPipeline* pipeline = AddTestPipeline(1, true, true); + + EXPECT_EQ(NULL, host_->CreateStreamOnExistingPipeline()); + EXPECT_CALL(delegate_, OnHostHasAdditionalCapacity(host_.get())) + .Times(1); + host_->OnPipelineFeedback(pipeline, HttpPipelinedConnection::OK); + + EXPECT_CALL(*pipeline, CreateNewStream()) + .Times(1) + .WillOnce(Return(kDummyStream)); + EXPECT_EQ(kDummyStream, host_->CreateStreamOnExistingPipeline()); + + EXPECT_CALL(delegate_, OnHostHasAdditionalCapacity(host_.get())) + .Times(1); + ClearTestPipeline(pipeline); +} + +TEST_F(HttpPipelinedHostImplTest, OpensAllPipelinesOnPipelineSuccess) { + SetCapability(HttpPipelinedHost::UNKNOWN); + MockPipeline* pipeline1 = AddTestPipeline(1, false, true); + MockPipeline* pipeline2 = AddTestPipeline(1, true, true); + + EXPECT_EQ(NULL, host_->CreateStreamOnExistingPipeline()); + EXPECT_CALL(delegate_, OnHostHasAdditionalCapacity(host_.get())) + .Times(1); + host_->OnPipelineFeedback(pipeline1, HttpPipelinedConnection::OK); + + EXPECT_CALL(*pipeline2, CreateNewStream()) + .Times(1) + .WillOnce(Return(kDummyStream)); + EXPECT_EQ(kDummyStream, host_->CreateStreamOnExistingPipeline()); + + EXPECT_CALL(delegate_, OnHostHasAdditionalCapacity(host_.get())) + .Times(2); + ClearTestPipeline(pipeline1); + ClearTestPipeline(pipeline2); +} + +TEST_F(HttpPipelinedHostImplTest, ShutsDownOnOldVersion) { + SetCapability(HttpPipelinedHost::UNKNOWN); + MockPipeline* pipeline = AddTestPipeline(1, true, true); + + EXPECT_EQ(NULL, host_->CreateStreamOnExistingPipeline()); + EXPECT_CALL(delegate_, OnHostHasAdditionalCapacity(host_.get())) + .Times(0); + EXPECT_CALL(delegate_, + OnHostDeterminedCapability(host_.get(), + HttpPipelinedHost::INCAPABLE)) + .Times(1); + host_->OnPipelineFeedback(pipeline, + HttpPipelinedConnection::OLD_HTTP_VERSION); + + ClearTestPipeline(pipeline); + EXPECT_EQ(NULL, host_->CreateStreamOnNewPipeline( + kDummyConnection, ssl_config_, proxy_info_, net_log_, true)); +} + +TEST_F(HttpPipelinedHostImplTest, ConnectionCloseHasNoEffect) { + SetCapability(HttpPipelinedHost::UNKNOWN); + MockPipeline* pipeline = AddTestPipeline(1, true, true); + + EXPECT_CALL(delegate_, OnHostHasAdditionalCapacity(host_.get())) + .Times(0); + EXPECT_CALL(delegate_, OnHostDeterminedCapability(host_.get(), _)) + .Times(0); + host_->OnPipelineFeedback(pipeline, + HttpPipelinedConnection::MUST_CLOSE_CONNECTION); + EXPECT_EQ(NULL, host_->CreateStreamOnExistingPipeline()); + + EXPECT_CALL(delegate_, OnHostHasAdditionalCapacity(host_.get())) + .Times(1); + ClearTestPipeline(pipeline); +} + +TEST_F(HttpPipelinedHostImplTest, SuccessesLeadToCapable) { + SetCapability(HttpPipelinedHost::UNKNOWN); + MockPipeline* pipeline = AddTestPipeline(1, true, true); + + EXPECT_CALL(delegate_, OnHostHasAdditionalCapacity(host_.get())) + .Times(1); + EXPECT_CALL(delegate_, + OnHostDeterminedCapability(host_.get(), + HttpPipelinedHost::CAPABLE)) + .Times(1); + host_->OnPipelineFeedback(pipeline, HttpPipelinedConnection::OK); + + pipeline->SetState(3, true, true); + host_->OnPipelineFeedback(pipeline, HttpPipelinedConnection::OK); + host_->OnPipelineFeedback(pipeline, HttpPipelinedConnection::OK); + + EXPECT_CALL(delegate_, OnHostHasAdditionalCapacity(host_.get())) + .Times(1); + ClearTestPipeline(pipeline); +} + +} // anonymous namespace + +} // namespace net diff --git a/net/http/http_pipelined_host_pool.cc b/net/http/http_pipelined_host_pool.cc index bd238d8..fa705a0 100644 --- a/net/http/http_pipelined_host_pool.cc +++ b/net/http/http_pipelined_host_pool.cc @@ -6,19 +6,46 @@ #include "base/logging.h" #include "base/stl_util.h" -#include "net/http/http_pipelined_host.h" -#include "net/http/http_stream_factory_impl.h" +#include "net/http/http_pipelined_host_impl.h" namespace net { -HttpPipelinedHostPool::HttpPipelinedHostPool(HttpStreamFactoryImpl* factory) - : factory_(factory) { +// TODO(simonjam): Run experiments with different values of this to see what +// value is good at avoiding evictions without eating too much memory. Until +// then, this is just a bad guess. +static const int kNumHostsToRemember = 200; + +class HttpPipelinedHostImplFactory : public HttpPipelinedHost::Factory { + public: + virtual HttpPipelinedHost* CreateNewHost( + HttpPipelinedHost::Delegate* delegate, const HostPortPair& origin, + HttpPipelinedConnection::Factory* factory, + HttpPipelinedHost::Capability capability) OVERRIDE { + return new HttpPipelinedHostImpl(delegate, origin, factory, capability); + } +}; + +HttpPipelinedHostPool::HttpPipelinedHostPool( + Delegate* delegate, + HttpPipelinedHost::Factory* factory) + : delegate_(delegate), + factory_(factory), + known_capability_map_(kNumHostsToRemember) { + if (!factory) { + factory_.reset(new HttpPipelinedHostImplFactory); + } } HttpPipelinedHostPool::~HttpPipelinedHostPool() { CHECK(host_map_.empty()); } +bool HttpPipelinedHostPool::IsHostEligibleForPipelining( + const HostPortPair& origin) { + HttpPipelinedHost::Capability capability = GetHostCapability(origin); + return capability != HttpPipelinedHost::INCAPABLE; +} + HttpPipelinedStream* HttpPipelinedHostPool::CreateStreamOnNewPipeline( const HostPortPair& origin, ClientSocketHandle* connection, @@ -27,6 +54,9 @@ HttpPipelinedStream* HttpPipelinedHostPool::CreateStreamOnNewPipeline( const BoundNetLog& net_log, bool was_npn_negotiated) { HttpPipelinedHost* host = GetPipelinedHost(origin, true); + if (!host) { + return NULL; + } return host->CreateStreamOnNewPipeline(connection, used_ssl_config, used_proxy_info, net_log, was_npn_negotiated); @@ -52,14 +82,21 @@ bool HttpPipelinedHostPool::IsExistingPipelineAvailableForOrigin( HttpPipelinedHost* HttpPipelinedHostPool::GetPipelinedHost( const HostPortPair& origin, bool create_if_not_found) { - HostMap::iterator it = host_map_.find(origin); - if (it != host_map_.end()) { - CHECK(it->second); - return it->second; + HostMap::iterator host_it = host_map_.find(origin); + if (host_it != host_map_.end()) { + CHECK(host_it->second); + return host_it->second; } else if (!create_if_not_found) { return NULL; } - HttpPipelinedHost* host = new HttpPipelinedHost(this, origin, NULL); + + HttpPipelinedHost::Capability capability = GetHostCapability(origin); + if (capability == HttpPipelinedHost::INCAPABLE) { + return NULL; + } + + HttpPipelinedHost* host = factory_->CreateNewHost( + this, origin, NULL, capability); host_map_[origin] = host; return host; } @@ -67,14 +104,33 @@ HttpPipelinedHost* HttpPipelinedHostPool::GetPipelinedHost( void HttpPipelinedHostPool::OnHostIdle(HttpPipelinedHost* host) { const HostPortPair& origin = host->origin(); CHECK(ContainsKey(host_map_, origin)); - // TODO(simonjam): We should remember the pipeline state for each host. host_map_.erase(origin); delete host; } void HttpPipelinedHostPool::OnHostHasAdditionalCapacity( HttpPipelinedHost* host) { - factory_->OnHttpPipelinedHostHasAdditionalCapacity(host->origin()); + delegate_->OnHttpPipelinedHostHasAdditionalCapacity(host->origin()); +} + +void HttpPipelinedHostPool::OnHostDeterminedCapability( + HttpPipelinedHost* host, + HttpPipelinedHost::Capability capability) { + CapabilityMap::iterator known_it = known_capability_map_.Get(host->origin()); + if (known_it == known_capability_map_.end() || + known_it->second != HttpPipelinedHost::INCAPABLE) { + known_capability_map_.Put(host->origin(), capability); + } +} + +HttpPipelinedHost::Capability HttpPipelinedHostPool::GetHostCapability( + const HostPortPair& origin) { + HttpPipelinedHost::Capability capability = HttpPipelinedHost::UNKNOWN; + CapabilityMap::const_iterator it = known_capability_map_.Get(origin); + if (it != known_capability_map_.end()) { + capability = it->second; + } + return capability; } } // namespace net diff --git a/net/http/http_pipelined_host_pool.h b/net/http/http_pipelined_host_pool.h index 5e7c146..3be0cb5 100644 --- a/net/http/http_pipelined_host_pool.h +++ b/net/http/http_pipelined_host_pool.h @@ -9,21 +9,37 @@ #include <map> #include "base/basictypes.h" +#include "base/gtest_prod_util.h" +#include "base/memory/mru_cache.h" +#include "base/memory/scoped_ptr.h" #include "net/http/http_pipelined_host.h" namespace net { class HostPortPair; class HttpPipelinedStream; -class HttpStreamFactoryImpl; // Manages all of the pipelining state for specific host with active pipelined // HTTP requests. Manages connection jobs, constructs pipelined streams, and // assigns requests to the least loaded pipelined connection. -class HttpPipelinedHostPool : public HttpPipelinedHost::Delegate { +class NET_EXPORT_PRIVATE HttpPipelinedHostPool + : public HttpPipelinedHost::Delegate { public: - explicit HttpPipelinedHostPool(HttpStreamFactoryImpl* factory); - ~HttpPipelinedHostPool(); + class Delegate { + public: + // Called when a HttpPipelinedHost has new capacity. Attempts to allocate + // any pending pipeline-capable requests to pipelines. + virtual void OnHttpPipelinedHostHasAdditionalCapacity( + const HostPortPair& origin) = 0; + }; + + HttpPipelinedHostPool(Delegate* delegate, + HttpPipelinedHost::Factory* factory); + virtual ~HttpPipelinedHostPool(); + + // Returns true if pipelining might work for |origin|. Generally, this returns + // true, unless |origin| is known to have failed pipelining recently. + bool IsHostEligibleForPipelining(const HostPortPair& origin); // Constructs a new pipeline on |connection| and returns a new // HttpPipelinedStream that uses it. @@ -49,14 +65,24 @@ class HttpPipelinedHostPool : public HttpPipelinedHost::Delegate { virtual void OnHostHasAdditionalCapacity(HttpPipelinedHost* host) OVERRIDE; + virtual void OnHostDeterminedCapability( + HttpPipelinedHost* host, + HttpPipelinedHost::Capability capability) OVERRIDE; + private: + typedef base::MRUCache<HostPortPair, + HttpPipelinedHost::Capability> CapabilityMap; typedef std::map<const HostPortPair, HttpPipelinedHost*> HostMap; HttpPipelinedHost* GetPipelinedHost(const HostPortPair& origin, bool create_if_not_found); - HttpStreamFactoryImpl* factory_; + HttpPipelinedHost::Capability GetHostCapability(const HostPortPair& origin); + + Delegate* delegate_; + scoped_ptr<HttpPipelinedHost::Factory> factory_; HostMap host_map_; + CapabilityMap known_capability_map_; DISALLOW_COPY_AND_ASSIGN(HttpPipelinedHostPool); }; diff --git a/net/http/http_pipelined_host_pool_unittest.cc b/net/http/http_pipelined_host_pool_unittest.cc new file mode 100644 index 0000000..928e4e9 --- /dev/null +++ b/net/http/http_pipelined_host_pool_unittest.cc @@ -0,0 +1,185 @@ +// 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/http/http_pipelined_host_pool.h" + +#include "base/memory/scoped_ptr.h" +#include "net/base/ssl_config_service.h" +#include "net/http/http_pipelined_host.h" +#include "net/proxy/proxy_info.h" +#include "testing/gmock/include/gmock/gmock.h" +#include "testing/gtest/include/gtest/gtest.h" + +using testing::_; +using testing::Ref; +using testing::Return; +using testing::ReturnNull; + +namespace net { + +namespace { + +ClientSocketHandle* kDummyConnection = + reinterpret_cast<ClientSocketHandle*>(188); +HttpPipelinedStream* kDummyStream = + reinterpret_cast<HttpPipelinedStream*>(99); + +class MockPoolDelegate : public HttpPipelinedHostPool::Delegate { + public: + MOCK_METHOD1(OnHttpPipelinedHostHasAdditionalCapacity, + void(const HostPortPair& origin)); +}; + +class MockHostFactory : public HttpPipelinedHost::Factory { + public: + MOCK_METHOD4(CreateNewHost, HttpPipelinedHost*( + HttpPipelinedHost::Delegate* delegate, const HostPortPair& origin, + HttpPipelinedConnection::Factory* factory, + HttpPipelinedHost::Capability capability)); +}; + +class MockHost : public HttpPipelinedHost { + public: + MockHost(const HostPortPair& origin) + : origin_(origin) { + } + + MOCK_METHOD5(CreateStreamOnNewPipeline, HttpPipelinedStream*( + ClientSocketHandle* connection, + const SSLConfig& used_ssl_config, + const ProxyInfo& used_proxy_info, + const BoundNetLog& net_log, + bool was_npn_negotiated)); + MOCK_METHOD0(CreateStreamOnExistingPipeline, HttpPipelinedStream*()); + MOCK_CONST_METHOD0(IsExistingPipelineAvailable, bool()); + + virtual const HostPortPair& origin() const OVERRIDE { return origin_; } + + private: + HostPortPair origin_; +}; + +class HttpPipelinedHostPoolTest : public testing::Test { + public: + HttpPipelinedHostPoolTest() + : origin_("host", 123), + factory_(new MockHostFactory), // Owned by pool_. + host_(new MockHost(origin_)), // Owned by pool_. + pool_(new HttpPipelinedHostPool(&delegate_, factory_)), + was_npn_negotiated_(false) { + } + + void CreateDummyStream() { + EXPECT_CALL(*host_, CreateStreamOnNewPipeline(kDummyConnection, + Ref(ssl_config_), + Ref(proxy_info_), + Ref(net_log_), + was_npn_negotiated_)) + .Times(1) + .WillOnce(Return(kDummyStream)); + EXPECT_EQ(kDummyStream, + pool_->CreateStreamOnNewPipeline(origin_, kDummyConnection, + ssl_config_, proxy_info_, + net_log_, was_npn_negotiated_)); + } + + HostPortPair origin_; + MockPoolDelegate delegate_; + MockHostFactory* factory_; + MockHost* host_; + scoped_ptr<HttpPipelinedHostPool> pool_; + + const SSLConfig ssl_config_; + const ProxyInfo proxy_info_; + const BoundNetLog net_log_; + bool was_npn_negotiated_; +}; + +TEST_F(HttpPipelinedHostPoolTest, DefaultUnknown) { + EXPECT_TRUE(pool_->IsHostEligibleForPipelining(origin_)); + EXPECT_CALL(*factory_, CreateNewHost(pool_.get(), Ref(origin_), _, + HttpPipelinedHost::UNKNOWN)) + .Times(1) + .WillOnce(Return(host_)); + + CreateDummyStream(); + pool_->OnHostIdle(host_); +} + +TEST_F(HttpPipelinedHostPoolTest, RemembersIncapable) { + EXPECT_CALL(*factory_, CreateNewHost(pool_.get(), Ref(origin_), _, + HttpPipelinedHost::UNKNOWN)) + .Times(1) + .WillOnce(Return(host_)); + + CreateDummyStream(); + pool_->OnHostDeterminedCapability(host_, HttpPipelinedHost::INCAPABLE); + pool_->OnHostIdle(host_); + EXPECT_FALSE(pool_->IsHostEligibleForPipelining(origin_)); + EXPECT_CALL(*factory_, CreateNewHost(pool_.get(), Ref(origin_), _, + HttpPipelinedHost::INCAPABLE)) + .Times(0); + EXPECT_EQ(NULL, + pool_->CreateStreamOnNewPipeline(origin_, kDummyConnection, + ssl_config_, proxy_info_, net_log_, + was_npn_negotiated_)); +} + +TEST_F(HttpPipelinedHostPoolTest, RemembersCapable) { + EXPECT_CALL(*factory_, CreateNewHost(pool_.get(), Ref(origin_), _, + HttpPipelinedHost::UNKNOWN)) + .Times(1) + .WillOnce(Return(host_)); + + CreateDummyStream(); + pool_->OnHostDeterminedCapability(host_, HttpPipelinedHost::CAPABLE); + pool_->OnHostIdle(host_); + EXPECT_TRUE(pool_->IsHostEligibleForPipelining(origin_)); + + host_ = new MockHost(origin_); + EXPECT_CALL(*factory_, CreateNewHost(pool_.get(), Ref(origin_), _, + HttpPipelinedHost::CAPABLE)) + .Times(1) + .WillOnce(Return(host_)); + CreateDummyStream(); + pool_->OnHostIdle(host_); +} + +TEST_F(HttpPipelinedHostPoolTest, IncapableIsSticky) { + EXPECT_CALL(*factory_, CreateNewHost(pool_.get(), Ref(origin_), _, + HttpPipelinedHost::UNKNOWN)) + .Times(1) + .WillOnce(Return(host_)); + + CreateDummyStream(); + pool_->OnHostDeterminedCapability(host_, HttpPipelinedHost::CAPABLE); + pool_->OnHostDeterminedCapability(host_, HttpPipelinedHost::INCAPABLE); + pool_->OnHostDeterminedCapability(host_, HttpPipelinedHost::CAPABLE); + pool_->OnHostIdle(host_); + EXPECT_FALSE(pool_->IsHostEligibleForPipelining(origin_)); +} + +TEST_F(HttpPipelinedHostPoolTest, RemainsUnknownWithoutFeedback) { + EXPECT_CALL(*factory_, CreateNewHost(pool_.get(), Ref(origin_), _, + HttpPipelinedHost::UNKNOWN)) + .Times(1) + .WillOnce(Return(host_)); + + CreateDummyStream(); + pool_->OnHostIdle(host_); + EXPECT_TRUE(pool_->IsHostEligibleForPipelining(origin_)); + + host_ = new MockHost(origin_); + EXPECT_CALL(*factory_, CreateNewHost(pool_.get(), Ref(origin_), _, + HttpPipelinedHost::UNKNOWN)) + .Times(1) + .WillOnce(Return(host_)); + + CreateDummyStream(); + pool_->OnHostIdle(host_); +} + +} // anonymous namespace + +} // namespace net diff --git a/net/http/http_pipelined_host_unittest.cc b/net/http/http_pipelined_host_unittest.cc deleted file mode 100644 index de78e9e..0000000 --- a/net/http/http_pipelined_host_unittest.cc +++ /dev/null @@ -1,201 +0,0 @@ -// 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/http/http_pipelined_host.h" - -#include "net/base/ssl_config_service.h" -#include "net/http/http_pipelined_connection.h" -#include "net/proxy/proxy_info.h" -#include "testing/gmock/include/gmock/gmock.h" -#include "testing/gtest/include/gtest/gtest.h" - -using testing::_; -using testing::NiceMock; -using testing::Ref; -using testing::Return; -using testing::ReturnNull; - -static const int kMaxCapacity = 3; - -namespace net { - -static ClientSocketHandle* kDummyConnection = - reinterpret_cast<ClientSocketHandle*>(84); -static HttpPipelinedStream* kDummyStream = - reinterpret_cast<HttpPipelinedStream*>(42); - -class MockHostDelegate : public HttpPipelinedHost::Delegate { - public: - MOCK_METHOD1(OnHostIdle, void(HttpPipelinedHost* host)); - MOCK_METHOD1(OnHostHasAdditionalCapacity, void(HttpPipelinedHost* host)); -}; - -class MockPipelineFactory : public HttpPipelinedConnection::Factory { - public: - MOCK_METHOD6(CreateNewPipeline, HttpPipelinedConnection*( - ClientSocketHandle* connection, - HttpPipelinedConnection::Delegate* delegate, - const SSLConfig& used_ssl_config, - const ProxyInfo& used_proxy_info, - const BoundNetLog& net_log, - bool was_npn_negotiated)); -}; - -class MockPipeline : public HttpPipelinedConnection { - public: - MockPipeline(int depth, bool usable, bool active) - : depth_(depth), - usable_(usable), - active_(active) { - } - - void SetState(int depth, bool usable, bool active) { - depth_ = depth; - usable_ = usable; - active_ = active; - } - - virtual int depth() const OVERRIDE { return depth_; } - virtual bool usable() const OVERRIDE { return usable_; } - virtual bool active() const OVERRIDE { return active_; } - - MOCK_METHOD0(CreateNewStream, HttpPipelinedStream*()); - MOCK_METHOD1(OnStreamDeleted, void(int pipeline_id)); - MOCK_CONST_METHOD0(used_ssl_config, const SSLConfig&()); - MOCK_CONST_METHOD0(used_proxy_info, const ProxyInfo&()); - MOCK_CONST_METHOD0(source, const NetLog::Source&()); - MOCK_CONST_METHOD0(was_npn_negotiated, bool()); - - private: - int depth_; - bool usable_; - bool active_; -}; - -class HttpPipelinedHostTest : public testing::Test { - public: - HttpPipelinedHostTest() - : origin_("host", 123), - factory_(new MockPipelineFactory), // Owned by host_. - host_(&delegate_, origin_, factory_) { - } - - MockPipeline* AddTestPipeline(int depth, bool usable, bool active) { - MockPipeline* pipeline = new MockPipeline(depth, usable, active); - EXPECT_CALL(*factory_, CreateNewPipeline(kDummyConnection, &host_, - Ref(ssl_config_), Ref(proxy_info_), - Ref(net_log_), true)) - .Times(1) - .WillOnce(Return(pipeline)); - EXPECT_CALL(*pipeline, CreateNewStream()) - .Times(1) - .WillOnce(Return(kDummyStream)); - EXPECT_EQ(kDummyStream, host_.CreateStreamOnNewPipeline( - kDummyConnection, ssl_config_, proxy_info_, net_log_, true)); - return pipeline; - } - - void ClearTestPipeline(MockPipeline* pipeline) { - pipeline->SetState(0, true, true); - host_.OnPipelineHasCapacity(pipeline); - } - - NiceMock<MockHostDelegate> delegate_; - HostPortPair origin_; - MockPipelineFactory* factory_; - HttpPipelinedHost host_; - - SSLConfig ssl_config_; - ProxyInfo proxy_info_; - BoundNetLog net_log_; -}; - -TEST_F(HttpPipelinedHostTest, Delegate) { - EXPECT_TRUE(origin_.Equals(host_.origin())); -} - -TEST_F(HttpPipelinedHostTest, OnHostIdle) { - MockPipeline* pipeline = AddTestPipeline(0, false, true); - - EXPECT_CALL(delegate_, OnHostHasAdditionalCapacity(&host_)) - .Times(0); - EXPECT_CALL(delegate_, OnHostIdle(&host_)) - .Times(1); - host_.OnPipelineHasCapacity(pipeline); -} - -TEST_F(HttpPipelinedHostTest, OnHostHasAdditionalCapacity) { - MockPipeline* pipeline = AddTestPipeline(1, true, true); - - EXPECT_CALL(delegate_, OnHostHasAdditionalCapacity(&host_)) - .Times(2); - EXPECT_CALL(delegate_, OnHostIdle(&host_)) - .Times(0); - - host_.OnPipelineHasCapacity(pipeline); - - EXPECT_CALL(delegate_, OnHostIdle(&host_)) - .Times(1); - ClearTestPipeline(pipeline); -} - -TEST_F(HttpPipelinedHostTest, IgnoresUnusablePipeline) { - MockPipeline* pipeline = AddTestPipeline(1, false, true); - - EXPECT_FALSE(host_.IsExistingPipelineAvailable()); - EXPECT_EQ(NULL, host_.CreateStreamOnExistingPipeline()); - - ClearTestPipeline(pipeline); -} - -TEST_F(HttpPipelinedHostTest, IgnoresInactivePipeline) { - MockPipeline* pipeline = AddTestPipeline(1, true, false); - - EXPECT_FALSE(host_.IsExistingPipelineAvailable()); - EXPECT_EQ(NULL, host_.CreateStreamOnExistingPipeline()); - - ClearTestPipeline(pipeline); -} - -TEST_F(HttpPipelinedHostTest, IgnoresFullPipeline) { - MockPipeline* pipeline = AddTestPipeline(kMaxCapacity, true, true); - - EXPECT_FALSE(host_.IsExistingPipelineAvailable()); - EXPECT_EQ(NULL, host_.CreateStreamOnExistingPipeline()); - - ClearTestPipeline(pipeline); -} - -TEST_F(HttpPipelinedHostTest, PicksLeastLoadedPipeline) { - MockPipeline* full_pipeline = AddTestPipeline(kMaxCapacity, true, true); - MockPipeline* usable_pipeline = AddTestPipeline(kMaxCapacity - 1, true, true); - MockPipeline* empty_pipeline = AddTestPipeline(0, true, true); - - EXPECT_TRUE(host_.IsExistingPipelineAvailable()); - EXPECT_CALL(*empty_pipeline, CreateNewStream()) - .Times(1) - .WillOnce(ReturnNull()); - EXPECT_EQ(NULL, host_.CreateStreamOnExistingPipeline()); - - ClearTestPipeline(full_pipeline); - ClearTestPipeline(usable_pipeline); - ClearTestPipeline(empty_pipeline); -} - -TEST_F(HttpPipelinedHostTest, EmptyPipelineIsRemoved) { - MockPipeline* empty_pipeline = AddTestPipeline(0, true, true); - - EXPECT_TRUE(host_.IsExistingPipelineAvailable()); - EXPECT_CALL(*empty_pipeline, CreateNewStream()) - .Times(1) - .WillOnce(Return(kDummyStream)); - EXPECT_EQ(kDummyStream, host_.CreateStreamOnExistingPipeline()); - - ClearTestPipeline(empty_pipeline); - - EXPECT_FALSE(host_.IsExistingPipelineAvailable()); - EXPECT_EQ(NULL, host_.CreateStreamOnExistingPipeline()); -} - -} // namespace net diff --git a/net/http/http_pipelined_network_transaction_unittest.cc b/net/http/http_pipelined_network_transaction_unittest.cc index c342b2c..70e8d657 100644 --- a/net/http/http_pipelined_network_transaction_unittest.cc +++ b/net/http/http_pipelined_network_transaction_unittest.cc @@ -97,7 +97,7 @@ class HttpPipelinedNetworkTransactionTest : public testing::Test { EXPECT_EQ(OK, transaction.Read(buffer.get(), expected.size(), &callback_)); } - void CompleteTwoRequests() { + void CompleteTwoRequests(int data_index, int stop_at_step) { scoped_ptr<HttpNetworkTransaction> one_transaction( new HttpNetworkTransaction(session_.get())); TestOldCompletionCallback one_callback; @@ -117,9 +117,10 @@ class HttpPipelinedNetworkTransactionTest : public testing::Test { EXPECT_EQ(ERR_IO_PENDING, one_transaction->Read(buffer.get(), 8, &one_read_callback)); - data_vector_[0]->RunFor(2); + data_vector_[data_index]->SetStop(stop_at_step); + data_vector_[data_index]->Run(); EXPECT_EQ(8, one_read_callback.WaitForResult()); - data_vector_[0]->SetStop(10); + data_vector_[data_index]->SetStop(10); std::string actual(buffer->data(), 8); EXPECT_THAT(actual, StrEq("one.html")); EXPECT_EQ(OK, one_transaction->Read(buffer.get(), 8, &one_read_callback)); @@ -128,6 +129,44 @@ class HttpPipelinedNetworkTransactionTest : public testing::Test { ExpectResponse("two.html", two_transaction); } + void CompleteFourRequests() { + scoped_ptr<HttpNetworkTransaction> one_transaction( + new HttpNetworkTransaction(session_.get())); + TestOldCompletionCallback one_callback; + EXPECT_EQ(ERR_IO_PENDING, + one_transaction->Start(GetRequestInfo("one.html"), &one_callback, + BoundNetLog())); + EXPECT_EQ(OK, one_callback.WaitForResult()); + + HttpNetworkTransaction two_transaction(session_.get()); + TestOldCompletionCallback two_callback; + EXPECT_EQ(ERR_IO_PENDING, + two_transaction.Start(GetRequestInfo("two.html"), &two_callback, + BoundNetLog())); + + HttpNetworkTransaction three_transaction(session_.get()); + TestOldCompletionCallback three_callback; + EXPECT_EQ(ERR_IO_PENDING, + three_transaction.Start(GetRequestInfo("three.html"), + &three_callback, BoundNetLog())); + + HttpNetworkTransaction four_transaction(session_.get()); + TestOldCompletionCallback four_callback; + EXPECT_EQ(ERR_IO_PENDING, + four_transaction.Start(GetRequestInfo("four.html"), + &four_callback, BoundNetLog())); + + ExpectResponse("one.html", *one_transaction.get()); + EXPECT_EQ(OK, two_callback.WaitForResult()); + ExpectResponse("two.html", two_transaction); + EXPECT_EQ(OK, three_callback.WaitForResult()); + ExpectResponse("three.html", three_transaction); + + one_transaction.reset(); + EXPECT_EQ(OK, four_callback.WaitForResult()); + ExpectResponse("four.html", four_transaction); + } + DeterministicMockClientSocketFactory factory_; ClientSocketPoolHistograms histograms_; MockTransportClientSocketPool pool_; @@ -188,7 +227,7 @@ TEST_F(HttpPipelinedNetworkTransactionTest, ReusePipeline) { }; AddExpectedConnection(reads, arraysize(reads), writes, arraysize(writes)); - CompleteTwoRequests(); + CompleteTwoRequests(0, 5); } TEST_F(HttpPipelinedNetworkTransactionTest, ReusesOnSpaceAvailable) { @@ -226,41 +265,7 @@ TEST_F(HttpPipelinedNetworkTransactionTest, ReusesOnSpaceAvailable) { }; AddExpectedConnection(reads, arraysize(reads), writes, arraysize(writes)); - scoped_ptr<HttpNetworkTransaction> one_transaction( - new HttpNetworkTransaction(session_.get())); - TestOldCompletionCallback one_callback; - EXPECT_EQ(ERR_IO_PENDING, - one_transaction->Start(GetRequestInfo("one.html"), &one_callback, - BoundNetLog())); - EXPECT_EQ(OK, one_callback.WaitForResult()); - - HttpNetworkTransaction two_transaction(session_.get()); - TestOldCompletionCallback two_callback; - EXPECT_EQ(ERR_IO_PENDING, - two_transaction.Start(GetRequestInfo("two.html"), &two_callback, - BoundNetLog())); - - HttpNetworkTransaction three_transaction(session_.get()); - TestOldCompletionCallback three_callback; - EXPECT_EQ(ERR_IO_PENDING, - three_transaction.Start(GetRequestInfo("three.html"), - &three_callback, BoundNetLog())); - - HttpNetworkTransaction four_transaction(session_.get()); - TestOldCompletionCallback four_callback; - EXPECT_EQ(ERR_IO_PENDING, - four_transaction.Start(GetRequestInfo("four.html"), &four_callback, - BoundNetLog())); - - ExpectResponse("one.html", *one_transaction.get()); - EXPECT_EQ(OK, two_callback.WaitForResult()); - ExpectResponse("two.html", two_transaction); - EXPECT_EQ(OK, three_callback.WaitForResult()); - ExpectResponse("three.html", three_transaction); - - one_transaction.reset(); - EXPECT_EQ(OK, four_callback.WaitForResult()); - ExpectResponse("four.html", four_transaction); + CompleteFourRequests(); ClientSocketPoolManager::set_max_sockets_per_group(old_max_sockets); } @@ -272,14 +277,11 @@ TEST_F(HttpPipelinedNetworkTransactionTest, UnknownSizeEvictsToNewPipeline) { MockWrite(false, 0, "GET /one.html HTTP/1.1\r\n" "Host: localhost\r\n" "Connection: keep-alive\r\n\r\n"), - MockWrite(false, 2, "GET /two.html HTTP/1.1\r\n" - "Host: localhost\r\n" - "Connection: keep-alive\r\n\r\n"), }; MockRead reads[] = { MockRead(false, 1, "HTTP/1.1 200 OK\r\n\r\n"), - MockRead(true, 3, "one.html"), - MockRead(false, OK, 4), + MockRead(true, 2, "one.html"), + MockRead(false, OK, 3), }; AddExpectedConnection(reads, arraysize(reads), writes, arraysize(writes)); @@ -295,7 +297,7 @@ TEST_F(HttpPipelinedNetworkTransactionTest, UnknownSizeEvictsToNewPipeline) { }; AddExpectedConnection(reads2, arraysize(reads2), writes2, arraysize(writes2)); - CompleteTwoRequests(); + CompleteTwoRequests(0, 3); } TEST_F(HttpPipelinedNetworkTransactionTest, ConnectionCloseEvictToNewPipeline) { @@ -329,7 +331,7 @@ TEST_F(HttpPipelinedNetworkTransactionTest, ConnectionCloseEvictToNewPipeline) { }; AddExpectedConnection(reads2, arraysize(reads2), writes2, arraysize(writes2)); - CompleteTwoRequests(); + CompleteTwoRequests(0, 5); } TEST_F(HttpPipelinedNetworkTransactionTest, ErrorEvictsToNewPipeline) { @@ -508,6 +510,136 @@ TEST_F(HttpPipelinedNetworkTransactionTest, BasicHttpAuthentication) { ExpectResponse("one.html", transaction); } +TEST_F(HttpPipelinedNetworkTransactionTest, OldVersionDisablesPipelining) { + Initialize(); + + MockWrite writes[] = { + MockWrite(false, 0, "GET /pipelined.html HTTP/1.1\r\n" + "Host: localhost\r\n" + "Connection: keep-alive\r\n\r\n"), + }; + MockRead reads[] = { + MockRead(false, 1, "HTTP/1.0 200 OK\r\n"), + MockRead(false, 2, "Content-Length: 14\r\n\r\n"), + MockRead(false, 3, "pipelined.html"), + }; + AddExpectedConnection(reads, arraysize(reads), writes, arraysize(writes)); + + MockWrite writes2[] = { + MockWrite(false, 0, "GET /one.html HTTP/1.1\r\n" + "Host: localhost\r\n" + "Connection: keep-alive\r\n\r\n"), + }; + MockRead reads2[] = { + MockRead(false, 1, "HTTP/1.1 200 OK\r\n"), + MockRead(false, 2, "Content-Length: 8\r\n\r\n"), + MockRead(true, 3, "one.html"), + MockRead(false, OK, 4), + }; + AddExpectedConnection(reads2, arraysize(reads2), writes2, arraysize(writes2)); + + MockWrite writes3[] = { + MockWrite(false, 0, "GET /two.html HTTP/1.1\r\n" + "Host: localhost\r\n" + "Connection: keep-alive\r\n\r\n"), + }; + MockRead reads3[] = { + MockRead(false, 1, "HTTP/1.1 200 OK\r\n"), + MockRead(false, 2, "Content-Length: 8\r\n\r\n"), + MockRead(false, 3, "two.html"), + MockRead(false, OK, 4), + }; + AddExpectedConnection(reads3, arraysize(reads3), writes3, arraysize(writes3)); + + HttpNetworkTransaction one_transaction(session_.get()); + TestOldCompletionCallback one_callback; + EXPECT_EQ(ERR_IO_PENDING, + one_transaction.Start(GetRequestInfo("pipelined.html"), + &one_callback, BoundNetLog())); + EXPECT_EQ(OK, one_callback.WaitForResult()); + ExpectResponse("pipelined.html", one_transaction); + + CompleteTwoRequests(1, 4); +} + +TEST_F(HttpPipelinedNetworkTransactionTest, PipelinesImmediatelyIfKnownGood) { + // The first request gets us an HTTP/1.1. The next 3 test pipelining. When the + // 3rd request completes, we know pipelining is safe. After the first 4 + // complete, the 5th and 6th should then be immediately sent pipelined on a + // new HttpPipelinedConnection. + int old_max_sockets = ClientSocketPoolManager::max_sockets_per_group(); + ClientSocketPoolManager::set_max_sockets_per_group(1); + Initialize(); + + MockWrite writes[] = { + MockWrite(false, 0, "GET /one.html HTTP/1.1\r\n" + "Host: localhost\r\n" + "Connection: keep-alive\r\n\r\n"), + MockWrite(false, 4, "GET /two.html HTTP/1.1\r\n" + "Host: localhost\r\n" + "Connection: keep-alive\r\n\r\n"), + MockWrite(false, 7, "GET /three.html HTTP/1.1\r\n" + "Host: localhost\r\n" + "Connection: keep-alive\r\n\r\n"), + MockWrite(false, 12, "GET /four.html HTTP/1.1\r\n" + "Host: localhost\r\n" + "Connection: keep-alive\r\n\r\n"), + MockWrite(false, 16, "GET /second-pipeline-one.html HTTP/1.1\r\n" + "Host: localhost\r\n" + "Connection: keep-alive\r\n\r\n"), + MockWrite(false, 17, "GET /second-pipeline-two.html HTTP/1.1\r\n" + "Host: localhost\r\n" + "Connection: keep-alive\r\n\r\n"), + }; + MockRead reads[] = { + MockRead(false, 1, "HTTP/1.1 200 OK\r\n"), + MockRead(false, 2, "Content-Length: 8\r\n\r\n"), + MockRead(false, 3, "one.html"), + MockRead(false, 5, "HTTP/1.1 200 OK\r\n"), + MockRead(false, 6, "Content-Length: 8\r\n\r\n"), + MockRead(false, 8, "two.html"), + MockRead(false, 9, "HTTP/1.1 200 OK\r\n"), + MockRead(false, 10, "Content-Length: 10\r\n\r\n"), + MockRead(false, 11, "three.html"), + MockRead(false, 13, "HTTP/1.1 200 OK\r\n"), + MockRead(false, 14, "Content-Length: 9\r\n\r\n"), + MockRead(false, 15, "four.html"), + MockRead(true, 18, "HTTP/1.1 200 OK\r\n"), + MockRead(true, 19, "Content-Length: 24\r\n\r\n"), + MockRead(false, 20, "second-pipeline-one.html"), + MockRead(false, 21, "HTTP/1.1 200 OK\r\n"), + MockRead(false, 22, "Content-Length: 24\r\n\r\n"), + MockRead(false, 23, "second-pipeline-two.html"), + }; + AddExpectedConnection(reads, arraysize(reads), writes, arraysize(writes)); + + CompleteFourRequests(); + + HttpNetworkTransaction second_one_transaction(session_.get()); + TestOldCompletionCallback second_one_callback; + EXPECT_EQ(ERR_IO_PENDING, + second_one_transaction.Start( + GetRequestInfo("second-pipeline-one.html"), + &second_one_callback, BoundNetLog())); + MessageLoop::current()->RunAllPending(); + + HttpNetworkTransaction second_two_transaction(session_.get()); + TestOldCompletionCallback second_two_callback; + EXPECT_EQ(ERR_IO_PENDING, + second_two_transaction.Start( + GetRequestInfo("second-pipeline-two.html"), + &second_two_callback, BoundNetLog())); + + data_vector_[0]->RunFor(3); + EXPECT_EQ(OK, second_one_callback.WaitForResult()); + data_vector_[0]->StopAfter(100); + ExpectResponse("second-pipeline-one.html", second_one_transaction); + EXPECT_EQ(OK, second_two_callback.WaitForResult()); + ExpectResponse("second-pipeline-two.html", second_two_transaction); + + ClientSocketPoolManager::set_max_sockets_per_group(old_max_sockets); +} + } // anonymous namespace } // namespace net diff --git a/net/http/http_stream_factory_impl.cc b/net/http/http_stream_factory_impl.cc index 9593dc4..dd7776f 100644 --- a/net/http/http_stream_factory_impl.cc +++ b/net/http/http_stream_factory_impl.cc @@ -37,7 +37,7 @@ GURL UpgradeUrlToHttps(const GURL& original_url, int port) { HttpStreamFactoryImpl::HttpStreamFactoryImpl(HttpNetworkSession* session) : session_(session), - http_pipelined_host_pool_(this) {} + http_pipelined_host_pool_(this, NULL) {} HttpStreamFactoryImpl::~HttpStreamFactoryImpl() { DCHECK(request_map_.empty()); diff --git a/net/http/http_stream_factory_impl.h b/net/http/http_stream_factory_impl.h index ce1a04eb..8f13f3f 100644 --- a/net/http/http_stream_factory_impl.h +++ b/net/http/http_stream_factory_impl.h @@ -21,12 +21,14 @@ class HttpNetworkSession; class HttpPipelinedHost; class SpdySession; -class NET_EXPORT_PRIVATE HttpStreamFactoryImpl : public HttpStreamFactory { +class NET_EXPORT_PRIVATE HttpStreamFactoryImpl : + public HttpStreamFactory, + public HttpPipelinedHostPool::Delegate { public: explicit HttpStreamFactoryImpl(HttpNetworkSession* session); virtual ~HttpStreamFactoryImpl(); - // HttpStreamFactory Interface + // HttpStreamFactory interface virtual HttpStreamRequest* RequestStream( const HttpRequestInfo& info, const SSLConfig& server_ssl_config, @@ -42,10 +44,9 @@ class NET_EXPORT_PRIVATE HttpStreamFactoryImpl : public HttpStreamFactory { virtual void AddTLSIntolerantServer(const HostPortPair& server) OVERRIDE; virtual bool IsTLSIntolerantServer(const HostPortPair& server) const OVERRIDE; - // Called when a HttpPipelinedHost has new capacity. Attempts to allocate any - // pending pipeline-capable requests to pipelines. + // HttpPipelinedHostPool::Delegate interface virtual void OnHttpPipelinedHostHasAdditionalCapacity( - const HostPortPair& origin); + const HostPortPair& origin) OVERRIDE; private: class Request; diff --git a/net/http/http_stream_factory_impl_job.cc b/net/http/http_stream_factory_impl_job.cc index a6ca05e..e1bffb374 100644 --- a/net/http/http_stream_factory_impl_job.cc +++ b/net/http/http_stream_factory_impl_job.cc @@ -1137,7 +1137,7 @@ bool HttpStreamFactoryImpl::Job::IsOrphaned() const { return !IsPreconnecting() && !request_; } -bool HttpStreamFactoryImpl::Job::IsRequestEligibleForPipelining() const { +bool HttpStreamFactoryImpl::Job::IsRequestEligibleForPipelining() { if (!HttpStreamFactory::http_pipelining_enabled()) { return false; } @@ -1147,7 +1147,11 @@ bool HttpStreamFactoryImpl::Job::IsRequestEligibleForPipelining() const { if (using_ssl_) { return false; } - return request_info_.method == "GET" || request_info_.method == "HEAD"; + if (request_info_.method != "GET" && request_info_.method != "HEAD") { + return false; + } + return stream_factory_->http_pipelined_host_pool_.IsHostEligibleForPipelining( + origin_); } } // namespace net diff --git a/net/http/http_stream_factory_impl_job.h b/net/http/http_stream_factory_impl_job.h index b14897c..7c95135 100644 --- a/net/http/http_stream_factory_impl_job.h +++ b/net/http/http_stream_factory_impl_job.h @@ -192,7 +192,7 @@ class HttpStreamFactoryImpl::Job { // Should we force SPDY to run without SSL for this stream request. bool ShouldForceSpdyWithoutSSL() const; - bool IsRequestEligibleForPipelining() const; + bool IsRequestEligibleForPipelining(); // Record histograms of latency until Connect() completes. static void LogHttpConnectedMetrics(const ClientSocketHandle& handle); diff --git a/net/net.gyp b/net/net.gyp index 2694b27..777f0f6 100644 --- a/net/net.gyp +++ b/net/net.gyp @@ -427,8 +427,9 @@ 'http/http_pipelined_connection.h', 'http/http_pipelined_connection_impl.cc', 'http/http_pipelined_connection_impl.h', - 'http/http_pipelined_host.cc', 'http/http_pipelined_host.h', + 'http/http_pipelined_host_impl.cc', + 'http/http_pipelined_host_impl.h', 'http/http_pipelined_host_pool.cc', 'http/http_pipelined_host_pool.h', 'http/http_pipelined_stream.cc', @@ -1089,7 +1090,8 @@ 'http/http_network_layer_unittest.cc', 'http/http_network_transaction_unittest.cc', 'http/http_pipelined_connection_impl_unittest.cc', - 'http/http_pipelined_host_unittest.cc', + 'http/http_pipelined_host_impl_unittest.cc', + 'http/http_pipelined_host_pool_unittest.cc', 'http/http_pipelined_network_transaction_unittest.cc', 'http/http_proxy_client_socket_pool_unittest.cc', 'http/http_request_headers_unittest.cc', |