JobController 2: Remove reference between HttpStreamFactoryImpl::Jobs.

net/http/http_stream_factory_impl_job.h

 // Copyright (c) 2012 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_H_
 #define NET_HTTP_HTTP_STREAM_FACTORY_IMPL_JOB_H_
 
 #include <memory>
 
 #include "base/macros.h"
@@ skipping 98 matching lines @@
 
     // Invoked when the |job| finishes pre-connecting sockets.
     virtual void OnPreconnectsComplete(Job* job) = 0;
 
     // Invoked to record connection attempts made by the socket layer to
     // Request if |job| is associated with Request.
     virtual void AddConnectionAttemptsToRequest(
         Job* job,
         const ConnectionAttempts& attempts) = 0;
 
+    // Invoked to resume the main job when |job| initiates a connection but
+    // does not succeed.
+    virtual void OnInitConnectionNotSuccessful(

[Ryan Hamilton, 2016/06/29 23:12:28]

Discussed offline. How about:

virtual void OnConnectionInitialized(
   Job* job,
   int rv,
   const base::TimeDelta& delay) = 0;

Does this job know what |delay| should be set to, or can the controller figure
that out?

[Zhongyi Shi, 2016/06/30 22:53:39]

Discussed offline. The controller will need to know the wait_time_ for the main
job if it is a delayed TCP case. The wait time will be set by the alternative
job so that when this method is invoked, controller already knows the delay
time. 

+        Job* job,
+        const base::TimeDelta& delay) = 0;
+
+    // Return false if |job| has no blocking job and there's no wait time,
+    // i.e., |job| won't call Job::ResumeAfterDelay(). Else return true and
+    // resume |job| with appropriate wait time.

[Ryan Hamilton, 2016/06/29 23:12:28]

How about something like:

// Returns false if |job| can advance to the next state. Otherwise, |job| will
wait for <SomeMethod> to be called before advancing.

[Zhongyi Shi, 2016/06/30 22:53:39]

Done.

+    virtual bool ShoudWait(Job* job) const = 0;
+
     // 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.
     virtual void SetSpdySessionKey(Job* job,
                                    const SpdySessionKey& spdy_session_key) = 0;
 
     // Remove session from the SpdySessionRequestMap.
     virtual void RemoveRequestFromSpdySessionRequestMapForJob(Job* job) = 0;
 
     virtual const BoundNetLog* GetNetLog(Job* job) const = 0;
 
     virtual WebSocketHandshakeStreamBase::CreateHelper*
     websocket_handshake_stream_create_helper() = 0;
 
+    virtual bool blocking() = 0;
+    virtual const base::TimeDelta& wait_time_for_main_job() const = 0;
+    virtual void set_wait_time_for_main_job(const base::TimeDelta& delay) = 0;

[Ryan Hamilton, 2016/06/29 23:12:28]

It's not obvious to me why these methods are needed on the job, so I'll keep
reading.

[Zhongyi Shi, 2016/06/30 22:53:39]

The set method is mainly used for delayed TCP case to tell the controller cache
the delay for main job. Also used in the test case in request_unittest.

The get method is used in Job::Resume, where the job may want to query the
controller for waiting time to ResumeAfterDelay.

