Basic HTTP pipelining support

net/http/http_stream_factory_impl_job.cc

Index: net/http/http_stream_factory_impl_job.cc
diff --git a/net/http/http_stream_factory_impl_job.cc b/net/http/http_stream_factory_impl_job.cc
index b55d1ac989efc19ec83dfdbc90732d2a6f125d10..ec74eb3c6e257bb5d38a56d6597f4976adae16f7 100644
--- a/net/http/http_stream_factory_impl_job.cc
+++ b/net/http/http_stream_factory_impl_job.cc
@@ -16,9 +16,14 @@
 #include "net/base/ssl_cert_request_info.h"
 #include "net/http/http_basic_stream.h"
 #include "net/http/http_network_session.h"
+#include "net/http/http_pipelined_connection.h"
+#include "net/http/http_pipelined_host.h"
+#include "net/http/http_pipelined_host_pool.h"
+#include "net/http/http_pipelined_stream.h"
 #include "net/http/http_proxy_client_socket.h"
 #include "net/http/http_proxy_client_socket_pool.h"
 #include "net/http/http_request_info.h"
+#include "net/http/http_stream_factory.h"
 #include "net/http/http_stream_factory_impl_request.h"
 #include "net/socket/client_socket_handle.h"
 #include "net/socket/client_socket_pool.h"
@@ -87,6 +92,7 @@ HttpStreamFactoryImpl::Job::Job(HttpStreamFactoryImpl* stream_factory,
       was_npn_negotiated_(false),
       num_streams_(0),
       spdy_session_direct_(false),
+      existing_available_pipeline_(NULL),
       ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)) {
   DCHECK(stream_factory);
   DCHECK(session);
@@ -587,6 +593,21 @@ int HttpStreamFactoryImpl::Job::DoInitConnection() {
   } else if (request_ && (using_ssl_ || ShouldForceSpdyWithoutSSL())) {
     // Update the spdy session key for the request that launched this job.
     request_->SetSpdySessionKey(spdy_session_key);
+  } else if (IsRequestEligibleForPipelining()) {
+    // TODO(simonjam): With pipelining, we might be better off using fewer
+    // connections and thus should make fewer preconnections. Explore
+    // preconnecting fewer than the requested num_connections.
+    HttpPipelinedHost* host = session_->http_pipelined_host_pool()->
+        GetPipelinedHostIfExists(origin_);
+    if (host) {
+      HttpPipelinedConnection* connection = host->FindAvailablePipeline();
+      if (connection) {
+        next_state_ = STATE_CREATE_STREAM;
+        existing_available_pipeline_ = connection;
+        return OK;
+      }
+    }
+    request_->SetHttpPipeliningKey(origin_);
   }
 
   // OK, there's no available SPDY session. Let |dependent_job_| resume if it's
@@ -763,9 +784,13 @@ int HttpStreamFactoryImpl::Job::DoWaitingUserAction(int result) {
 }
 
 int HttpStreamFactoryImpl::Job::DoCreateStream() {
-  if (!connection_->socket() && !existing_spdy_session_)
+  if (!connection_->socket() && !existing_spdy_session_ &&
+      !existing_available_pipeline_)
     HACKCrashHereToDebug80095();

[willchan no longer on Chromium, 2011/09/26 23:49:20]

Can you just kill this code? It's been debugged, I haven't cleaned it up yet.
Thanks.

[James Simonsen, 2011/09/29 02:39:31]

Done.

 
+  DCHECK(connection_->socket() || existing_spdy_session_ ||
+         existing_available_pipeline_);
+
   next_state_ = STATE_CREATE_STREAM_COMPLETE;
 
   // We only set the socket motivation if we're the first to use
@@ -779,8 +804,22 @@ int HttpStreamFactoryImpl::Job::DoCreateStream() {
   if (!using_spdy_) {
     bool using_proxy = (proxy_info_.is_http() || proxy_info_.is_https()) &&
         request_info_.url.SchemeIs("http");
-    stream_.reset(new HttpBasicStream(connection_.release(), NULL,
-                                      using_proxy));
+    // TODO(simonjam): Support proxies.
+    if (existing_available_pipeline_) {
+      stream_.reset(existing_available_pipeline_->CreateNewStream());
+    } else if (!using_proxy && IsRequestEligibleForPipelining()) {
+      HttpPipelinedConnection* pipeline = session_->http_pipelined_host_pool()->

[willchan no longer on Chromium, 2011/09/26 23:49:20]

It's not clear why the HttpPipelinedConnection* is exposed to
HttpStreamFactoryImpl::Job. It sort of seems like, from the Job*'s perspective,
we should just have an API on the HttpPipelinedHostPool that creates a stream.
The HttpPipelinedConnection and HttpPipelinedHost are implementation details
behind HttpPipelinedHostPool.

[James Simonsen, 2011/09/29 02:39:31]

Done.

[willchan no longer on Chromium, 2011/10/13 02:29:16]

I see. I feel like this is revealing a problem with the design, since I would
think that HttpStreamFactory should be able to hide the pipelining details. It
seems to me that the reason the tight coupling is getting introduced is we have
an issue with how we locate some of our HTTP state. It seems to me that the
HttpPipelinedHostPool should live in HttpStreamFactoryImpl instead of in
HttpNetworkSession. I think the same is true of SpdySessionPool. I should file a
SPDY bug to clean this up. Can you move HttpPipelinedHostPool to be a member of
HttpStreamFactoryImpl?

+          GetOrCreatePipelinedHost(origin_)->CreateNewPipeline(
+              connection_.release(),
+              server_ssl_config_,
+              proxy_info_,
+              net_log_,
+              was_npn_negotiated_);
+      stream_.reset(pipeline->CreateNewStream());
+    } else {
+      stream_.reset(new HttpBasicStream(connection_.release(), NULL,
+                                        using_proxy));
+    }
     return OK;
   }
 
@@ -1086,6 +1125,19 @@ bool HttpStreamFactoryImpl::Job::IsOrphaned() const {
   return !IsPreconnecting() && !request_;
 }
 
+bool HttpStreamFactoryImpl::Job::IsRequestEligibleForPipelining() const {
+  if (!HttpStreamFactory::http_pipelining_enabled()) {
+    return false;
+  }
+  if (IsPreconnecting() || !request_) {
+    return false;
+  }
+  if (using_ssl_) {
+    return false;
+  }
+  return request_info_.method == "GET" || request_info_.method == "HEAD";
+}
+
 #if defined(OS_WIN)
 #pragma warning (disable: 4748)
 #pragma optimize( "", off )