RequestServer: Use an OwnPtr for the connection cache vector

Just as removing individual connections can cause the vector entries to
change positions, adding or removing connections to the cache can also
move the connections around, which would make it possible for a
connection to avoid being deleted (and make the RS spin on the Notifier
for that connection).
This commit makes it so that no connection cache is left when it's
supposed to be deleted.
Fixes a few more RS spins.

2 files changed, 21 insertions, 19 deletions

diff --git a/Userland/Services/RequestServer/ConnectionCache.cpp b/Userland/Services/RequestServer/ConnectionCache.cpp
index 6571715f52..f729eb03d5 100644
--- a/Userland/Services/RequestServer/ConnectionCache.cpp
+++ b/Userland/Services/RequestServer/ConnectionCache.cpp
@@ -9,8 +9,8 @@
 
 namespace RequestServer::ConnectionCache {
 
-HashMap<ConnectionKey, NonnullOwnPtrVector<Connection<Core::TCPSocket>>> g_tcp_connection_cache {};
-HashMap<ConnectionKey, NonnullOwnPtrVector<Connection<TLS::TLSv12>>> g_tls_connection_cache {};
+HashMap<ConnectionKey, NonnullOwnPtr<NonnullOwnPtrVector<Connection<Core::TCPSocket>>>> g_tcp_connection_cache {};
+HashMap<ConnectionKey, NonnullOwnPtr<NonnullOwnPtrVector<Connection<TLS::TLSv12>>>> g_tls_connection_cache {};
 
 void request_did_finish(URL const& url, Core::Socket const* socket)
 {
@@ -28,7 +28,7 @@ void request_did_finish(URL const& url, Core::Socket const* socket)
             dbgln("Request for URL {} finished, but we don't own that!", url);
             return;
         }
-        auto connection_it = it->value.find_if([&](auto& connection) { return connection->socket == socket; });
+        auto connection_it = it->value->find_if([&](auto& connection) { return connection->socket == socket; });
         if (connection_it.is_end()) {
             dbgln("Request for URL {} finished, but we don't have a socket for that!", url);
             return;
@@ -39,10 +39,11 @@ void request_did_finish(URL const& url, Core::Socket const* socket)
             connection->has_started = false;
             if constexpr (REQUEST_SERVER_DEBUG)
                 connection->current_url = {};
-            connection->removal_timer->on_timeout = [ptr = connection.ptr(), &cache_entry = it->value, &key = it->key, &cache] {
-                Core::deferred_invoke([&] {
-                    dbgln("Removing no-longer-used connection {}", ptr);
-                    cache_entry.remove_first_matching([&](auto& entry) { return entry.ptr() == ptr; });
+            connection->removal_timer->on_timeout = [ptr = connection.ptr(), &cache_entry = *it->value, key = it->key, &cache]() mutable {
+                Core::deferred_invoke([&, key = move(key), ptr] {
+                    dbgln("Removing no-longer-used connection {} (socket {})", ptr, ptr->socket);
+                    auto did_remove = cache_entry.remove_first_matching([&](auto& entry) { return entry == ptr; });
+                    VERIFY(did_remove);
                     if (cache_entry.is_empty())
                         cache.remove(key);
                 });
@@ -57,10 +58,10 @@ void request_did_finish(URL const& url, Core::Socket const* socket)
                 is_connected = connection->socket->is_connected();
             if (!is_connected) {
                 // Create another socket for the connection.
-                dbgln("Creating a new socket for {}", url);
                 connection->socket = SocketType::construct(nullptr);
+                dbgln("Creating a new socket for {} -> {}", url, connection->socket);
             }
-            dbgln("Running next job in queue for connection {}", &connection);
+            dbgln("Running next job in queue for connection {} @{}", &connection, connection->socket);
             auto request = connection->request_queue.take_first();
             if constexpr (REQUEST_SERVER_DEBUG) {
                 connection->timer.start();
@@ -84,8 +85,8 @@ void dump_jobs()
     dbgln("=========== TLS Connection Cache ==========");
     for (auto& connection : g_tls_connection_cache) {
         dbgln(" - {}:{}", connection.key.hostname, connection.key.port);
-        for (auto& entry : connection.value) {
-            dbgln("  - Connection {} (started={})", &entry, entry.has_started);
+        for (auto& entry : *connection.value) {
+            dbgln("  - Connection {} (started={}) (socket={})", &entry, entry.has_started, entry.socket);
             dbgln("    Currently loading {} ({} elapsed)", entry.current_url, entry.timer.elapsed());
             dbgln("    Request Queue:");
             for (auto& job : entry.request_queue)
@@ -95,8 +96,8 @@ void dump_jobs()
     dbgln("=========== TCP Connection Cache ==========");
     for (auto& connection : g_tcp_connection_cache) {
         dbgln(" - {}:{}", connection.key.hostname, connection.key.port);
-        for (auto& entry : connection.value) {
-            dbgln("  - Connection {} (started={})", &entry, entry.has_started);
+        for (auto& entry : *connection.value) {
+            dbgln("  - Connection {} (started={}) (socket={})", &entry, entry.has_started, entry.socket);
             dbgln("    Currently loading {} ({} elapsed)", entry.current_url, entry.timer.elapsed());
             dbgln("    Request Queue:");
             for (auto& job : entry.request_queue)
diff --git a/Userland/Services/RequestServer/ConnectionCache.h b/Userland/Services/RequestServer/ConnectionCache.h
index 4221244330..eba52f2b12 100644
--- a/Userland/Services/RequestServer/ConnectionCache.h
+++ b/Userland/Services/RequestServer/ConnectionCache.h
@@ -61,8 +61,8 @@ struct AK::Traits<RequestServer::ConnectionCache::ConnectionKey> : public AK::Ge
 
 namespace RequestServer::ConnectionCache {
 
-extern HashMap<ConnectionKey, NonnullOwnPtrVector<Connection<Core::TCPSocket>>> g_tcp_connection_cache;
-extern HashMap<ConnectionKey, NonnullOwnPtrVector<Connection<TLS::TLSv12>>> g_tls_connection_cache;
+extern HashMap<ConnectionKey, NonnullOwnPtr<NonnullOwnPtrVector<Connection<Core::TCPSocket>>>> g_tcp_connection_cache;
+extern HashMap<ConnectionKey, NonnullOwnPtr<NonnullOwnPtrVector<Connection<TLS::TLSv12>>>> g_tls_connection_cache;
 
 void request_did_finish(URL const&, Core::Socket const*);
 void dump_jobs();
@@ -72,14 +72,15 @@ constexpr static inline size_t ConnectionKeepAliveTimeMilliseconds = 10'000;
 
 decltype(auto) get_or_create_connection(auto& cache, URL const& url, auto& job)
 {
+    using CacheEntryType = RemoveCVReference<decltype(*cache.begin()->value)>;
     auto start_job = [&job](auto& socket) {
         job.start(socket);
     };
-    auto& sockets_for_url = cache.ensure({ url.host(), url.port_or_default() });
+    auto& sockets_for_url = *cache.ensure({ url.host(), url.port_or_default() }, [] { return make<CacheEntryType>(); });
     auto it = sockets_for_url.find_if([](auto& connection) { return connection->request_queue.is_empty(); });
     auto did_add_new_connection = false;
     if (it.is_end() && sockets_for_url.size() < ConnectionCache::MaxConcurrentConnectionsPerURL) {
-        using ConnectionType = RemoveCVReference<decltype(cache.begin()->value.at(0))>;
+        using ConnectionType = RemoveCVReference<decltype(cache.begin()->value->at(0))>;
         sockets_for_url.append(make<ConnectionType>(
             ConnectionType::SocketType::construct(nullptr),
             typename ConnectionType::QueueType {},
@@ -106,7 +107,7 @@ decltype(auto) get_or_create_connection(auto& cache, URL const& url, auto& job)
     }
     auto& connection = sockets_for_url[index];
     if (!connection.has_started) {
-        dbgln("Immediately start request for url {} in {}", url, &connection);
+        dbgln("Immediately start request for url {} in {} - {}", url, &connection, connection.socket);
         connection.has_started = true;
         connection.removal_timer->stop();
         if constexpr (REQUEST_SERVER_DEBUG) {
@@ -115,7 +116,7 @@ decltype(auto) get_or_create_connection(auto& cache, URL const& url, auto& job)
         }
         start_job(*connection.socket);
     } else {
-        dbgln("Enqueue request for URL {} in {}", url, &connection);
+        dbgln("Enqueue request for URL {} in {} - {}", url, &connection, connection.socket);
         connection.request_queue.append(move(start_job));
     }
     return connection;