Throttle HttpStreamFactoryImpl::Job to HTTP/2-supported servers

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 c97829f54020485fc1c5c6008383ab3a1e2759d9..edc6649925e193a35b9c92bce72536e03dbf03d2 100644
--- a/net/http/http_stream_factory_impl_job.cc
+++ b/net/http/http_stream_factory_impl_job.cc
@@ -224,6 +224,7 @@ HttpStreamFactoryImpl::Job::Job(Delegate* delegate,
           !(proxy_info.is_https() && origin_url_.SchemeIs(url::kHttpScheme))),
       spdy_session_key_(GetSpdySessionKey()),
       stream_type_(HttpStreamRequest::BIDIRECTIONAL_STREAM),
+      resumed_init_connection_(false),
       ptr_factory_(this) {
   DCHECK(session);
   // The job can't have alternative service and alternative proxy server set at
@@ -555,6 +556,7 @@ void HttpStreamFactoryImpl::Job::RunLoop(int result) {
   if (result == ERR_IO_PENDING)
     return;
 
+  session_->spdy_session_pool()->ResumeAllPendingRequests(spdy_session_key_);
   if (job_type_ == PRECONNECT) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE,
@@ -686,6 +688,10 @@ int HttpStreamFactoryImpl::Job::DoLoop(int result) {
       case STATE_WAIT_COMPLETE:
         rv = DoWaitComplete(rv);
         break;
+      case STATE_BEFORE_INIT_CONNECTION:
+        DCHECK_EQ(OK, rv);
+        rv = DoBeforeInitConnection();
+        break;
       case STATE_INIT_CONNECTION:
         DCHECK_EQ(OK, rv);
         rv = DoInitConnection();
@@ -772,10 +778,43 @@ int HttpStreamFactoryImpl::Job::DoWait() {
 int HttpStreamFactoryImpl::Job::DoWaitComplete(int result) {
   net_log_.EndEvent(NetLogEventType::HTTP_STREAM_JOB_WAITING);
   DCHECK_EQ(OK, result);
+  next_state_ = STATE_BEFORE_INIT_CONNECTION;
+  return OK;
+}
+
+int HttpStreamFactoryImpl::Job::DoBeforeInitConnection() {
   next_state_ = STATE_INIT_CONNECTION;
+  // Throttle connect to an HTTP/2 supported server, if there are pending
+  // requests with the same SpdySessionKey.
+  if (!session_->http_server_properties()->RequiresHTTP11(

[Bence, 2017/06/09 19:17:38]

Please invert this condition and do an early return.

[xunjieli, 2017/06/13 15:23:10]

Done.

+          spdy_session_key_.host_port_pair())) {
+    url::SchemeHostPort scheme_host_port(
+        using_ssl_ ? url::kHttpsScheme : url::kHttpScheme,
+        spdy_session_key_.host_port_pair().host(),
+        spdy_session_key_.host_port_pair().port());
+    if (session_->http_server_properties()->GetSupportsSpdy(scheme_host_port)) {

[Bence, 2017/06/09 19:17:38]

Please invert this condition and do an early return.

[xunjieli, 2017/06/13 15:23:10]

Done.

+      base::Closure callback =
+          base::Bind(&HttpStreamFactoryImpl::Job::ResumeInitConnection,
+                     ptr_factory_.GetWeakPtr());
+      if (!session_->spdy_session_pool()->StartRequest(spdy_session_key_,
+                                                       callback)) {
+        base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+            FROM_HERE, callback, base::TimeDelta::FromMilliseconds(300));

[Bence, 2017/06/09 19:17:38]

Please define a file-scope const int at the top of this file instead of
hardcoding 300 here.

[xunjieli, 2017/06/13 15:23:10]

Done.

+        return ERR_IO_PENDING;
+      }
+    }
+  }
   return OK;
 }
 
+void HttpStreamFactoryImpl::Job::ResumeInitConnection() {
+  if (resumed_init_connection_)

[Bence, 2017/06/09 19:17:38]

Under what circumstances can this be called multiple times?  In other words, why
is this member necessary at all?

[xunjieli, 2017/06/13 15:23:10]

Assuming there are 2 Jobs (A and B) issued at the same time to the same
H2-supported server. Job B is throttled because A went first, so B posts a
delayed task to resume itself. When the Job A completes, Job A posts a task to
resume B (through SpdySessionPool).
The boolean makes sure that when the delayed task run, it doesn't make the Job B
run DoLoop() again. 

One the other hand, if B already has resumed itself (and is happily doing
connection establishment), when A completes, SpdySession doesn't try to make B
do DoLoop() because B might be in the middle of DoInitConnection() or is in
STATE_DONE.

[Bence, 2017/06/13 16:04:51]

Thanks for the explanation.

+    return;
+  CHECK_EQ(next_state_, STATE_INIT_CONNECTION);
+  resumed_init_connection_ = true;
+  OnIOComplete(OK);
+}
+
 int HttpStreamFactoryImpl::Job::DoInitConnection() {
   net_log_.BeginEvent(NetLogEventType::HTTP_STREAM_JOB_INIT_CONNECTION);
   int result = DoInitConnectionImpl();