H2 session reuse refactor 1: Rework AddRequestToSpdySessionRequestMap()

Instead of taking an HttpStreamRequest,
SpdySessionPool::AddRequestToSpdySessionRequestMap() now takes a new
delegate interface. It returns an object that can be deleted to cancel the
request, instead of taking the HttpStreamRequest against to cancel.

Instead of relying on the HttpStreamRequest to track what
SpdySessionKey is being watched, the SpdySessionPool now handles that
itself.

Finally, the new delegate interface takes the SpdySession itself when
a matching SpdySession is creating, and then makes the appropriate stream
type on top of it, instead of requiring the SpdySessionPool to make a
stream of the correct type. The new behavior more closely matches the
common case where HttpStreamFactory::Job makes the streams.

Bug: 912727
Change-Id: Icb1138e2f7438459019e9ed6f04e38aecf408c94
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1549900
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#650403}

net/http/http_stream_factory_job_controller.cc

@@ -666,11 +666,12 @@ void HttpStreamFactory::JobController::SetSpdySessionKey(
     const SpdySessionKey& spdy_session_key) {
   DCHECK(!job->using_quic());
 
-  if (is_preconnect_ || IsJobOrphaned(job))
+  if (is_preconnect_ || IsJobOrphaned(job) || spdy_session_request_)
     return;
 
-  session_->spdy_session_pool()->AddRequestToSpdySessionRequestMap(
-      spdy_session_key, request_);
+  spdy_session_request_ =
+      session_->spdy_session_pool()->CreateRequestForSpdySession(
+          spdy_session_key, request_);
 }
 
 void HttpStreamFactory::JobController::
@@ -680,14 +681,16 @@ void HttpStreamFactory::JobController::
   if (is_preconnect_ || IsJobOrphaned(job))
     return;
 
