Put back in the IntrusiveHashMap::insert ordering

Reverting back IntrusiveHashMap::insert ordering so that it puts things
at the end of the list when there is a colision.

Fix HttpSessionManager to walk the connection pool backwards.

Author: bneradt

lib/swoc/include/swoc/IntrusiveHashMap.h

@@ -514,6 +514,15 @@ IntrusiveHashMap<H>::insert(value_type *v) {
     if (spot != bucket->_v) {
       mixed_p = true; // found some other key, it's going to be mixed.
     }
+    if (spot != limit) {
+      // If an equal key was found, walk past those to insert at the upper end of the range.
+      do {
+        spot = H::next_ptr(spot);
+      } while (spot != limit && H::equal(key, H::key_of(spot)));
+      if (spot != limit) { // something not equal past last equivalent, it's going to be mixed.
+        mixed_p = true;
+      }
+    }
 
     _list.insert_before(spot, v);
     if (spot == bucket->_v) { // added before the bucket start, update the start.

lib/swoc/unit_tests/test_IntrusiveHashMap.cc

@@ -19,6 +19,7 @@
 */
 
 #include <iostream>
+#include <iterator>
 #include <string_view>
 #include <string>
 #include <bitset>
@@ -114,17 +115,38 @@ TEST_CASE("IntrusiveHashMap", "[libts][IntrusiveHashMap]") {
   map.insert(new Thing("dup"sv, 81));
 
   auto r = map.equal_range("dup"sv);
+
+  // Verify the range is correct and that accessing them one at a time works in
+  // FIFO order.
+  REQUIRE(r.begin()->_payload == "dup"sv);
+  REQUIRE(r.begin()->_n == 79);
   REQUIRE(r.first != r.second);
+  REQUIRE(r.first == r.begin());
+  REQUIRE(r.second == r.end());
   REQUIRE(r.first->_payload == "dup"sv);
+  REQUIRE(r.first->_n == 79);
+  REQUIRE((++r.first)->_payload == "dup"sv);
+  REQUIRE(r.first->_n == 80);
+  REQUIRE((++r.first)->_payload == "dup"sv);
   REQUIRE(r.first->_n == 81);
 
+  // Verify you can walk backwards, accessing the elements in a LIFO order.
+  r = map.equal_range("dup"sv);
+  auto reverse_it = std::make_reverse_iterator(r.end());
+  REQUIRE(reverse_it->_payload == "dup"sv);
+  REQUIRE(reverse_it->_n == 81);
+  REQUIRE((++reverse_it)->_payload == "dup"sv);
+  REQUIRE(reverse_it->_n == 80);
+  REQUIRE((++reverse_it)->_payload == "dup"sv);
+  REQUIRE(reverse_it->_n == 79);
+  REQUIRE((++reverse_it) == std::make_reverse_iterator(r.begin()));
+
   Map::iterator idx;
 
   // Erase all the non-"dup" and see if the range is still correct.
   map.apply([&map](Thing &thing) {
-    if (thing._payload != "dup"sv) {
+    if (thing._payload != "dup"sv)
       map.erase(map.iterator_for(&thing));
-    }
   });
   r = map.equal_range("dup"sv);
   REQUIRE(r.first != r.second);
@@ -138,6 +160,14 @@ TEST_CASE("IntrusiveHashMap", "[libts][IntrusiveHashMap]") {
   REQUIRE(idx->_n != r.first->_n);
   REQUIRE(idx->_n == 81);
   REQUIRE(++idx == map.end());
+
+  // Now verify we can go backwards.
+  REQUIRE((--idx)->_payload == "dup"sv);
+  REQUIRE(idx->_n != r.first->_n);
+  REQUIRE(idx->_n == 81);
+  REQUIRE((--idx)->_payload == "dup"sv);
+  REQUIRE(idx->_n != r.first->_n);
+  REQUIRE(idx->_n == 80);
   // Verify only the "dup" items are left.
   for (auto &&elt : map) {
     REQUIRE(elt._payload == "dup"sv);

src/proxy/http/HttpSessionManager.cc

@@ -34,6 +34,7 @@
 #include "proxy/ProxySession.h"
 #include "proxy/http/HttpSM.h"
 #include "proxy/http/HttpDebugNames.h"
+#include <iterator>
 
 // Initialize a thread to handle HTTP session management
 void
@@ -151,47 +152,52 @@ ServerSessionPool::acquireSession(sockaddr const *addr, CryptoHash const &hostna
     // This is broken out because only in this case do we check the host hash first. The range must be checked
     // to verify an upstream that matches port and SNI name is selected. Walk backwards to select oldest.
     in_port_t port = ats_ip_port_cast(addr);
-    auto first     = m_fqdn_pool.find(hostname_hash);
-    while (first != m_fqdn_pool.end() && first->hostname_hash == hostname_hash) {
-      Debug("http_ss", "Compare port 0x%x against 0x%x", port, ats_ip_port_cast(first->get_remote_addr()));
-      if (port == ats_ip_port_cast(first->get_remote_addr()) &&
-          (!(match_style & TS_SERVER_SESSION_SHARING_MATCH_MASK_SNI) || validate_sni(sm, first->get_netvc())) &&
-          (!(match_style & TS_SERVER_SESSION_SHARING_MATCH_MASK_HOSTSNISYNC) || validate_host_sni(sm, first->get_netvc())) &&
-          (!(match_style & TS_SERVER_SESSION_SHARING_MATCH_MASK_CERT) || validate_cert(sm, first->get_netvc()))) {
+    auto range     = m_fqdn_pool.equal_range(hostname_hash);
+    auto iter      = std::make_reverse_iterator(range.end());
+    auto const end = std::make_reverse_iterator(range.begin());
+    while (iter != end) {
+      Debug("http_ss", "Compare port 0x%x against 0x%x", port, ats_ip_port_cast(iter->get_remote_addr()));
+      if (port == ats_ip_port_cast(iter->get_remote_addr()) &&
+          (!(match_style & TS_SERVER_SESSION_SHARING_MATCH_MASK_SNI) || validate_sni(sm, iter->get_netvc())) &&
+          (!(match_style & TS_SERVER_SESSION_SHARING_MATCH_MASK_HOSTSNISYNC) || validate_host_sni(sm, iter->get_netvc())) &&
+          (!(match_style & TS_SERVER_SESSION_SHARING_MATCH_MASK_CERT) || validate_cert(sm, iter->get_netvc()))) {
         zret = HSM_DONE;
         break;
       }
-      ++first;
+      ++iter;
     }
     if (zret == HSM_DONE) {
-      to_return = first;
+      to_return = iter.base();
       if (!to_return->is_multiplexing()) {
         this->removeSession(to_return);
       }
-    } else if (first != m_fqdn_pool.end()) {
+    } else if (iter != end) {
       Debug("http_ss", "Failed find entry due to name mismatch %s", sm->t_state.current.server->name);
     }
   } else if (TS_SERVER_SESSION_SHARING_MATCH_MASK_IP & match_style) { // matching is not disabled.
-    auto first = m_ip_pool.find(addr);
+    auto range = m_ip_pool.equal_range(addr);
+    // We want to access the sessions in LIFO order, so start from the back of the list.
+    auto iter      = std::make_reverse_iterator(range.end());
+    auto const end = std::make_reverse_iterator(range.begin());
     // The range is all that is needed in the match IP case, otherwise need to scan for matching fqdn
     // And matches the other constraints as well
     // Note the port is matched as part of the address key so it doesn't need to be checked again.
     if (match_style & (~TS_SERVER_SESSION_SHARING_MATCH_MASK_IP)) {
-      while (first != m_ip_pool.end() && ats_ip_addr_port_eq(first->get_remote_addr(), addr)) {
-        if ((!(match_style & TS_SERVER_SESSION_SHARING_MATCH_MASK_HOSTONLY) || first->hostname_hash == hostname_hash) &&
-            (!(match_style & TS_SERVER_SESSION_SHARING_MATCH_MASK_SNI) || validate_sni(sm, first->get_netvc())) &&
-            (!(match_style & TS_SERVER_SESSION_SHARING_MATCH_MASK_HOSTSNISYNC) || validate_host_sni(sm, first->get_netvc())) &&
-            (!(match_style & TS_SERVER_SESSION_SHARING_MATCH_MASK_CERT) || validate_cert(sm, first->get_netvc()))) {
+      while (iter != end) {
+        if ((!(match_style & TS_SERVER_SESSION_SHARING_MATCH_MASK_HOSTONLY) || iter->hostname_hash == hostname_hash) &&
+            (!(match_style & TS_SERVER_SESSION_SHARING_MATCH_MASK_SNI) || validate_sni(sm, iter->get_netvc())) &&
+            (!(match_style & TS_SERVER_SESSION_SHARING_MATCH_MASK_HOSTSNISYNC) || validate_host_sni(sm, iter->get_netvc())) &&
+            (!(match_style & TS_SERVER_SESSION_SHARING_MATCH_MASK_CERT) || validate_cert(sm, iter->get_netvc()))) {
           zret = HSM_DONE;
           break;
         }
-        ++first;
+        ++iter;
       }
-    } else if (first != m_ip_pool.end()) {
+    } else if (iter != end) {
       zret = HSM_DONE;
     }
     if (zret == HSM_DONE) {
-      to_return = first;
+      to_return = iter.base();
       if (!to_return->is_multiplexing()) {
         this->removeSession(to_return);
       }