JobController 1: Adding a new class HttpStreamFactoryImpl::JobController

net/http/http_stream_factory_impl_request.cc

Index: net/http/http_stream_factory_impl_request.cc
diff --git a/net/http/http_stream_factory_impl_request.cc b/net/http/http_stream_factory_impl_request.cc
index 923e9e0ebe7f22b1b13d94aa022bcc9509eae1ce..f194651e1f05b37a84ed0b691cce148b5e3c9a80 100644
--- a/net/http/http_stream_factory_impl_request.cc
+++ b/net/http/http_stream_factory_impl_request.cc
@@ -9,6 +9,7 @@
 #include "base/stl_util.h"
 #include "net/http/bidirectional_stream_impl.h"
 #include "net/http/http_stream_factory_impl_job.h"
+#include "net/http/http_stream_factory_impl_job_controller.h"
 #include "net/spdy/spdy_http_stream.h"
 #include "net/spdy/spdy_session.h"
 
@@ -16,14 +17,14 @@ namespace net {
 
 HttpStreamFactoryImpl::Request::Request(
     const GURL& url,
-    HttpStreamFactoryImpl* factory,
+    JobController* job_controller,
     HttpStreamRequest::Delegate* delegate,
     WebSocketHandshakeStreamBase::CreateHelper*
         websocket_handshake_stream_create_helper,
     const BoundNetLog& net_log,
     StreamType stream_type)
     : url_(url),
-      factory_(factory),
+      job_controller_(job_controller),
       websocket_handshake_stream_create_helper_(
           websocket_handshake_stream_create_helper),
       delegate_(delegate),
@@ -33,19 +34,16 @@ HttpStreamFactoryImpl::Request::Request(
       protocol_negotiated_(kProtoUnknown),
       using_spdy_(false),
       stream_type_(stream_type) {
-  DCHECK(factory_);
   DCHECK(delegate_);
 
   net_log_.BeginEvent(NetLog::TYPE_HTTP_STREAM_REQUEST);
 }
 
 HttpStreamFactoryImpl::Request::~Request() {
-  if (bound_job_.get())
-    DCHECK(jobs_.empty());
-
   net_log_.EndEvent(NetLog::TYPE_HTTP_STREAM_REQUEST);
 
-  CancelJobs();
+  job_controller_->CancelJobs();
+  job_controller_->OnRequestFinish();

[Ryan Hamilton, 2016/05/06 20:49:02]

nit: I would unify these into a single method on the controller.

[Zhongyi Shi, 2016/05/12 07:26:24]

Done.

 }
 
 void HttpStreamFactoryImpl::Request::SetSpdySessionKey(
@@ -53,17 +51,11 @@ void HttpStreamFactoryImpl::Request::SetSpdySessionKey(
   CHECK(!spdy_session_key_.get());
   spdy_session_key_.reset(new SpdySessionKey(spdy_session_key));
   RequestSet& request_set =
-      factory_->spdy_session_request_map_[spdy_session_key];
+      job_controller_->GetSpdySessionRequestMap()[spdy_session_key];
   DCHECK(!ContainsKey(request_set, this));
   request_set.insert(this);

[Ryan Hamilton, 2016/05/06 20:49:02]

Can you add a method on the controller which is something like:

AddSpdySessionRequest(const SpdySessionKey& spdy_session_key,
HttpStreamFactoryImpl::Request* request);

and just call that here:

AddSpdySessionRequest(spdy_session_key, this);

Then move the logic of manipulating the set into the controller.

GAH! Scratch that. It looks like this method (SetSpdySessionKey) is called by
the controller, so the controller can do this logic when it calls this method.

[Zhongyi Shi, 2016/05/12 07:26:24]

Done.

 }
 
-void HttpStreamFactoryImpl::Request::AttachJob(Job* job) {
-  DCHECK(job);
-  jobs_.insert(job);
-  factory_->request_map_[job] = this;
-}
-
 void HttpStreamFactoryImpl::Request::Complete(bool was_npn_negotiated,
                                               NextProto protocol_negotiated,
                                               bool using_spdy) {
@@ -75,150 +67,80 @@ void HttpStreamFactoryImpl::Request::Complete(bool was_npn_negotiated,
 }
 
 void HttpStreamFactoryImpl::Request::OnStreamReady(
-    Job* job,
     const SSLConfig& used_ssl_config,
     const ProxyInfo& used_proxy_info,
     HttpStream* stream) {
-  DCHECK(!factory_->for_websockets_);
-  DCHECK_EQ(HttpStreamRequest::HTTP_STREAM, stream_type_);
-  DCHECK(stream);

[Ryan Hamilton, 2016/05/06 20:49:02]

Did you mean to remove these changes? I assume so, but it's not obvious why.

[Zhongyi Shi, 2016/05/12 07:26:24]

This is intentional. Request::OnStreamReady is now called by
JobController::OnStreamReady(), in which those checks will be performed. 

   DCHECK(completed_);
-
-  OnJobSucceeded(job);
   delegate_->OnStreamReady(used_ssl_config, used_proxy_info, stream);
 }
 
 void HttpStreamFactoryImpl::Request::OnBidirectionalStreamImplReady(
-    Job* job,
     const SSLConfig& used_ssl_config,
     const ProxyInfo& used_proxy_info,
     BidirectionalStreamImpl* stream_job) {
-  DCHECK(!factory_->for_websockets_);
-  DCHECK_EQ(HttpStreamRequest::BIDIRECTIONAL_STREAM, stream_type_);
-  DCHECK(stream_job);
   DCHECK(completed_);
-
-  OnJobSucceeded(job);
   delegate_->OnBidirectionalStreamImplReady(used_ssl_config, used_proxy_info,
                                             stream_job);
 }
 
 void HttpStreamFactoryImpl::Request::OnWebSocketHandshakeStreamReady(
-    Job* job,
     const SSLConfig& used_ssl_config,
     const ProxyInfo& used_proxy_info,
     WebSocketHandshakeStreamBase* stream) {
-  DCHECK(factory_->for_websockets_);
-  DCHECK_EQ(HttpStreamRequest::HTTP_STREAM, stream_type_);
-  DCHECK(stream);
   DCHECK(completed_);
-
-  OnJobSucceeded(job);
   delegate_->OnWebSocketHandshakeStreamReady(
       used_ssl_config, used_proxy_info, stream);
 }
 
 void HttpStreamFactoryImpl::Request::OnStreamFailed(
-    Job* job,
     int status,
     const SSLConfig& used_ssl_config,
     SSLFailureState ssl_failure_state) {
-  DCHECK_NE(OK, status);
-  DCHECK(job);
-  if (!bound_job_.get()) {
-    if (jobs_.size() > 1) {
-      // Hey, we've got other jobs! Maybe one of them will succeed, let's just
-      // ignore this failure.
-      jobs_.erase(job);
-      factory_->request_map_.erase(job);
-      // Notify all the other jobs that this one failed.
-      for (std::set<Job*>::iterator it = jobs_.begin(); it != jobs_.end(); ++it)
-        (*it)->MarkOtherJobComplete(*job);
-      delete job;
-      return;
-    } else {
-      BindJob(job);
-    }
-  } else {
-    DCHECK(jobs_.empty());
-  }
   delegate_->OnStreamFailed(status, used_ssl_config, ssl_failure_state);
 }
 
 void HttpStreamFactoryImpl::Request::OnCertificateError(
-    Job* job,
     int status,
     const SSLConfig& used_ssl_config,
     const SSLInfo& ssl_info) {
-  DCHECK_NE(OK, status);

[Ryan Hamilton, 2016/05/06 20:49:02]

Did you mean to remove this?

[Zhongyi Shi, 2016/05/12 07:26:24]

Yup. The Job binding work is done in Controller. Thus in
JobController::OnCertificateError, we will check to see whether BindJob should
be called.

-  if (!bound_job_.get())
-    BindJob(job);
-  else
-    DCHECK(jobs_.empty());
   delegate_->OnCertificateError(status, used_ssl_config, ssl_info);
 }
 
 void HttpStreamFactoryImpl::Request::OnNeedsProxyAuth(
-    Job* job,
     const HttpResponseInfo& proxy_response,
     const SSLConfig& used_ssl_config,
     const ProxyInfo& used_proxy_info,
     HttpAuthController* auth_controller) {
-  if (!bound_job_.get())
-    BindJob(job);
-  else
-    DCHECK(jobs_.empty());
   delegate_->OnNeedsProxyAuth(
       proxy_response, used_ssl_config, used_proxy_info, auth_controller);
 }
 
 void HttpStreamFactoryImpl::Request::OnNeedsClientAuth(
-    Job* job,
     const SSLConfig& used_ssl_config,
     SSLCertRequestInfo* cert_info) {
-  if (!bound_job_.get())
-    BindJob(job);
-  else
-    DCHECK(jobs_.empty());
   delegate_->OnNeedsClientAuth(used_ssl_config, cert_info);
 }
 
 void HttpStreamFactoryImpl::Request::OnHttpsProxyTunnelResponse(
-    Job *job,
     const HttpResponseInfo& response_info,
     const SSLConfig& used_ssl_config,
     const ProxyInfo& used_proxy_info,
     HttpStream* stream) {
-  if (!bound_job_.get())
-    BindJob(job);
-  else
-    DCHECK(jobs_.empty());
   delegate_->OnHttpsProxyTunnelResponse(
       response_info, used_ssl_config, used_proxy_info, stream);
 }
 
 int HttpStreamFactoryImpl::Request::RestartTunnelWithProxyAuth(
     const AuthCredentials& credentials) {
-  DCHECK(bound_job_.get());
-  return bound_job_->RestartTunnelWithProxyAuth(credentials);
+  return job_controller_->RestartTunnelWithProxyAuth(credentials);
 }
 
 void HttpStreamFactoryImpl::Request::SetPriority(RequestPriority priority) {
-  for (std::set<HttpStreamFactoryImpl::Job*>::const_iterator it = jobs_.begin();
-       it != jobs_.end(); ++it) {
-    (*it)->SetPriority(priority);
-  }
-  if (bound_job_)
-    bound_job_->SetPriority(priority);
+  job_controller_->SetPriority(priority);
 }
 
 LoadState HttpStreamFactoryImpl::Request::GetLoadState() const {
-  if (bound_job_.get())
-    return bound_job_->GetLoadState();
-  DCHECK(!jobs_.empty());
-
-  // Just pick the first one.
-  return (*jobs_.begin())->GetLoadState();
+  return job_controller_->GetLoadState();
 }

[Ryan Hamilton, 2016/05/06 20:49:02]

THIS IS SO MUCH CLEANER!

 
 bool HttpStreamFactoryImpl::Request::was_npn_negotiated() const {
@@ -246,7 +168,7 @@ void
 HttpStreamFactoryImpl::Request::RemoveRequestFromSpdySessionRequestMap() {
   if (spdy_session_key_.get()) {
     SpdySessionRequestMap& spdy_session_request_map =
-        factory_->spdy_session_request_map_;
+        job_controller_->GetSpdySessionRequestMap();
     DCHECK(ContainsKey(spdy_session_request_map, *spdy_session_key_));
     RequestSet& request_set =
         spdy_session_request_map[*spdy_session_key_];
@@ -270,40 +192,20 @@ bool HttpStreamFactoryImpl::Request::HasSpdySessionKey() const {
 // path for notifying any requests waiting for the session (including the
 // request which spawned it).
 void HttpStreamFactoryImpl::Request::OnNewSpdySessionReady(
-    Job* job,
     std::unique_ptr<HttpStream> stream,
     std::unique_ptr<BidirectionalStreamImpl> bidirectional_stream_impl,
     const base::WeakPtr<SpdySession>& spdy_session,
+    const SSLConfig& used_ssl_config,
+    const ProxyInfo& used_proxy_info,
+    bool was_npn_negotiated,
+    const NextProto& protocol_negotiated,
+    bool using_spdy,
+    const BoundNetLog& net_log,
     bool direct) {
-  DCHECK(job);
-  DCHECK(job->using_spdy());
-
   // Note: |spdy_session| may be NULL. In that case, |delegate_| should still
   // receive |stream| so the error propagates up correctly, however there is no
   // point in broadcasting |spdy_session| to other requests.
-
-  // The first case is the usual case.
-  if (!bound_job_.get()) {
-    BindJob(job);
-  } else {  // This is the case for HTTPS proxy tunneling.
-    DCHECK_EQ(bound_job_.get(), job);
-    DCHECK(jobs_.empty());
-  }
-
-  // Cache these values in case the job gets deleted.
-  const SSLConfig used_ssl_config = job->server_ssl_config();
-  const ProxyInfo used_proxy_info = job->proxy_info();
-  const bool was_npn_negotiated = job->was_npn_negotiated();
-  const NextProto protocol_negotiated =
-      job->protocol_negotiated();
-  const bool using_spdy = job->using_spdy();
-  const BoundNetLog net_log = job->net_log();
-
-  Complete(was_npn_negotiated, protocol_negotiated, using_spdy);
-
-  // Cache this so we can still use it if the request is deleted.
-  HttpStreamFactoryImpl* factory = factory_;
-  if (factory->for_websockets_) {
+  if (job_controller_->for_websockets()) {

[Randy Smith (Not in Mondays), 2016/05/09 21:42:10]

Can we hoist this conditional into JobController and just use
Request::On{Bidirectional,}Stream{Impl,}Ready?

[Zhongyi Shi, 2016/05/12 07:26:24]

This method is now removed!!

     // TODO(ricea): Re-instate this code when WebSockets over SPDY is
     // implemented.
     NOTREACHED();
@@ -311,25 +213,13 @@ void HttpStreamFactoryImpl::Request::OnNewSpdySessionReady(
     DCHECK(bidirectional_stream_impl);
     DCHECK(!stream);
     delegate_->OnBidirectionalStreamImplReady(
-        job->server_ssl_config(), job->proxy_info(),
-        bidirectional_stream_impl.release());
+        used_ssl_config, used_proxy_info, bidirectional_stream_impl.release());
   } else {
     DCHECK(!bidirectional_stream_impl);
     DCHECK(stream);
-    delegate_->OnStreamReady(job->server_ssl_config(), job->proxy_info(),
+    delegate_->OnStreamReady(used_ssl_config, used_proxy_info,
                              stream.release());
   }
-  // |this| may be deleted after this point.
-  if (spdy_session && spdy_session->IsAvailable()) {
-    factory->OnNewSpdySessionReady(spdy_session,
-                                   direct,
-                                   used_ssl_config,
-                                   used_proxy_info,
-                                   was_npn_negotiated,
-                                   protocol_negotiated,
-                                   using_spdy,
-                                   net_log);
-  }
 }
 
 void HttpStreamFactoryImpl::Request::AddConnectionAttempts(
@@ -338,76 +228,4 @@ void HttpStreamFactoryImpl::Request::AddConnectionAttempts(
     connection_attempts_.push_back(attempt);
 }
 
-void HttpStreamFactoryImpl::Request::BindJob(Job* job) {
-  DCHECK(job);
-  DCHECK(!bound_job_.get());
-  DCHECK(ContainsKey(jobs_, job));
-  bound_job_.reset(job);
-  jobs_.erase(job);
-  factory_->request_map_.erase(job);
-
-  net_log_.AddEvent(NetLog::TYPE_HTTP_STREAM_REQUEST_BOUND_TO_JOB,
-                    job->net_log().source().ToEventParametersCallback());
-  job->net_log().AddEvent(NetLog::TYPE_HTTP_STREAM_JOB_BOUND_TO_REQUEST,
-                          net_log_.source().ToEventParametersCallback());
-
-  OrphanJobs();
-}
-
-void HttpStreamFactoryImpl::Request::OrphanJobs() {
-  RemoveRequestFromSpdySessionRequestMap();
-
-  std::set<Job*> tmp;
-  tmp.swap(jobs_);
-
-  for (Job* job : tmp)
-    factory_->OrphanJob(job, this);
-}
-
-void HttpStreamFactoryImpl::Request::CancelJobs() {
-  RemoveRequestFromSpdySessionRequestMap();
-
-  std::set<Job*> tmp;
-  tmp.swap(jobs_);
-
-  for (Job* job : tmp) {
-    factory_->request_map_.erase(job);
-    delete job;
-  }
-}
-
-void HttpStreamFactoryImpl::Request::OnJobSucceeded(Job* job) {
-  // |job| should only be NULL if we're being serviced by a late bound
-  // SpdySession (one that was not created by a job in our |jobs_| set).
-  if (!job) {
-    DCHECK(!bound_job_.get());
-    DCHECK(!jobs_.empty());
-    // NOTE(willchan): We do *NOT* call OrphanJobs() here. The reason is because
-    // we *WANT* to cancel the unnecessary Jobs from other requests if another
-    // Job completes first.
-    // TODO(mbelshe): Revisit this when we implement ip connection pooling of
-    // SpdySessions. Do we want to orphan the jobs for a different hostname so
-    // they complete? Or do we want to prevent connecting a new SpdySession if
-    // we've already got one available for a different hostname where the ip
-    // address matches up?
-    CancelJobs();
-    return;
-  }
-  if (!bound_job_.get()) {
-    if (jobs_.size() > 1)
-      job->ReportJobSucceededForRequest();
-    // Notify all the other jobs that this one succeeded.
-    for (std::set<Job*>::iterator it = jobs_.begin(); it != jobs_.end(); ++it) {
-      if (*it != job) {
-        (*it)->MarkOtherJobComplete(*job);
-      }
-    }
-    // We may have other jobs in |jobs_|. For example, if we start multiple jobs
-    // for Alternate-Protocol.
-    BindJob(job);
-    return;
-  }
-  DCHECK(jobs_.empty());
-}
-
 }  // namespace net