Merge bitcoin/bitcoin#28551: http: bugfix: allow server shutdown in case of remote client disconnection

68f23f5 http: bugfix: track closed connection (stickies-v)
084d037 http: log connection instead of request count (stickies-v)
41f9027 http: refactor: use encapsulated HTTPRequestTracker (stickies-v)

Pull request description:

  #26742 significantly increased the http server shutdown speed, but also introduced a bug (#27722 - see bitcoin/bitcoin#27722 (comment) for steps to reproduce on master) that causes http server shutdown to halt in case of a remote client disconnection. This happens because `evhttp_request_set_on_complete_cb` is never called and thus the request is never removed from `g_requests`.

  This PR fixes that bug, and improves robustness of the code by encapsulating the request tracking logic. Earlier approaches (#27909, #27245, #19434) attempted to resolve this but [imo are fundamentally unsafe](bitcoin/bitcoin#27909 (comment)) because of differences in lifetime between an `evhttp_request` and `evhttp_connection`.

  We don't need to keep track of open requests or connections, we just [need to ensure](bitcoin/bitcoin#19420 (comment)) that there are no active requests on server shutdown. Because a connection can have multiple requests, and a request can be completed in various ways (the request actually being handled, or the client performing a remote disconnect), keeping a counter per connection seems like the approach with the least overhead to me.

  Fixes #27722

ACKs for top commit:
  vasild:
    ACK 68f23f5
  theStack:
    ACK 68f23f5

Tree-SHA512: dfa711ff55ec75ba44d73e9e6fac16b0be25cf3c20868c2145a844a7878ad9fc6998d9ff62d72f3a210bfa411ef03d3757b73d68a7c22926e874c421e51444d6

Author: fanquake

src/httpserver.cpp

@@ -17,6 +17,7 @@
 #include <rpc/protocol.h> // For HTTP status codes
 #include <shutdown.h>
 #include <sync.h>
+#include <util/check.h>
 #include <util/strencodings.h>
 #include <util/threadnames.h>
 #include <util/translation.h>
@@ -28,7 +29,7 @@
 #include <memory>
 #include <optional>
 #include <string>
-#include <unordered_set>
+#include <unordered_map>
 
 #include <sys/types.h>
 #include <sys/stat.h>
@@ -149,10 +150,61 @@ static GlobalMutex g_httppathhandlers_mutex;
 static std::vector<HTTPPathHandler> pathHandlers GUARDED_BY(g_httppathhandlers_mutex);
 //! Bound listening sockets
 static std::vector<evhttp_bound_socket *> boundSockets;
+
+/**
+ * @brief Helps keep track of open `evhttp_connection`s with active `evhttp_requests`
+ *
+ */
+class HTTPRequestTracker
+{
+private:
+    mutable Mutex m_mutex;
+    mutable std::condition_variable m_cv;
+    //! For each connection, keep a counter of how many requests are open
+    std::unordered_map<const evhttp_connection*, size_t> m_tracker GUARDED_BY(m_mutex);
+
+    void RemoveConnectionInternal(const decltype(m_tracker)::iterator it) EXCLUSIVE_LOCKS_REQUIRED(m_mutex)
+    {
+        m_tracker.erase(it);
+        if (m_tracker.empty()) m_cv.notify_all();
+    }
+public:
+    //! Increase request counter for the associated connection by 1
+    void AddRequest(evhttp_request* req) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
+    {
+        const evhttp_connection* conn{Assert(evhttp_request_get_connection(Assert(req)))};
+        WITH_LOCK(m_mutex, ++m_tracker[conn]);
+    }
+    //! Decrease request counter for the associated connection by 1, remove connection if counter is 0
+    void RemoveRequest(evhttp_request* req) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
+    {
+        const evhttp_connection* conn{Assert(evhttp_request_get_connection(Assert(req)))};
+        LOCK(m_mutex);
+        auto it{m_tracker.find(conn)};
+        if (it != m_tracker.end() && it->second > 0) {
+            if (--(it->second) == 0) RemoveConnectionInternal(it);
+        }
+    }
+    //! Remove a connection entirely
+    void RemoveConnection(const evhttp_connection* conn) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
+    {
+        LOCK(m_mutex);
+        auto it{m_tracker.find(Assert(conn))};
+        if (it != m_tracker.end()) RemoveConnectionInternal(it);
+    }
+    size_t CountActiveConnections() const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
+    {
+        return WITH_LOCK(m_mutex, return m_tracker.size());
+    }
+    //! Wait until there are no more connections with active requests in the tracker
+    void WaitUntilEmpty() const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
+    {
+        WAIT_LOCK(m_mutex, lock);
+        m_cv.wait(lock, [this]() EXCLUSIVE_LOCKS_REQUIRED(m_mutex) { return m_tracker.empty(); });
+    }
+};
 //! Track active requests
-static GlobalMutex g_requests_mutex;
-static std::condition_variable g_requests_cv;
-static std::unordered_set<evhttp_request*> g_requests GUARDED_BY(g_requests_mutex);
+static HTTPRequestTracker g_requests;
 
 /** Check if a network address is allowed to access the HTTP server */
 static bool ClientAllowed(const CNetAddr& netaddr)
@@ -210,22 +262,22 @@ std::string RequestMethodString(HTTPRequest::RequestMethod m)
 /** HTTP request callback */
 static void http_request_cb(struct evhttp_request* req, void* arg)
 {
-    // Track requests and notify when a request is completed.
+    evhttp_connection* conn{evhttp_request_get_connection(req)};
+    // Track active requests
     {
-        WITH_LOCK(g_requests_mutex, g_requests.insert(req));
-        g_requests_cv.notify_all();
+        g_requests.AddRequest(req);
         evhttp_request_set_on_complete_cb(req, [](struct evhttp_request* req, void*) {
-            auto n{WITH_LOCK(g_requests_mutex, return g_requests.erase(req))};
-            assert(n == 1);
-            g_requests_cv.notify_all();
+            g_requests.RemoveRequest(req);
+        }, nullptr);
+        evhttp_connection_set_closecb(conn, [](evhttp_connection* conn, void* arg) {
+            g_requests.RemoveConnection(conn);
         }, nullptr);
     }
 
     // Disable reading to work around a libevent bug, fixed in 2.1.9
     // See https://github.com/libevent/libevent/commit/5ff8eb26371c4dc56f384b2de35bea2d87814779
     // and https://github.com/bitcoin/bitcoin/pull/11593.
     if (event_get_version_number() >= 0x02010600 && event_get_version_number() < 0x02010900) {
-        evhttp_connection* conn = evhttp_request_get_connection(req);
         if (conn) {
             bufferevent* bev = evhttp_connection_get_bufferevent(conn);
             if (bev) {
@@ -473,13 +525,10 @@ void StopHTTPServer()
     }
     boundSockets.clear();
     {
-        WAIT_LOCK(g_requests_mutex, lock);
-        if (!g_requests.empty()) {
-            LogPrint(BCLog::HTTP, "Waiting for %d requests to stop HTTP server\n", g_requests.size());
+        if (const auto n_connections{g_requests.CountActiveConnections()}; n_connections != 0) {
+            LogPrint(BCLog::HTTP, "Waiting for %d connections to stop HTTP server\n", n_connections);
         }
-        g_requests_cv.wait(lock, []() EXCLUSIVE_LOCKS_REQUIRED(g_requests_mutex) {
-            return g_requests.empty();
-        });
+        g_requests.WaitUntilEmpty();
     }
     if (eventHTTP) {
         // Schedule a callback to call evhttp_free in the event base thread, so