ABANDONED Allow SpdySessionPool to find/create SpdySession with IP pooling disabled.

net/spdy/spdy_session_pool.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_SPDY_SPDY_SESSION_POOL_H_
 #define NET_SPDY_SPDY_SESSION_POOL_H_
 
 #include <stddef.h>
 
+#include <iterator>
 #include <map>
 #include <memory>
 #include <set>
 #include <string>
 #include <vector>
 
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "net/base/host_port_pair.h"
@@ skipping 45 matching lines @@
   ~SpdySessionPool() override;
 
   // In the functions below, a session is "available" if this pool has
   // a reference to it and there is some SpdySessionKey for which
   // FindAvailableSession() will return it. A session is "unavailable"
   // if this pool has a reference to it but it won't be returned by
   // FindAvailableSession() for any SpdySessionKey; for example, this
   // can happen when a session receives a GOAWAY frame and is still
   // processing existing streams.
 
-  // Create a new SPDY session from an existing socket.  There must
-  // not already be a session for the given key.
+  // Create a new SPDY session from an existing socket.  There must not already
+  // be a non IP pooled session for |key|.  However, a session originally opened
+  // with a different key may be marked as an available IP pooled session for
+  // |key|.  In other words, CreateAvailableSessionFromSocket() must not be
+  // called if a FindAvailableSession(enable_ip_based_pooling = false) call

[Ryan Hamilton, 2017/03/14 22:20:20]

nit: I'd just remove "enable... = " from this, or else wrap it in /**/s so as to
look more like actual code. Oh, but that method takes more arguments. Confusing
perhaps:

.. if a call to Find..Session() with enable_ip_based_pooling false would return
true.

[Bence, 2017/03/15 10:05:26]

Done.

+  // would return true (or just returned true).

[Ryan Hamilton, 2017/03/14 22:20:20]

nit: I'd remove the "(or just returned true)" clause here.

[Bence, 2017/03/15 10:05:26]

Done.

   //
   // |is_secure| can be false for testing or when SPDY is configured
   // to work with non-secure sockets.
   //
   // Returns the new SpdySession. Note that the SpdySession begins reading from
   // |connection| on a subsequent event loop iteration, so it may be closed
   // immediately afterwards if the first read of |connection| fails.
   base::WeakPtr<SpdySession> CreateAvailableSessionFromSocket(
       const SpdySessionKey& key,
       std::unique_ptr<ClientSocketHandle> connection,
       const NetLogWithSource& net_log,
       bool is_secure);
 
-  // Return an available session for |key| that has an unclaimed push stream for
-  // |url| if such exists and |url| is not empty, or else an available session
-  // for |key| if such exists, or else nullptr.
+  // If |url| is not empty and there is a session for |key| that has an
+  // unclaimed push stream for |url|, return it.  Otherwise if there is an
+  // available session for |key|, return it.  Otherwise if there is a session to
+  // use based on IP address, mark it as available for |key| and return it.
+  // Otherwise return nullptr.
   base::WeakPtr<SpdySession> FindAvailableSession(
       const SpdySessionKey& key,
       const GURL& url,
+      bool enable_ip_based_pooling,
       const NetLogWithSource& net_log);
 
   // Remove all mappings and aliases for the given session, which must
   // still be available. Except for in tests, this must be called by
   // the given session itself.
   void MakeSessionUnavailable(
       const base::WeakPtr<SpdySession>& available_session);
 
   // Removes an unavailable session from the pool.  Except for in
   // tests, this must be called by the given session itself.
@@ skipping 49 matching lines @@
   // We perform the same flushing as described above when certificate database
   // is changed.
   void OnCertDBChanged() override;
 
   void DumpMemoryStats(base::trace_event::ProcessMemoryDump* pmd,
                        const std::string& parent_dump_absolute_name) const;
 
  private:
   friend class SpdySessionPoolPeer;  // For testing.
 