-  RemoveRequestFromSpdySessionRequestMap();
+  spdy_session_request_.reset();
 }
 
 void HttpStreamFactory::JobController::
     RemoveRequestFromSpdySessionRequestMap() {
+  // Since |spdy_session_request_| references |request_|, |request_| should
+  // still be live at this point.
   DCHECK(request_);
-  session_->spdy_session_pool()->RemoveRequestFromSpdySessionRequestMap(
-      request_);
+
+  spdy_session_request_.reset();
 }
 
 const NetLogWithSource* HttpStreamFactory::JobController::GetNetLog() const {

net/http/http_stream_factory_job_controller.h

@@ -14,6 +14,7 @@
 #include "net/http/http_stream_factory_job.h"
 #include "net/http/http_stream_request.h"
 #include "net/socket/next_proto.h"
+#include "net/spdy/spdy_session_pool.h"
 
 namespace net {
 
@@ -397,6 +398,7 @@ class HttpStreamFactory::JobController
   int num_streams_;
   HttpStreamRequest::StreamType stream_type_;
   RequestPriority priority_;
+  std::unique_ptr<SpdySessionPool::SpdySessionRequest> spdy_session_request_;
   const NetLogWithSource net_log_;
 
   base::WeakPtrFactory<JobController> ptr_factory_;

net/http/http_stream_request.cc

@@ -11,6 +11,7 @@
 #include "base/stl_util.h"
 #include "net/http/bidirectional_stream_impl.h"
 #include "net/log/net_log_event_type.h"
+#include "net/spdy/bidirectional_stream_spdy_impl.h"
 #include "net/spdy/spdy_http_stream.h"
 #include "net/spdy/spdy_session.h"
 
@@ -52,6 +53,29 @@ void HttpStreamRequest::Complete(bool was_alpn_negotiated,
   using_spdy_ = using_spdy;
 }
 
+void HttpStreamRequest::OnSpdySessionAvailable(
+    bool was_alpn_negotiated,
+    NextProto negotiated_protocol,
+    bool using_spdy,
+    const SSLConfig& used_ssl_config,
+    const ProxyInfo& used_proxy_info,
+    NetLogSource source_dependency,
+    base::WeakPtr<SpdySession> spdy_session) {
+  DCHECK(spdy_session);
+  Complete(was_alpn_negotiated, negotiated_protocol, using_spdy);
+  if (stream_type_ == HttpStreamRequest::BIDIRECTIONAL_STREAM) {
+    helper_->OnBidirectionalStreamImplReadyOnPooledConnection(
+        used_ssl_config, used_proxy_info,
+        std::make_unique<BidirectionalStreamSpdyImpl>(spdy_session,
+                                                      source_dependency));
+  } else {
+    helper_->OnStreamReadyOnPooledConnection(
+        used_ssl_config, used_proxy_info,
+        std::make_unique<SpdyHttpStream>(spdy_session, kNoPushedStreamFound,
+                                         source_dependency));
+  }
+}
+
 void HttpStreamRequest::OnStreamReadyOnPooledConnection(
     const SSLConfig& used_ssl_config,
     const ProxyInfo& used_proxy_info,

net/http/http_stream_request.h

@@ -8,17 +8,18 @@
 #include <memory>
 
 #include "base/macros.h"
-#include "base/optional.h"
 #include "net/base/load_states.h"
 #include "net/base/net_error_details.h"
 #include "net/base/net_export.h"
 #include "net/base/request_priority.h"
 #include "net/http/http_response_info.h"
+#include "net/log/net_log_source.h"
 #include "net/log/net_log_with_source.h"
 #include "net/proxy_resolution/proxy_info.h"
 #include "net/socket/connection_attempts.h"
 #include "net/socket/next_proto.h"
 #include "net/spdy/spdy_session_key.h"
+#include "net/spdy/spdy_session_pool.h"
 #include "net/ssl/ssl_config.h"
 #include "net/ssl/ssl_info.h"
 #include "net/websockets/websocket_handshake_stream_base.h"
@@ -36,7 +37,8 @@ class SSLCertRequestInfo;
 // created, this object is the creator's handle for interacting with the
 // HttpStream creation process.  The request is cancelled by deleting it, after
 // which no callbacks will be invoked.
-class NET_EXPORT_PRIVATE HttpStreamRequest {
+class NET_EXPORT_PRIVATE HttpStreamRequest
+    : public SpdySessionPool::SpdySessionRequest::Delegate {
  public:
   // Indicates which type of stream is requested.
   enum StreamType {
@@ -191,7 +193,7 @@ class NET_EXPORT_PRIVATE HttpStreamRequest {
                     const NetLogWithSource& net_log,
                     StreamType stream_type);
 
-  ~HttpStreamRequest();
+  ~HttpStreamRequest() override;
 
   // When a HttpStream creation process is stalled due to necessity
   // of Proxy authentication credentials, the delegate OnNeedsProxyAuth
@@ -212,8 +214,17 @@ class NET_EXPORT_PRIVATE HttpStreamRequest {
   // layer in an attached Job for this stream request.
   void AddConnectionAttempts(const ConnectionAttempts& attempts);
 
-  // Called when a stream becomes available on a connection that was not created
-  // by this request.
+  // SpdySessionRequest::Delegate implementation:
+  //
+  // TODO(mmenke): Move this to HttpStreamFactory::Job.
+  void OnSpdySessionAvailable(bool was_alpn_negotiated,
+                              NextProto negotiated_protocol,
+                              bool using_spdy,
+                              const SSLConfig& used_ssl_config,
+                              const ProxyInfo& used_proxy_info,
+                              NetLogSource source_dependency,
+                              base::WeakPtr<SpdySession> spdy_session) override;
+
   void OnStreamReadyOnPooledConnection(const SSLConfig& used_ssl_config,
                                        const ProxyInfo& used_proxy_info,
                                        std::unique_ptr<HttpStream> stream);
@@ -247,20 +258,6 @@ class NET_EXPORT_PRIVATE HttpStreamRequest {
 
   const NetLogWithSource& net_log() const { return net_log_; }
 
-  // Called when the |helper_| 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) {
-    spdy_session_key_ = spdy_session_key;
-  }
-  bool HasSpdySessionKey() const { return spdy_session_key_.has_value(); }
-  const SpdySessionKey& GetSpdySessionKey() const {
-    DCHECK(HasSpdySessionKey());
-    return spdy_session_key_.value();
-  }
-  void ResetSpdySessionKey() { spdy_session_key_.reset(); }
-
   StreamType stream_type() const { return stream_type_; }
 
   bool completed() const { return completed_; }
@@ -275,8 +272,6 @@ class NET_EXPORT_PRIVATE HttpStreamRequest {
       websocket_handshake_stream_create_helper_;
   const NetLogWithSource net_log_;
 
-  base::Optional<SpdySessionKey> spdy_session_key_;
-
   bool completed_;
   bool was_alpn_negotiated_;
   // Protocol negotiated with the server.

net/spdy/spdy_session_pool.cc

@@ -30,8 +30,6 @@
 #include "net/log/net_log_source.h"
 #include "net/log/net_log_with_source.h"
 #include "net/socket/client_socket_handle.h"
-#include "net/spdy/bidirectional_stream_spdy_impl.h"
-#include "net/spdy/spdy_http_stream.h"
 #include "net/spdy/spdy_session.h"
 #include "net/third_party/quiche/src/spdy/core/hpack/hpack_constants.h"
 #include "net/third_party/quiche/src/spdy/core/hpack/hpack_huffman_table.h"
@@ -51,6 +49,25 @@ enum SpdySessionGetTypes {
 
 }  // namespace
 
+SpdySessionPool::SpdySessionRequest::Delegate::Delegate() = default;
+SpdySessionPool::SpdySessionRequest::Delegate::~Delegate() = default;
+
+SpdySessionPool::SpdySessionRequest::SpdySessionRequest(
+    const SpdySessionKey& key,
+    Delegate* delegate,
+    SpdySessionPool* spdy_session_pool)
+    : key_(key), delegate_(delegate), spdy_session_pool_(spdy_session_pool) {}
+
+SpdySessionPool::SpdySessionRequest::~SpdySessionRequest() {
+  if (spdy_session_pool_)
+    spdy_session_pool_->RemoveRequestForSpdySession(this);
+}
+
+void SpdySessionPool::SpdySessionRequest::OnRemovedFromPool() {
+  DCHECK(spdy_session_pool_);
+  spdy_session_pool_ = nullptr;
+}
+
 SpdySessionPool::SpdySessionPool(
     HostResolver* resolver,
     SSLConfigService* ssl_config_service,
@@ -406,20 +423,12 @@ void SpdySessionPool::OnNewSpdySessionReady(
     auto iter = spdy_session_request_map_.find(spdy_session_key);
     if (iter == spdy_session_request_map_.end())
       return;
-    HttpStreamRequest* request = *iter->second.begin();
-    request->Complete(was_alpn_negotiated, negotiated_protocol, using_spdy);
-    RemoveRequestFromSpdySessionRequestMap(request);
-    if (request->stream_type() == HttpStreamRequest::BIDIRECTIONAL_STREAM) {
-      request->OnBidirectionalStreamImplReadyOnPooledConnection(
-          used_ssl_config, used_proxy_info,
-          std::make_unique<BidirectionalStreamSpdyImpl>(spdy_session,
-                                                        source_dependency));
-    } else {
-      request->OnStreamReadyOnPooledConnection(
-          used_ssl_config, used_proxy_info,
-          std::make_unique<SpdyHttpStream>(spdy_session, kNoPushedStreamFound,
-                                           source_dependency));
-    }
+    SpdySessionRequest* request = *iter->second.begin();
+    SpdySessionRequest::Delegate* delegate = request->delegate();
+    RemoveRequestForSpdySession(request);
+    delegate->OnSpdySessionAvailable(
+        was_alpn_negotiated, negotiated_protocol, using_spdy, used_ssl_config,
+        used_proxy_info, source_dependency, spdy_session);
   }
   // TODO(mbelshe): Alert other valid requests.
 }
@@ -447,22 +456,23 @@ void SpdySessionPool::ResumePendingRequests(
   }
 }
 
-void SpdySessionPool::AddRequestToSpdySessionRequestMap(
+std::unique_ptr<SpdySessionPool::SpdySessionRequest>
+SpdySessionPool::CreateRequestForSpdySession(
     const SpdySessionKey& spdy_session_key,
-    HttpStreamRequest* request) {
-  if (request->HasSpdySessionKey())
-    return;
+    SpdySessionRequest::Delegate* delegate) {
+  DCHECK(delegate);
+
   RequestSet& request_set = spdy_session_request_map_[spdy_session_key];
-  DCHECK(!base::ContainsKey(request_set, request));
-  request_set.insert(request);
-  request->SetSpdySessionKey(spdy_session_key);
+  auto spdy_session_request =
+      std::make_unique<SpdySessionRequest>(spdy_session_key, delegate, this);
+  request_set.insert(spdy_session_request.get());
+  return spdy_session_request;
 }
 
-void SpdySessionPool::RemoveRequestFromSpdySessionRequestMap(
-    HttpStreamRequest* request) {
-  if (!request->HasSpdySessionKey())
-    return;
-  const SpdySessionKey& spdy_session_key = request->GetSpdySessionKey();
+void SpdySessionPool::RemoveRequestForSpdySession(SpdySessionRequest* request) {
+  DCHECK_EQ(this, request->spdy_session_pool());
+
+  const SpdySessionKey& spdy_session_key = request->key();
   // Resume all pending requests now that |request| is done/canceled.
   ResumePendingRequests(spdy_session_key);
 
@@ -473,8 +483,9 @@ void SpdySessionPool::RemoveRequestFromSpdySessionRequestMap(
   request_set.erase(request);
   if (request_set.empty())
     spdy_session_request_map_.erase(spdy_session_key);
-  // Resets |request|'s SpdySessionKey. This will invalid |spdy_session_key|.
-  request->ResetSpdySessionKey();
+  // Clears |request|'s SpdySessionPool pointer, to prevent it from calling into
+  // this method again.
+  request->OnRemovedFromPool();
 }
 
 void SpdySessionPool::DumpMemoryStats(

net/spdy/spdy_session_pool.h

@@ -26,6 +26,7 @@
 #include "net/base/network_change_notifier.h"
 #include "net/base/proxy_server.h"
 #include "net/cert/cert_database.h"
+#include "net/log/net_log_source.h"
 #include "net/proxy_resolution/proxy_config.h"
 #include "net/spdy/http2_push_promise_index.h"
 #include "net/spdy/server_push_delegate.h"
@@ -45,7 +46,6 @@ namespace net {
 class ClientSocketHandle;
 class HostResolver;
 class HttpServerProperties;
-class HttpStreamRequest;
 class NetLogWithSource;
 class NetworkQualityEstimator;
 class SpdySession;
@@ -70,6 +70,69 @@ class NET_EXPORT SpdySessionPool
     std::string payload;
   };
 
+  // A request for a SpdySession with a particular SpdySessionKey. These are
+  // created by calling CreateRequestForSpdySession(). The Delegate's
+  // OnSpdySessionAvailable() method will be invoked when a matching SpdySession
+  // is added to the pool, if the consumer that inserts the SpdySession also
+  // calls OnNewSpdySessionReady. The Delegate's OnSpdySessionAvailable() method
+  // will be invoked at most once for a single SpdySessionRequest.
+  //
+  // Destroying the request will stop watching the pool for such a session. The
+  // request must be destroyed before the SpdySessionPool is.
+  //
+  // TODO(mmenke): Remove the dependency on OnNewSpdySessionReady.
+  class SpdySessionRequest {
+   public:
+    // Interface for watching for when a SpdySession with a provided key is
+    // created.
+    class Delegate {
+     public:
+      Delegate();
+      virtual ~Delegate();
+
+      // |spdy_session| will not be null.
+      //
+      // TODO(mmenke): Remove all parameters except |spdy_session| and possibly
+      // |source_dependency|.
+      virtual void OnSpdySessionAvailable(
+          bool was_alpn_negotiated,
+          NextProto negotiated_protocol,
+          bool using_spdy,
+          const SSLConfig& used_ssl_config,
+          const ProxyInfo& used_proxy_info,
+          NetLogSource source_dependency,
+          base::WeakPtr<SpdySession> spdy_session) = 0;
+
+     private:
+      DISALLOW_COPY_AND_ASSIGN(Delegate);
+    };
+
+    // Constructor - this is called by the SpdySessionPool.
+    SpdySessionRequest(const SpdySessionKey& key,
+                       Delegate* delegate,
+                       SpdySessionPool* spdy_session_pool);
+
+    ~SpdySessionRequest();
+
+    // Called by SpdySessionPool to signal that the request has been removed
+    // from the SpdySessionPool.
+    void OnRemovedFromPool();
+
+    const SpdySessionKey& key() const { return key_; }
+    Delegate* delegate() { return delegate_; }
+
+    // The associated SpdySessionPool, or nullptr if OnRemovedFromPool() has
+    // been called.
+    SpdySessionPool* spdy_session_pool() { return spdy_session_pool_; }
+
+   private:
+    const SpdySessionKey key_;
+    Delegate* const delegate_;
+    SpdySessionPool* spdy_session_pool_;
+
+    DISALLOW_COPY_AND_ASSIGN(SpdySessionRequest);
+  };
+
   SpdySessionPool(
       HostResolver* host_resolver,
       SSLConfigService* ssl_config_service,
@@ -217,14 +280,17 @@ class NET_EXPORT SpdySessionPool
   // Resumes pending requests with |spdy_session_key|.
   void ResumePendingRequests(const SpdySessionKey& spdy_session_key);
 
-  // Adds |request| to |spdy_session_request_map_| under |spdy_session_key| Key.
-  // Sets |spdy_session_key| as |request|'s SpdySessionKey.
-  void AddRequestToSpdySessionRequestMap(const SpdySessionKey& spdy_session_key,
-                                         HttpStreamRequest* request);
-
-  // Removes |request| from |spdy_session_request_map_|. No-op if |request| does
-  // not have a SpdySessionKey.
-  void RemoveRequestFromSpdySessionRequestMap(HttpStreamRequest* request);
+  // Create a request and add it to |spdy_session_request_map_| under
+  // |spdy_session_key| Key. |delegate|'s OnSpdySessionAvailable() callback will
+  // be invoked if a consumer calls OnNewSpdySessionReady() with a live
+  // SpdySession. |delegate| must remain valid until either its
+  // OnSpdySessionAvailable() callback has been invoked, or until the returned
+  // SpdySessionRequest has been destroyed.
+  //
+  // TODO(mmenke):  Merge with FindAvailableSession.
+  std::unique_ptr<SpdySessionRequest> CreateRequestForSpdySession(
+      const SpdySessionKey& spdy_session_key,
+      SpdySessionRequest::Delegate* delegate);
 
   void set_network_quality_estimator(
       NetworkQualityEstimator* network_quality_estimator) {
@@ -234,7 +300,7 @@ class NET_EXPORT SpdySessionPool
  private:
   friend class SpdySessionPoolPeer;  // For testing.
 
-  typedef std::set<HttpStreamRequest*> RequestSet;
+  typedef std::set<SpdySessionRequest*> RequestSet;
   typedef std::map<SpdySessionKey, RequestSet> SpdySessionRequestMap;
   typedef std::set<SpdySession*> SessionSet;
   typedef std::vector<base::WeakPtr<SpdySession> > WeakSessionList;
@@ -242,6 +308,9 @@ class NET_EXPORT SpdySessionPool
       AvailableSessionMap;
   typedef std::multimap<IPEndPoint, SpdySessionKey> AliasMap;
 
+  // Removes |request| from |spdy_session_request_map_|.
+  void RemoveRequestForSpdySession(SpdySessionRequest* request);
+
   // Returns true iff |session| is in |available_sessions_|.
   bool IsSessionAvailable(const base::WeakPtr<SpdySession>& session) const;