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

net/spdy/spdy_session_pool.cc

 // 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.
 
 #include "net/spdy/spdy_session_pool.h"
 
 #include <utility>
 
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
@@ skipping 89 matching lines @@
   auto new_session = base::MakeUnique<SpdySession>(
       key, http_server_properties_, transport_security_state_,
       enable_sending_initial_data_, enable_ping_based_connection_checking_,
       session_max_recv_window_size_, initial_settings_, time_func_,
       push_delegate_, proxy_delegate_, net_log.net_log());
 
   new_session->InitializeWithSocket(std::move(connection), this, is_secure);
 
   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,
       available_session->net_log().source().ToEventParametersCallback());
 
   // Look up the IP address for this session so that we can match
   // future sessions (potentially to different domains) which can
   // potentially be pooled with this one. Because GetPeerAddress()
   // reports the proxy's address instead of the origin server, check
   // to see if this is a direct connection.
   if (key.proxy_server().is_direct()) {
     IPEndPoint address;
     if (available_session->GetPeerAddress(&address) == OK)
       aliases_[address] = key;
   }
 
   return available_session;
 }
 
 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);
   if (!url.is_empty() && url_it != unclaimed_pushed_streams_.end()) {
     DCHECK(url.SchemeIsCryptographic());
     for (WeakSessionList::iterator it = url_it->second.begin();
          it != url_it->second.end();) {
       base::WeakPtr<SpdySession> spdy_session = *it;
       // Lazy deletion of destroyed SpdySessions.
       if (!spdy_session) {
@@ skipping 19 matching lines @@
 
   AvailableSessionMap::iterator it = LookupAvailableSessionByKey(key);
   if (it != available_sessions_.end()) {
     if (key.Equals(it->second->spdy_session_key())) {
       UMA_HISTOGRAM_ENUMERATION("Net.SpdySessionGet", FOUND_EXISTING,
                                 SPDY_SESSION_GET_MAX);
       net_log.AddEvent(
           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);
       net_log.AddEvent(
           NetLogEventType::
               HTTP2_SESSION_POOL_FOUND_EXISTING_SESSION_FROM_IP_POOL,
           it->second->net_log().source().ToEventParametersCallback());
     }
     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;
   int rv = resolver_->ResolveFromCache(resolve_info, &addresses, net_log);
   DCHECK_NE(rv, ERR_IO_PENDING);
   if (rv != OK)
     return base::WeakPtr<SpdySession>();
 
   // Check if we have a session through a domain alias.
   for (AddressList::const_iterator address_it = addresses.begin();
@@ skipping 31 matching lines @@
     }
 
     UMA_HISTOGRAM_ENUMERATION("Net.SpdyIPPoolDomainMatch", 1, 2);
     UMA_HISTOGRAM_ENUMERATION("Net.SpdySessionGet",
                               FOUND_EXISTING_FROM_IP_POOL,
                               SPDY_SESSION_GET_MAX);
     net_log.AddEvent(
         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;
   }
 
   return base::WeakPtr<SpdySession>();
 }
 
 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));
 }
 
 void SpdySessionPool::RemoveUnavailableSession(
     const base::WeakPtr<SpdySession>& unavailable_session) {
   DCHECK(!IsSessionAvailable(unavailable_session));
 
   unavailable_session->net_log().AddEvent(
@@ skipping 160 matching lines @@
                   base::trace_event::MemoryAllocatorDump::kUnitsBytes,
                   buffer_size);
   dump->AddScalar("cert_count",
                   base::trace_event::MemoryAllocatorDump::kUnitsObjects,
                   cert_count);
   dump->AddScalar("cert_size",
                   base::trace_event::MemoryAllocatorDump::kUnitsBytes,
                   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();
        it != available_sessions_.end(); ++it) {
     if (it->second.get() == session.get())
       return true;
   }
   return false;
 }
 
 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
 SpdySessionPool::LookupAvailableSessionByKey(
     const SpdySessionKey& key) {
   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) {
   // Walk the aliases map, find references to this pair.
   // TODO(mbelshe):  Figure out if this is too expensive.
   for (AliasMap::iterator it = aliases_.begin(); it != aliases_.end(); ) {
     if (it->second.Equals(key)) {
       AliasMap::iterator old_it = it;
       ++it;
       aliases_.erase(old_it);
@@ skipping 24 matching lines @@
 
     if (idle_only && (*it)->is_active())
       continue;
 
     (*it)->CloseSessionOnError(error, description);
     DCHECK(!IsSessionAvailable(*it));
   }
 }
 
 }  // namespace net