+  // A container class that wraps two maps: non IP pooled sessions and IP pooled
+  // sessions, both keyed off SpdySessionKey.  Every non IP pooled session is
+  // keyed by its own SpdySessionKey, but the same session can occur multiple
+  // times under different keys in the IP pooled map.
+  // Every key can have up to one non IP pooled session and up to one IP pooled
+  // session, possibly simultaneously: for example, if FindAvailableSession() is
+  // called with IP pooling enabled, then later
+  // CreateAvailableSessionFromSocket() is called with the same key to open a
+  // non IP pooled session.
+  class AvailableSessionMap {
+   public:
+    template <class T, class U>
+    class Iterator;
+
+    using MapType = std::map<SpdySessionKey, base::WeakPtr<SpdySession>>;
+    using key_type = MapType::key_type;
+    using value_type = MapType::value_type;
+    using iterator = Iterator<MapType::iterator, AvailableSessionMap>;
+    using const_iterator =
+        Iterator<MapType::const_iterator, const AvailableSessionMap>;
+
+    template <class T, class U>

[Ryan Hamilton, 2017/03/14 22:20:20]

I'm sure it's obvious, but why does this need to be templated?

[Bence, 2017/03/15 10:05:27]

Good question.  It totally wasn't obvious to me, and I tried hard not to make it
templated.  Turns out that SpdySessionPool::IsSessionAvailable() is marked
const, so it needs const iterators, and its available_sessions_ member also
needs to be treated const.  I felt that it would be wrong to cast away
constness.

+    class Iterator {
+     public:
+      Iterator();
+      Iterator(const Iterator& other);
+      void operator=(const Iterator& other);
+      ~Iterator();
+
+      bool operator==(const Iterator& other) const;
+      bool operator!=(const Iterator& other) const;
+      T& operator->();
+      Iterator& operator++();
+
+     private:
+      friend SpdySessionPool::AvailableSessionMap;

[Bence, 2017/03/15 10:05:26]

I still need this friend to allow AvailableSessionMap::find() to use the private
constructor.  Does that feel like an encapsulation violation?  Shall I refactor
that to a static Iterator::find(), just like I did with begin() and end()?

+
+      Iterator(T iterator, bool ip_pooled, U* session_map);
+
+      T iterator_;
+      bool ip_pooled_;
+      U* session_map_;
+    };
+
+    AvailableSessionMap();
+    AvailableSessionMap(const AvailableSessionMap&) = delete;
+    void operator=(const AvailableSessionMap&) = delete;
+    ~AvailableSessionMap();
+
+    const_iterator begin() const;
+    const_iterator end() const;
+    iterator begin();
+    iterator end();
+    bool empty() const;
+
+    // If both IP pooled and non IP pooled entries exist,
+    // then return the non IP pooled one.
+    iterator find(const key_type& key);
+
+    // |value| must not be in the map (this is unlike std::map::insert).
+    void insert(const value_type& value, bool ip_pooled);
+
+    // |key| must exist (this is unlike std::map::erase).
+    void erase(const key_type& key, bool ip_pooled);
+
+   private:
+    friend iterator;
+    friend const_iterator;
+
+    MapType non_ip_pooled_sessions_;
+    MapType ip_pooled_sessions_;
+  };
+
   typedef std::set<SpdySession*> SessionSet;
   typedef std::vector<base::WeakPtr<SpdySession> > WeakSessionList;
-  typedef std::map<SpdySessionKey, base::WeakPtr<SpdySession> >
-      AvailableSessionMap;
   typedef std::map<IPEndPoint, SpdySessionKey> AliasMap;
   typedef std::map<GURL, WeakSessionList> UnclaimedPushedStreamMap;
 
   // Returns true iff |session| is in |available_sessions_|.
   bool IsSessionAvailable(const base::WeakPtr<SpdySession>& session) const;
 
   // Map the given key to the given session. There must not already be
-  // a mapping for |key|.
+  // a mapping for |key| in the bucket defined by |ip_pooled|.
   void MapKeyToAvailableSession(const SpdySessionKey& key,
-                                const base::WeakPtr<SpdySession>& session);
+                                const base::WeakPtr<SpdySession>& session,
+                                bool ip_pooled);
 
-  // Returns an iterator into |available_sessions_| for the given key,
-  // which may be equal to |available_sessions_.end()|.
   AvailableSessionMap::iterator LookupAvailableSessionByKey(
       const SpdySessionKey& key);
 
-  // Remove the mapping of the given key, which must exist.
-  void UnmapKey(const SpdySessionKey& key);
+  // Remove the mapping of |key| from the bucket defined by |ip_pooled|,
+  // which must exist.
+  void UnmapKey(const SpdySessionKey& key, bool ip_pooled);
 
   // Remove all aliases for |key| from the aliases table.
   void RemoveAliases(const SpdySessionKey& key);
 
   // Get a copy of the current sessions as a list of WeakPtrs. Used by
   // CloseCurrentSessionsHelper() below.
   WeakSessionList GetCurrentSessions() const;
 
   // Close only the currently existing SpdySessions with |error|.  Let
   // any new ones created while this method is running continue to
   // live. If |idle_only| is true only idle sessions are closed.
   void CloseCurrentSessionsHelper(
       Error error,
       const std::string& description,
       bool idle_only);
 
   HttpServerProperties* http_server_properties_;
 
   TransportSecurityState* transport_security_state_;
 
   // The set of all sessions. This is a superset of the sessions in
   // |available_sessions_|.
   //
   // |sessions_| owns all its SpdySession objects.
   SessionSet sessions_;
 
-  // This is a map of available sessions by key. A session may appear
-  // more than once in this map if it has aliases.
+  // This is a container of available sessions by key, with IP pooled and non IP
+  // pooled sessions stored separately.
   AvailableSessionMap available_sessions_;
 
   // A map of IPEndPoint aliases for sessions.
   AliasMap aliases_;
 
   // A map of all SpdySessions owned by |this| that have an unclaimed pushed
   // streams for a GURL.  Might contain invalid WeakPtr's.
   // A single SpdySession can only have at most one pushed stream for each GURL,
   // but it is possible that multiple SpdySessions have pushed streams for the
   // same GURL.
@@ skipping 21 matching lines @@
   // resources from origins that are different from those of their associated
   // streams. May be nullptr.
   ProxyDelegate* proxy_delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(SpdySessionPool);
 };
 
 }  // namespace net
 
 #endif  // NET_SPDY_SPDY_SESSION_POOL_H_