From 41f9027813f849a9fd6a1479bbb74b9037990c3c Mon Sep 17 00:00:00 2001 From: stickies-v Date: Fri, 29 Sep 2023 15:24:03 +0100 Subject: [PATCH 1/3] http: refactor: use encapsulated HTTPRequestTracker Introduces and uses a HTTPRequestTracker class to keep track of how many HTTP requests are currently active, so we don't stop the server before they're all handled. This has two purposes: 1. In a next commit, allows us to untrack all requests associated with a connection without running into lifetime issues of the connection living longer than the request (see https://github.com/bitcoin/bitcoin/pull/27909#discussion_r1265614783) 2. Improve encapsulation by making the mutex and cv internal members, and exposing just the WaitUntilEmpty() method that can be safely used. --- src/httpserver.cpp | 86 +++++++++++++++++++++++++++++++++++++--------- 1 file changed, 70 insertions(+), 16 deletions(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index a83f4421d7..3ad329a669 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -17,6 +17,7 @@ #include // For HTTP status codes #include #include +#include #include #include #include @@ -26,9 +27,10 @@ #include #include #include +#include #include #include -#include +#include #include #include @@ -149,10 +151,68 @@ static GlobalMutex g_httppathhandlers_mutex; static std::vector pathHandlers GUARDED_BY(g_httppathhandlers_mutex); //! Bound listening sockets static std::vector 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 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 CountActiveRequests() const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) + { + LOCK(m_mutex); + return std::accumulate(m_tracker.begin(), m_tracker.end(), size_t(0), + [](size_t acc_count, const auto& pair) { return acc_count + pair.second; }); + } + 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 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,14 +270,11 @@ 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. + // 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); } @@ -473,13 +530,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 (g_requests.CountActiveConnections() != 0) { + LogPrint(BCLog::HTTP, "Waiting for %d requests to stop HTTP server\n", g_requests.CountActiveRequests()); } - 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 From 084d0372311e658a486622f720d2b827d8416591 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Fri, 29 Sep 2023 15:38:51 +0100 Subject: [PATCH 2/3] http: log connection instead of request count There is no significant benefit in logging the request count instead of the connection count. Reduces amount of code and computational complexity. --- src/httpserver.cpp | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 3ad329a669..995c446b58 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -27,7 +27,6 @@ #include #include #include -#include #include #include #include @@ -193,13 +192,6 @@ public: auto it{m_tracker.find(Assert(conn))}; if (it != m_tracker.end()) RemoveConnectionInternal(it); } - - size_t CountActiveRequests() const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) - { - LOCK(m_mutex); - return std::accumulate(m_tracker.begin(), m_tracker.end(), size_t(0), - [](size_t acc_count, const auto& pair) { return acc_count + pair.second; }); - } size_t CountActiveConnections() const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) { return WITH_LOCK(m_mutex, return m_tracker.size()); @@ -530,8 +522,8 @@ void StopHTTPServer() } boundSockets.clear(); { - if (g_requests.CountActiveConnections() != 0) { - LogPrint(BCLog::HTTP, "Waiting for %d requests to stop HTTP server\n", g_requests.CountActiveRequests()); + 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.WaitUntilEmpty(); } From 68f23f57d77bc172ed39ecdd4d2d5cd5e13cf483 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Fri, 29 Sep 2023 15:24:14 +0100 Subject: [PATCH 3/3] http: bugfix: track closed connection It is possible that the client disconnects before the request is handled. In those cases, evhttp_request_set_on_complete_cb is never called, which means that on shutdown the server we'll keep waiting endlessly. By adding evhttp_connection_set_closecb, libevent automatically cleans up those dead connections at latest when we shutdown, and depending on the libevent version already at the moment of remote client disconnect. In both cases, the bug is fixed. --- src/httpserver.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 995c446b58..069511563c 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -262,19 +262,22 @@ std::string RequestMethodString(HTTPRequest::RequestMethod m) /** HTTP request callback */ static void http_request_cb(struct evhttp_request* req, void* arg) { + evhttp_connection* conn{evhttp_request_get_connection(req)}; // Track active requests { g_requests.AddRequest(req); evhttp_request_set_on_complete_cb(req, [](struct evhttp_request* req, void*) { 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) {