JobController 1: Adding a new class HttpStreamFactoryImpl::JobController

net/http/http_stream_factory_impl_job_controller.h

+// Copyright (c) 2016 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_STREAM_FACTORY_IMPL_JOB_CONTROLLER_H_
+#define NET_HTTP_HTTP_STREAM_FACTORY_IMPL_JOB_CONTROLLER_H_
+
+#include "net/http/http_stream_factory_impl.h"
+
+namespace net {
+
+// HttpStreamFactoryImpl::JobController manages Request and Jobs.
+class HttpStreamFactoryImpl::JobController {
+ public:
+  JobController(HttpStreamFactoryImpl* factory);
+  ~JobController();
+
+  Request* CreatRequest(const HttpRequestInfo& request_info,

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

Would you be willing to add some documenting comments to this interface?  What
is the expected use cases?  Is CreateRequest ever called more than once?  

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

Comments added. CreateRequest(renamed as Start()) should only be called when we
RequestStream().

+                        HttpStreamRequest::Delegate* delegate,
+                        WebSocketHandshakeStreamBase::CreateHelper*
+                            websocket_handshake_stream_create_helper,
+                        const BoundNetLog& net_log,
+                        HttpStreamRequest::StreamType stream_type);
+
+  void Start(HttpNetworkSession* session,
+             const HttpRequestInfo& request_info,
+             RequestPriority priority,
+             const SSLConfig& server_ssl_config,
+             const SSLConfig& proxy_ssl_config,
+             HttpStreamRequest::Delegate* delegate,
+             HttpStreamRequest::StreamType stream_type,
+             const BoundNetLog& net_log);
+
+  void Preconnect(int num_streams,
+                  HttpNetworkSession* session,
+                  const HttpRequestInfo& request_info,
+                  const SSLConfig& server_ssl_config,
+                  const SSLConfig& proxy_ssl_config);
+
+  LoadState GetLoadState() const;
+
+  void SetPriority(RequestPriority priority);
+
+  int RestartTunnelWithProxyAuth(const AuthCredentials& credentials);
+
+  // Used to bind |job| to the |request_| and orphan all other jobs in |jobs_|.
+  void BindJob(Job* job);
+
+  // Used to orphan all jobs in |jobs_|.

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

nit, suggestion: When you talk about "orphaning" jobs in these comments, could
you define that?  Given that it used to mean "disassociate the job from the
request so that it will outlive the request" and we don't associate jobs with
requests anymore, I'm curious what orphaning means now.  Is it as simple as
"protect the job from being killed if the request is cancelled, and don't tell
the request about job completion"?

+  void OrphanJobs();
+
+  // Called when a Job succeeds.

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

I think this is an inaccurate comment; at least, it looks to me like this is
called on Request destruction, which can happen for reasons other than a Job
succeeding.  

I'd also like to advocate for documenting routines by what they do, from the
perspective of the consumer (i.e. after the implementation has been abstracted
away).  So I think this is "Cancel all non-orphaned jobs."?  (This point isn't
as important as it usually is because these classes are pretty tightly
integrated, but I think it's still goodness.)

+  void CancelJobs();
+
+  // Called when a Job succeeds.
+  void OnJobSucceeded(Job* job);

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

Why is this public?  I think it's only called from the controller.

(In general, I think it's worth trying for the minimal interface we need, which
means anything not called from outside from the controller should be private,
unless there's some strong API reason for exposing it.)

+
+  void OnRequestFinish();
+
+  // Marks completion of the |request_|. Must be called before OnStreamReady().

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

Suggestion: I wonder if it's worthwhile separating out the different pieces of
the JobController interface by what consumer they're targeted at (Request, Job,
HttpStreamFactoryImpl).  The a-r way to do that would be to have defined
"Delegate" interfaces for the Request and Job that JobController inherited from,
which I'm not sure is worth it given the tight coupling between the classes. 
But maybe just group them in this file that way, with comments?

+  void MarkRequestComplete(bool was_npn_negotiated,
+                           NextProto protocol_negotiated,
+                           bool using_spdy);
+
+  void OnStreamReady(Job* job,
+                     const SSLConfig& used_ssl_config,
+                     const ProxyInfo& used_proxy_info,
+                     HttpStream* stream);
+
+  void OnBidirectionalStreamImplReady(Job* job,
+                                      const SSLConfig& used_ssl_config,
+                                      const ProxyInfo& used_proxy_info,
+                                      BidirectionalStreamImpl* stream);
+
+  void OnWebSocketHandshakeStreamReady(Job* job,
+                                       const SSLConfig& used_ssl_config,
+                                       const ProxyInfo& used_proxy_info,
+                                       WebSocketHandshakeStreamBase* stream);
+
+  void OnStreamFailed(Job* job,
+                      int status,
+                      const SSLConfig& used_ssl_config,
+                      SSLFailureState ssl_failure_state);
+
+  void OnCertificateError(Job* job,
+                          int status,
+                          const SSLConfig& used_ssl_config,
+                          const SSLInfo& ssl_info);
+
+  void OnNeedsProxyAuth(Job* job,
+                        const HttpResponseInfo& proxy_response,
+                        const SSLConfig& used_ssl_config,
+                        const ProxyInfo& used_proxy_info,
+                        HttpAuthController* auth_controller);
+
+  void OnNeedsClientAuth(Job* job,
+                         const SSLConfig& used_ssl_config,
+                         SSLCertRequestInfo* cert_info);
+
+  void OnHttpsProxyTunnelResponse(Job* job,
+                                  const HttpResponseInfo& response_info,
+                                  const SSLConfig& used_ssl_config,
+                                  const ProxyInfo& used_proxy_info,
+                                  HttpStream* stream);
+
+  // Notify the |request_| and |factoy_| of the readiness of new SPDY session.

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

nit: factory_

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

Done.

+  void OnNewSpdySessionReady(
+      Job* job,
+      std::unique_ptr<HttpStream> stream,
+      std::unique_ptr<BidirectionalStreamImpl> bidirectional_stream_impl,
+      const base::WeakPtr<SpdySession>& spdy_session,
+      bool direct);
+
+  // Invoked when the Job finishes pre-connecting sockets.
+  void OnPreconnectsComplete(Job* job);
+
+  // Invoked when an orphaned Job finishes.
+  void OnOrphanedJobComplete(const Job* job);
+
+  // Called by an attached Job to record connection attempts made by the socket
+  // layer to |request_|.
+  void AddConnectionAttemptsToRequest(const ConnectionAttempts& attempts);
+
+  // Called when Job determines the appropriate |spdy_session_key| for the
+  // Request. Note that this does not mean that SPDY is necessarily supported
+  // for this SpdySessionKey, since we may need to wait for NPN to complete
+  // before knowing if SPDY is available.
+  void SetSpdySessionKey(const SpdySessionKey& spdy_session_key);
+
+  // Remove session from the SpdySessionRequestMap.
+  void RemoveRequestFromSpdySessionRequestMap();
+
+  void MaybeNotifyFactoryOfCompletion();

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

As above, why not private?  (You should do your own pass; I just happened to
notice this one.)

+
+  const BoundNetLog& GetNetLogFromRequest() const;
+
+  SpdySessionRequestMap& GetSpdySessionRequestMap() {
+    return factory_->spdy_session_request_map_;
+  }
+
+  bool for_websockets() { return factory_->for_websockets_; }
+
+  WebSocketHandshakeStreamBase::CreateHelper*
+  websocket_handshake_stream_create_helper();
+
+ private:
+  FRIEND_TEST_ALL_PREFIXES(HttpStreamFactoryImplRequestTest, SetPriority);
+  FRIEND_TEST_ALL_PREFIXES(HttpStreamFactoryImplRequestTest, DelayMainJob);
+
+  // Creates Job(s) for |request_|. Jobs will be owned by |this|.
+  void CreateJobs();
+
+  // Attaches |job| to |request_|. Does not mean that |request_| will use |job|.
+  void AttachJob(Job* job);
+
+  HttpStreamFactoryImpl* factory_;
+
+  // Request will be handed out to factory once created. This just keeps an
+  // reference. It will be set to NULL once the request is completed.
+  Request* request_;

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

This naively strikes me as fairly dangerous because it might lead to a
use-after-free error. I'd like there to be some documentation about why that
doesn't happen (e.g. safe because the request notifies the JobController when
it's cancelled/destroyed through X, which nulls this variable.)

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

Done.

+
+  std::set<Job*> jobs_;

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

These jobs are owned by this, right? If so, perhaps set<std::unique_ptr<>> ?

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

Acknowledged.

+
+  // |main_job_| is a job waiting to see if |alternative_job_| can reuse a
+  // connection. If |alternative_job_| is unable to do so, |this| will notify
+  // |main_job_| to proceed and then race the two jobs.
+  Job* main_job_;
+  Job* alternative_job_;
+
+  // At the point where a Job is irrevocably tied to |request_|, we set this.
+  std::unique_ptr<Job> bound_job_;
+
+  // These jobs correspond to jobs orphaned by |request_|. Since they are no
+  // longer tied to Requests, they will not be canceled when |request_| is
+  // canceled. Therefore, in ~JobController, it is possible for some jobs to
+  // still exist in this set. Leftover jobs will be deleted when the
+  // JobController is destroyed.
+  std::set<const Job*> orphaned_job_set_;

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

Just an observation, there are quite a few different members which are pointers
to the same two jobs:

jobs_
 main_job_
 alternative_job_
bound_job_
orphaned_job_set

It might be nice to find a way to clean this up.

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

Done.

+};
+
+}  // namespace net
+
+#endif  // NET_HTTP_HTTP_STREAM_FACTORY_IMPL_JOB_CONTROLLER_H_