+
     virtual bool for_websockets() = 0;
   };
 
   // Constructor for non-alternative Job.
   // Job is owned by |delegate|, hence |delegate| is valid for the
   // lifetime of the Job.
   Job(Delegate* delegate,
       JobType job_type,
       HttpNetworkSession* session,
       const HttpRequestInfo& request_info,
@@ skipping 24 matching lines @@
   // |delegate_| will be notified upon completion.
   virtual void Start(HttpStreamRequest::StreamType stream_type);
 
   // Preconnect will attempt to request |num_streams| sockets from the
   // appropriate ClientSocketPool.
   int Preconnect(int num_streams);
 
   int RestartTunnelWithProxyAuth(const AuthCredentials& credentials);
   LoadState GetLoadState() const;
 
-  // Tells |this| to wait for |job| to resume it.
-  void WaitFor(Job* job);
+  // Tells |this| that |delegate_| has determined it still needs to continue
+  // connecting, so allow |this| to continue after the specified |delay|. If
+  // this is not called, then Request is expected to cancel |this| by
+  // deleting it.
+  virtual void Resume(const base::TimeDelta& delay);
 
-  // Tells |this| that |job| has determined it still needs to continue
-  // connecting, so allow |this| to continue after the specified |delay|. If
-  // this is not called, then |request_| is expected to cancel |this| by
-  // deleting it.
-  void Resume(Job* job, const base::TimeDelta& delay);
+  // Resume |this| job after the specified |delay|.
+  void ResumeAfterDelay(const base::TimeDelta& delay);

[Ryan Hamilton, 2016/06/29 23:12:28]

What is the difference between these two methods?

[Zhongyi Shi, 2016/06/30 22:53:39]

In the old world, Resume calles ResumeAfterDelay if the job's next_state_ is
STATE_DO_WAIT_COMPLETE. 

However, ResumeAfterDelay is also called in DoWait where we want to tell the
MAIN job to wait. At that time, the next_state_ is not set to
STATE_DO_WAIT_COMPLETE yet. I have changed to the code in DoWait() which will
always set the next_state_ to STATE_DO_WAIT_COMPLETE, then query controller
whether it should wait, and call *Resume* if needed, then it will set the
next_state_ according to the result from ShouldWait().

So no ResumeAfterDealy any more. All in Resume! 

 
   // Called to detach |this| Job. May resume the other Job, will disconnect
   // the socket for |this| Job, and notify |delegate| upon completion.
   void Orphan();
 
   void SetPriority(RequestPriority priority);
 
   RequestPriority priority() const { return priority_; }
   bool was_npn_negotiated() const;
   NextProto protocol_negotiated() const;
@@ skipping 18 matching lines @@
   void ReportJobSucceededForRequest();
 
   // Marks that the other |job| has completed.
   virtual void MarkOtherJobComplete(const Job& job);
 
   JobType job_type() const { return job_type_; }
 
  private:
   FRIEND_TEST_ALL_PREFIXES(HttpStreamFactoryImplRequestTest, DelayMainJob);
 
+  friend class HttpStreamFactoryImplJobPeer;
+
   enum State {
     STATE_START,
     STATE_RESOLVE_PROXY,
     STATE_RESOLVE_PROXY_COMPLETE,
 
     // Note that when Alternate-Protocol says we can connect to an alternate
     // port using a different protocol, we have the choice of communicating over
     // the original protocol, or speaking the alternate protocol (currently,
     // only npn-spdy) over an alternate port. For a cold page load, the http
     // connection that delivers the http response that has the
     // Alternate-Protocol header will already be warm. So, blocking the next
     // http request on establishing a new npn-spdy connection would incur extra
     // latency. Even if the http connection was not reused, establishing a new
     // http connection is typically faster than npn-spdy, since npn-spdy
     // requires a SSL handshake. Therefore, we start both the http and the
     // npn-spdy jobs in parallel. In order not to unnecessarily waste sockets,
     // we have the http job block on the npn-spdy job after proxy resolution.
     // The npn-spdy job will Resume() the http job if, in
     // STATE_INIT_CONNECTION_COMPLETE, it detects an error or does not find an
     // existing SpdySession. In that case, the http and npn-spdy jobs will race.
     // When QUIC protocol is used by the npn-spdy job, then http job will wait
     // for |wait_time_| when the http job was resumed.
-    STATE_WAIT_FOR_JOB,
-    STATE_WAIT_FOR_JOB_COMPLETE,
+    STATE_WAIT,
+    STATE_WAIT_COMPLETE,
 
     STATE_INIT_CONNECTION,
     STATE_INIT_CONNECTION_COMPLETE,
     STATE_WAITING_USER_ACTION,
     STATE_RESTART_TUNNEL_AUTH,
     STATE_RESTART_TUNNEL_AUTH_COMPLETE,
     STATE_CREATE_STREAM,
     STATE_CREATE_STREAM_COMPLETE,
     STATE_DRAIN_BODY_FOR_AUTH_RESTART,
     STATE_DRAIN_BODY_FOR_AUTH_RESTART_COMPLETE,
@@ skipping 46 matching lines @@
     // the Job is not a SPDY alternative job, or if |spdy_session| is null.
     // Returns appropriate error code otherwise.
     int CheckAlternativeServiceValidityForOrigin(
         base::WeakPtr<SpdySession> spdy_session);
 
     SpdySessionPool* const spdy_session_pool_;
     const GURL origin_url_;
     const bool is_spdy_alternative_;
   };
 
-  // Resume the |this| job after the specified |wait_time_|.
-  void ResumeAfterDelay();
-
   void OnStreamReadyCallback();
   void OnBidirectionalStreamImplReadyCallback();
   void OnWebSocketHandshakeStreamReadyCallback();
   // This callback function is called when a new SPDY session is created.
   void OnNewSpdySessionReadyCallback();
   void OnStreamFailedCallback(int result);
   void OnCertificateErrorCallback(int result, const SSLInfo& ssl_info);
   void OnNeedsProxyAuthCallback(const HttpResponseInfo& response_info,
                                 HttpAuthController* auth_controller);
   void OnNeedsClientAuthCallback(SSLCertRequestInfo* cert_info);
   void OnHttpsProxyTunnelResponseCallback(const HttpResponseInfo& response_info,
                                           HttpStream* stream);
   void OnPreconnectsComplete();
 
   void OnIOComplete(int result);
   int RunLoop(int result);
   int DoLoop(int result);
   int StartInternal();
 
   // Each of these methods corresponds to a State value.  Those with an input
   // argument receive the result from the previous state.  If a method returns
   // ERR_IO_PENDING, then the result from OnIOComplete will be passed to the
   // next state method as the result arg.
   int DoStart();
   int DoResolveProxy();
   int DoResolveProxyComplete(int result);
-  int DoWaitForJob();
-  int DoWaitForJobComplete(int result);
+  int DoWait();
+  int DoWaitComplete(int result);
   int DoInitConnection();
   int DoInitConnectionComplete(int result);
   int DoWaitingUserAction(int result);
   int DoCreateStream();
   int DoCreateStreamComplete(int result);
   int DoRestartTunnelAuth();
   int DoRestartTunnelAuthComplete(int result);
 
   // Creates a SpdyHttpStream or a BidirectionalStreamImpl from the given values
   // and sets to |stream_| or |bidirectional_stream_impl_| respectively. Does
@@ skipping 72 matching lines @@
   const HttpRequestInfo request_info_;
   RequestPriority priority_;
   ProxyInfo proxy_info_;
   SSLConfig server_ssl_config_;
   SSLConfig proxy_ssl_config_;
   const BoundNetLog net_log_;
 
   CompletionCallback io_callback_;
   std::unique_ptr<ClientSocketHandle> connection_;
   HttpNetworkSession* const session_;
+  base::TimeDelta delay_;
+  State state_;

[Ryan Hamilton, 2016/06/29 23:12:28]

nit: as discussed, can we remove this?

[Zhongyi Shi, 2016/06/30 22:53:39]

Done.

   State next_state_;
   ProxyService::PacRequest* pac_request_;
   SSLInfo ssl_info_;
 
   // The server we are trying to reach, could be that of the origin or of the
   // alternative service (after applying host mapping rules).
   HostPortPair destination_;
 
   // The origin url we're trying to reach. This url may be different from the
   // original request when host mapping rules are set-up.
   GURL origin_url_;
 
   // AlternativeService for this Job if this is an alternative Job.
   const AlternativeService alternative_service_;
 
   // AlternativeService for the other Job if this is not an alternative Job.
   AlternativeService other_job_alternative_service_;
 
   // Unowned. |this| job is owned by |delegate_|.
   Delegate* delegate_;
 
   JobType job_type_;
 
-  // This is the Job we're dependent on. It will notify us if/when it's OK to
-  // proceed.
-  Job* blocking_job_;
-
-  // |waiting_job_| is a Job waiting to see if |this| can reuse a connection.
-  // If |this| is unable to do so, we'll notify |waiting_job_| that it's ok to
-  // proceed and then race the two Jobs.
-  Job* waiting_job_;
-
-  base::TimeDelta wait_time_;
-
   // True if handling a HTTPS request, or using SPDY with SSL
   bool using_ssl_;
 
   // True if this network transaction is using SPDY instead of HTTP.
   bool using_spdy_;
 
   // True if this network transaction is using QUIC instead of HTTP.
   bool using_quic_;
   QuicStreamRequest quic_request_;
 
@@ skipping 79 matching lines @@
       const SSLConfig& server_ssl_config,
       const SSLConfig& proxy_ssl_config,
       HostPortPair destination,
       GURL origin_url,
       NetLog* net_log) = 0;
 };
 
 }  // namespace net
 
 #endif  // NET_HTTP_HTTP_STREAM_FACTORY_IMPL_JOB_H_