RequestServer: Use an OwnPtr for cached connections

Otherwise we'd end up trying to delete the wrong connection if a
connection made before us is deleted.
Fixes _some_ RequestServer spins (though not all...).
This commit also adds a small debug mechanism to RequestServer (which
can be enabled by turning REQUEST_SERVER_DEBUG on), that can dump all
the current active connections in the cache, what they're doing, and how
long they've been doing that by sending it a SIGINFO.

AK/Debug.h.in

@@ -338,6 +338,10 @@
 #cmakedefine01 REGEX_DEBUG
 #endif
 
+#ifndef REQUEST_SERVER_DEBUG
+#cmakedefine01 REQUEST_SERVER_DEBUG
+#endif
+
 #ifndef RESIZE_DEBUG
 #cmakedefine01 RESIZE_DEBUG
 #endif

Meta/CMake/all_the_debug_macros.cmake

@@ -141,6 +141,7 @@ set(PTHREAD_DEBUG ON)
 set(PTMX_DEBUG ON)
 set(REACHABLE_DEBUG ON)
 set(REGEX_DEBUG ON)
+set(REQUEST_SERVER_DEBUG ON)
 set(RESIZE_DEBUG ON)
 set(RESOURCE_DEBUG ON)
 set(ROUTING_DEBUG ON)

Userland/Services/RequestServer/ConnectionCache.cpp

@@ -9,8 +9,8 @@
 
 namespace RequestServer::ConnectionCache {
 
-HashMap<ConnectionKey, Vector<Connection<Core::TCPSocket>>> g_tcp_connection_cache {};
+HashMap<ConnectionKey, NonnullOwnPtrVector<Connection<Core::TCPSocket>>> g_tcp_connection_cache {};
-HashMap<ConnectionKey, Vector<Connection<TLS::TLSv12>>> g_tls_connection_cache {};
+HashMap<ConnectionKey, NonnullOwnPtrVector<Connection<TLS::TLSv12>>> g_tls_connection_cache {};
 
 void request_did_finish(URL const& url, Core::Socket const* socket)
 {
@@ -28,37 +28,45 @@ 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;
         }
 
         auto& connection = *connection_it;
-        if (connection.request_queue.is_empty()) {
+        if (connection->request_queue.is_empty()) {
-            connection.has_started = false;
+            connection->has_started = false;
-            connection.removal_timer->on_timeout = [&connection, &cache_entry = it->value] {
+            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 {}", &connection);
+                    dbgln("Removing no-longer-used connection {}", ptr);
-                    cache_entry.remove_first_matching([&](auto& entry) { return &entry == &connection; });
+                    cache_entry.remove_first_matching([&](auto& entry) { return entry.ptr() == ptr; });
+                    if (cache_entry.is_empty())
+                        cache.remove(key);
                 });
             };
-            connection.removal_timer->start();
+            connection->removal_timer->start();
         } else {
-            using SocketType = RemoveCVReference<decltype(*connection.socket)>;
+            using SocketType = RemoveCVReference<decltype(*connection->socket)>;
             bool is_connected;
             if constexpr (IsSame<SocketType, TLS::TLSv12>)
-                is_connected = connection.socket->is_established();
+                is_connected = connection->socket->is_established();
             else
-                is_connected = connection.socket->is_connected();
+                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);
+                connection->socket = SocketType::construct(nullptr);
             }
             dbgln("Running next job in queue for connection {}", &connection);
-            auto request = connection.request_queue.take_first();
+            auto request = connection->request_queue.take_first();
-            request(connection.socket);
+            if constexpr (REQUEST_SERVER_DEBUG) {
+                connection->timer.start();
+                connection->current_url = url;
+            }
+            request(connection->socket);
         }
     };
 
@@ -70,4 +78,32 @@ void request_did_finish(URL const& url, Core::Socket const* socket)
         dbgln("Unknown socket {} finished for URL {}", *socket, url);
 }
 
+#if REQUEST_SERVER_DEBUG
+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);
+            dbgln("    Currently loading {} ({} elapsed)", entry.current_url, entry.timer.elapsed());
+            dbgln("    Request Queue:");
+            for (auto& job : entry.request_queue)
+                dbgln("    - {}", &job);
+        }
+    }
+    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);
+            dbgln("    Currently loading {} ({} elapsed)", entry.current_url, entry.timer.elapsed());
+            dbgln("    Request Queue:");
+            for (auto& job : entry.request_queue)
+                dbgln("    - {}", &job);
+        }
+    }
+}
+#endif
+
 }

