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

net/spdy/spdy_session_pool.cc

Index: net/spdy/spdy_session_pool.cc
diff --git a/net/spdy/spdy_session_pool.cc b/net/spdy/spdy_session_pool.cc
index 308a80b1e7175cefa941b7c3f2b4a7f55df6f586..fffb686debfb5be950ed275abf74644330c440b5 100644
--- a/net/spdy/spdy_session_pool.cc
+++ b/net/spdy/spdy_session_pool.cc
@@ -107,7 +107,7 @@ base::WeakPtr<SpdySession> SpdySessionPool::CreateAvailableSessionFromSocket(
 
   base::WeakPtr<SpdySession> available_session = new_session->GetWeakPtr();
   sessions_.insert(new_session.release());
-  MapKeyToAvailableSession(key, available_session);
+  MapKeyToAvailableSession(key, available_session, /* ip_pooled = */ false);
 
   net_log.AddEvent(
       NetLogEventType::HTTP2_SESSION_POOL_IMPORTED_SESSION_FROM_SOCKET,
@@ -130,6 +130,7 @@ base::WeakPtr<SpdySession> SpdySessionPool::CreateAvailableSessionFromSocket(
 base::WeakPtr<SpdySession> SpdySessionPool::FindAvailableSession(
     const SpdySessionKey& key,
     const GURL& url,
+    bool enable_ip_based_pooling,
     const NetLogWithSource& net_log) {
   UnclaimedPushedStreamMap::iterator url_it =
       unclaimed_pushed_streams_.find(url);
@@ -169,6 +170,9 @@ base::WeakPtr<SpdySession> SpdySessionPool::FindAvailableSession(
           NetLogEventType::HTTP2_SESSION_POOL_FOUND_EXISTING_SESSION,
           it->second->net_log().source().ToEventParametersCallback());
     } else {
+      if (!enable_ip_based_pooling)
+        return base::WeakPtr<SpdySession>();
+
       UMA_HISTOGRAM_ENUMERATION("Net.SpdySessionGet",
                                 FOUND_EXISTING_FROM_IP_POOL,
                                 SPDY_SESSION_GET_MAX);
@@ -180,6 +184,9 @@ base::WeakPtr<SpdySession> SpdySessionPool::FindAvailableSession(
     return it->second;
   }
 
+  if (!enable_ip_based_pooling)
+    return base::WeakPtr<SpdySession>();
+
   // Look up IP addresses from resolver cache.
   HostResolver::RequestInfo resolve_info(key.host_port_pair());
   AddressList addresses;
@@ -231,7 +238,7 @@ base::WeakPtr<SpdySession> SpdySessionPool::FindAvailableSession(
         NetLogEventType::HTTP2_SESSION_POOL_FOUND_EXISTING_SESSION_FROM_IP_POOL,
         available_session->net_log().source().ToEventParametersCallback());
     // Add this session to the map so that we can find it next time.
-    MapKeyToAvailableSession(key, available_session);
+    MapKeyToAvailableSession(key, available_session, /* ip_pooled = */ true);
     available_session->AddPooledAlias(key);
     return available_session;
   }
@@ -241,12 +248,12 @@ base::WeakPtr<SpdySession> SpdySessionPool::FindAvailableSession(
 
 void SpdySessionPool::MakeSessionUnavailable(
     const base::WeakPtr<SpdySession>& available_session) {
-  UnmapKey(available_session->spdy_session_key());
+  UnmapKey(available_session->spdy_session_key(), /* ip_pooled = */ false);

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

Can you add a comment for why we use false here, and true below. (1 non-pooled,
and N-pooled entries, I believe)

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

Done.

   RemoveAliases(available_session->spdy_session_key());
   const std::set<SpdySessionKey>& aliases = available_session->pooled_aliases();
   for (std::set<SpdySessionKey>::const_iterator it = aliases.begin();
        it != aliases.end(); ++it) {
-    UnmapKey(*it);
+    UnmapKey(*it, /* ip_pooled = */ true);
     RemoveAliases(*it);
   }
   DCHECK(!IsSessionAvailable(available_session));
@@ -427,6 +434,175 @@ void SpdySessionPool::DumpMemoryStats(
                   cert_size);
 }
 
+template <class T, class U>
+SpdySessionPool::AvailableSessionMap::Iterator<T, U>::Iterator()
+    : ip_pooled_(false), session_map_(nullptr) {}
+
+template <class T, class U>
+SpdySessionPool::AvailableSessionMap::Iterator<T, U>::Iterator(
+    const Iterator& other)
+    : iterator_(other.iterator_),
+      ip_pooled_(other.ip_pooled_),
+      session_map_(other.session_map_) {}
+
+template <class T, class U>
+void SpdySessionPool::AvailableSessionMap::Iterator<T, U>::operator=(
+    const Iterator& other) {
+  iterator_ = other.iterator_;
+  ip_pooled_ = other.ip_pooled_;
+  session_map_ = other.session_map_;
+}
+
+template <class T, class U>
+SpdySessionPool::AvailableSessionMap::Iterator<T, U>::~Iterator() {}
+
+template <class T, class U>
+bool SpdySessionPool::AvailableSessionMap::Iterator<T, U>::operator==(
+    const SpdySessionPool::AvailableSessionMap::Iterator<T, U>& other) const {
+  // Compare |iterator_| last, because comparing iterators from different
+  // containers results in a runtime error when using debug_map.
+  return session_map_ == other.session_map_ && ip_pooled_ == other.ip_pooled_ &&
+         iterator_ == other.iterator_;
+}
+
+template <class T, class U>
+bool SpdySessionPool::AvailableSessionMap::Iterator<T, U>::operator!=(
+    const SpdySessionPool::AvailableSessionMap::Iterator<T, U>& other) const {
+  return !(*this == other);
+}
+
+template <class T, class U>
+T& SpdySessionPool::AvailableSessionMap::Iterator<T, U>::operator->() {
+  return iterator_;
+}
+
+template <class T, class U>
+SpdySessionPool::AvailableSessionMap::Iterator<T, U>&
+    SpdySessionPool::AvailableSessionMap::Iterator<T, U>::operator++() {
+  // ++(std_map.end()) is undefined, and so is ++(available_session_map.end()).

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

DCHECK_NE(available_session_map.end(), iterator_);

or some such?

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

Done.

+  ++iterator_;
+
+  if (ip_pooled_)

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

Can you add a comment which explains why this is true? (basically that first we
go through non-pooled, then pooled. So if we're already pooled we can skip the
special logic below. (or something)

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

Done.

+    return *this;
+
+  if (iterator_ == session_map_->non_ip_pooled_sessions_.end()) {
+    // This might coincide with ip_pooled_sessions_.end()
+    // if ip_pooled_sessions_ is empty,
+    // which yields the internal representation of available_session_map.end().

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

I don't understand this comment. Perhaps you're saying that if
ip_pooled_sessions_ is empty() then being() will return end() which is correct?

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

Done.

+    iterator_ = session_map_->ip_pooled_sessions_.begin();
+    ip_pooled_ = true;
+  }
+
+  return *this;
+}
+
+template <class T, class U>
+SpdySessionPool::AvailableSessionMap::Iterator<T, U>::Iterator(T iterator,
+                                                               bool ip_pooled,
+                                                               U* session_map)
+    : iterator_(iterator), ip_pooled_(ip_pooled), session_map_(session_map) {}
+
+SpdySessionPool::AvailableSessionMap::AvailableSessionMap() {}
+SpdySessionPool::AvailableSessionMap::~AvailableSessionMap() {}
+
+SpdySessionPool::AvailableSessionMap::const_iterator
+SpdySessionPool::AvailableSessionMap::begin() const {
+  // Must return a dereferencable iterator unless both maps are empty.
+  if (non_ip_pooled_sessions_.empty()) {
+    // If |ip_pooled_sessions_| is not empty, this is deferencable.
+    // If |ip_pooled_sessions_| is empty, then |cbegin() == cend()|, therefore
+    // the return value represents AvailableSessionMap::end().
+    return SpdySessionPool::AvailableSessionMap::Iterator<
+        MapType::const_iterator, const SpdySessionPool::AvailableSessionMap>(
+        ip_pooled_sessions_.cbegin(), /* ip_pooled = */ true, this);
+  } else {

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

nit: since 517 returned, you can avoid the else here (and the associated
nesting).

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

Done.

+    // |non_ip_pooled_sessions_| is not empty, therefore this is dereferencable.
+    return SpdySessionPool::AvailableSessionMap::Iterator<
+        MapType::const_iterator, const SpdySessionPool::AvailableSessionMap>(
+        non_ip_pooled_sessions_.cbegin(), /* ip_pooled = */ false, this);

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

I think it would make more sense to move all of this logic to the ::Iterator
constructor. In this case, you could pass in only the session map and let the
iterator construct the correct state internally. Since T iterator and ip_pool
are basically private, that would put all the logic in the same place, I think.

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

Done.  Ended up using a static factory method.  Templating allows me to use the
same method for iterator and const_iterator.

+  }
+}
+
+SpdySessionPool::AvailableSessionMap::const_iterator
+SpdySessionPool::AvailableSessionMap::end() const {
+  return SpdySessionPool::AvailableSessionMap::Iterator<
+      MapType::const_iterator, const SpdySessionPool::AvailableSessionMap>(
+      ip_pooled_sessions_.cend(), /* ip_pooled = */ true, this);

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

Consider implementing end() directly in Iterator 'cause this feel a bit like an
encapsulation violation.

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

Done.

+}
+
+SpdySessionPool::AvailableSessionMap::iterator
+SpdySessionPool::AvailableSessionMap::begin() {
+  // Must return a dereferencable iterator unless both maps are empty.
+  if (non_ip_pooled_sessions_.empty()) {
+    // If |ip_pooled_sessions_| is not empty, this is deferencable.
+    // If |ip_pooled_sessions_| is empty, then |cbegin() == cend()|, therefore
+    // the return value represents AvailableSessionMap::end().
+    return SpdySessionPool::AvailableSessionMap::Iterator<
+        MapType::iterator, SpdySessionPool::AvailableSessionMap>(
+        ip_pooled_sessions_.begin(), /* ip_pooled = */ true, this);
+  } else {
+    // |non_ip_pooled_sessions_| is not empty, therefore this is dereferencable.
+    return SpdySessionPool::AvailableSessionMap::Iterator<
+        MapType::iterator, SpdySessionPool::AvailableSessionMap>(
+        non_ip_pooled_sessions_.begin(), /* ip_pooled = */ false, this);
+  }

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

ditto to const begin.

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

Done.

+}
+
+SpdySessionPool::AvailableSessionMap::iterator
+SpdySessionPool::AvailableSessionMap::end() {
+  return SpdySessionPool::AvailableSessionMap::Iterator<
+      MapType::iterator, SpdySessionPool::AvailableSessionMap>(
+      ip_pooled_sessions_.end(), /* ip_pooled = */ true, this);
+}
+
+bool SpdySessionPool::AvailableSessionMap::empty() const {
+  return non_ip_pooled_sessions_.empty() && ip_pooled_sessions_.empty();
+}
+
+SpdySessionPool::AvailableSessionMap::iterator
+SpdySessionPool::AvailableSessionMap::find(const key_type& key) {
+  MapType::iterator it = non_ip_pooled_sessions_.find(key);
+  if (it != non_ip_pooled_sessions_.end())
+    return SpdySessionPool::AvailableSessionMap::Iterator<
+        MapType::iterator, SpdySessionPool::AvailableSessionMap>(
+        it,
+        /* ip_pooled = */ false, this);
+
+  it = ip_pooled_sessions_.find(key);
+  // If |it == ip_pooled_sessions_.end()|, then this is the internal
+  // representation of AvailableSessionMap::end().
+  return SpdySessionPool::AvailableSessionMap::Iterator<
+      MapType::iterator, SpdySessionPool::AvailableSessionMap>(
+      it, /* ip_pooled = */ true, this);
+}
+
+void SpdySessionPool::AvailableSessionMap::insert(const value_type& value,
+                                                  bool ip_pooled) {
+  // A session is non IP pooled with respect to the key
+  // iff the key matches the session's proper key.
+  CHECK_EQ(!ip_pooled, value.first.Equals(value.second->spdy_session_key()));
+
+  // There must not already be an entry with key |value.first| in the map.
+  std::pair<MapType::iterator, bool> result =
+      ip_pooled ? ip_pooled_sessions_.insert(value)
+                : non_ip_pooled_sessions_.insert(value);
+  CHECK(result.second);
+}
+
+void SpdySessionPool::AvailableSessionMap::erase(const key_type& key,
+                                                 bool ip_pooled) {
+  // |key| must exist in the map.
+  if (ip_pooled) {
+    MapType::iterator it = ip_pooled_sessions_.find(key);
+    CHECK(it != ip_pooled_sessions_.end());
+    ip_pooled_sessions_.erase(it);
+  } else {
+    MapType::iterator it = non_ip_pooled_sessions_.find(key);
+    CHECK(it != non_ip_pooled_sessions_.end());
+    non_ip_pooled_sessions_.erase(it);
+  }
+}
+
 bool SpdySessionPool::IsSessionAvailable(
     const base::WeakPtr<SpdySession>& session) const {
   for (AvailableSessionMap::const_iterator it = available_sessions_.begin();
@@ -439,11 +615,10 @@ bool SpdySessionPool::IsSessionAvailable(
 
 void SpdySessionPool::MapKeyToAvailableSession(
     const SpdySessionKey& key,
-    const base::WeakPtr<SpdySession>& session) {
+    const base::WeakPtr<SpdySession>& session,
+    bool ip_pooled) {
   DCHECK(base::ContainsKey(sessions_, session.get()));
-  std::pair<AvailableSessionMap::iterator, bool> result =
-      available_sessions_.insert(std::make_pair(key, session));
-  CHECK(result.second);
+  available_sessions_.insert(std::make_pair(key, session), ip_pooled);
 }
 
 SpdySessionPool::AvailableSessionMap::iterator
@@ -452,10 +627,8 @@ SpdySessionPool::LookupAvailableSessionByKey(
   return available_sessions_.find(key);
 }
 
-void SpdySessionPool::UnmapKey(const SpdySessionKey& key) {
-  AvailableSessionMap::iterator it = LookupAvailableSessionByKey(key);
-  CHECK(it != available_sessions_.end());
-  available_sessions_.erase(it);
+void SpdySessionPool::UnmapKey(const SpdySessionKey& key, bool ip_pooled) {
+  available_sessions_.erase(key, ip_pooled);
 }
 
 void SpdySessionPool::RemoveAliases(const SpdySessionKey& key) {