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

net/spdy/spdy_session_pool.h

Index: net/spdy/spdy_session_pool.h
diff --git a/net/spdy/spdy_session_pool.h b/net/spdy/spdy_session_pool.h
index fc82c40cc39b38ea2e99cf740b5d320f34e7d97d..2352214a1607e28aed2ca6a1b67ed906833e3e1a 100644
--- a/net/spdy/spdy_session_pool.h
+++ b/net/spdy/spdy_session_pool.h
@@ -7,6 +7,7 @@
 
 #include <stddef.h>
 
+#include <iterator>
 #include <map>
 #include <memory>
 #include <set>
@@ -72,8 +73,12 @@ class NET_EXPORT SpdySessionPool
   // 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.
@@ -87,12 +92,15 @@ class NET_EXPORT SpdySessionPool
       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
@@ -162,10 +170,81 @@ class NET_EXPORT SpdySessionPool
  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;
 
@@ -173,17 +252,17 @@ class NET_EXPORT SpdySessionPool
   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);
@@ -210,8 +289,8 @@ class NET_EXPORT SpdySessionPool
   // |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.