Userland/Services/RequestServer/ConnectionCache.h

@@ -6,9 +6,12 @@
 
 #pragma once
 
+#include <AK/Debug.h>
 #include <AK/HashMap.h>
+#include <AK/NonnullOwnPtrVector.h>
 #include <AK/URL.h>
 #include <AK/Vector.h>
+#include <LibCore/ElapsedTimer.h>
 #include <LibCore/TCPSocket.h>
 #include <LibCore/Timer.h>
 #include <LibTLS/TLSv12.h>
@@ -33,6 +36,10 @@ struct Connection {
     QueueType request_queue;
     NonnullRefPtr<Core::Timer> removal_timer;
     bool has_started { false };
+#if REQUEST_SERVER_DEBUG
+    URL current_url {};
+    Core::ElapsedTimer timer {};
+#endif
 };
 
 struct ConnectionKey {
@@ -54,10 +61,11 @@ struct AK::Traits<RequestServer::ConnectionCache::ConnectionKey> : public AK::Ge
 
 namespace RequestServer::ConnectionCache {
 
-extern HashMap<ConnectionKey, Vector<Connection<Core::TCPSocket>>> g_tcp_connection_cache;
+extern HashMap<ConnectionKey, NonnullOwnPtrVector<Connection<Core::TCPSocket>>> g_tcp_connection_cache;
-extern HashMap<ConnectionKey, Vector<Connection<TLS::TLSv12>>> g_tls_connection_cache;
+extern HashMap<ConnectionKey, NonnullOwnPtrVector<Connection<TLS::TLSv12>>> g_tls_connection_cache;
 
 void request_did_finish(URL const&, Core::Socket const*);
+void dump_jobs();
 
 constexpr static inline size_t MaxConcurrentConnectionsPerURL = 2;
 constexpr static inline size_t ConnectionKeepAliveTimeMilliseconds = 10'000;
@@ -68,14 +76,14 @@ decltype(auto) get_or_create_connection(auto& cache, URL const& url, auto& job)
         job.start(socket);
     };
     auto& sockets_for_url = cache.ensure({ url.host(), url.port_or_default() });
-    auto it = sockets_for_url.find_if([](auto& connection) { return connection.request_queue.is_empty(); });
+    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) {
-        sockets_for_url.append({
+        using ConnectionType = RemoveCVReference<decltype(cache.begin()->value.at(0))>;
-            RemoveCVReference<decltype(cache.begin()->value.at(0))>::SocketType::construct(nullptr),
+        sockets_for_url.append(make<ConnectionType>(
-            {},
+            ConnectionType::SocketType::construct(nullptr),
-            Core::Timer::create_single_shot(ConnectionKeepAliveTimeMilliseconds, nullptr),
+            typename ConnectionType::QueueType {},
-        });
+            Core::Timer::create_single_shot(ConnectionKeepAliveTimeMilliseconds, nullptr)));
         did_add_new_connection = true;
     }
     size_t index;
@@ -101,6 +109,10 @@ decltype(auto) get_or_create_connection(auto& cache, URL const& url, auto& job)
         dbgln("Immediately start request for url {} in {}", url, &connection);
         connection.has_started = true;
         connection.removal_timer->stop();
+        if constexpr (REQUEST_SERVER_DEBUG) {
+            connection.timer.start();
+            connection.current_url = url;
+        }
         start_job(*connection.socket);
     } else {
         dbgln("Enqueue request for URL {} in {}", url, &connection);

Userland/Services/RequestServer/main.cpp

@@ -4,6 +4,7 @@
  * SPDX-License-Identifier: BSD-2-Clause
  */
 
+#include <AK/Debug.h>
 #include <AK/OwnPtr.h>
 #include <LibCore/EventLoop.h>
 #include <LibCore/LocalServer.h>
@@ -16,9 +17,18 @@
 
 int main(int, char**)
 {
-    if (pledge("stdio inet accept unix rpath sendfd recvfd", nullptr) < 0) {
+    if constexpr (REQUEST_SERVER_DEBUG) {
-        perror("pledge");
+        if (pledge("stdio inet accept unix rpath sendfd recvfd sigaction", nullptr) < 0) {
-        return 1;
+            perror("pledge");
+            return 1;
+        }
+
+        signal(SIGINFO, [](int) { RequestServer::ConnectionCache::dump_jobs(); });
+    } else {
+        if (pledge("stdio inet accept unix rpath sendfd recvfd", nullptr) < 0) {
+            perror("pledge");
+            return 1;
+        }
     }
 
     // Ensure the certificates are read out here.