From 49bf74010439e50dbbf39abb8ba4cbade38521eb Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 6 Mar 2023 19:29:59 -0500 Subject: [PATCH 1/2] Merge bitcoin/bitcoin#26742: http: Track active requests and wait for last to finish - 2nd attempt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 60978c8080ec13ff4571c8a89e742517b2aca692 test: Reduce extended timeout on abortnode test (Fabian Jahr) 660bdbf785a32024f0694915fa043968a0afb573 http: Release server before waiting for event base loop exit (João Barbosa) 8c6d007c80dc3fec5ce6c0196381444a5ed7e424 http: Track active requests and wait for last to finish (João Barbosa) Pull request description: This revives #19420. Since promag is not so active at the moment, I can support this to finally get it merged. The PR is rebased and comments by jonatack have been addressed. Once this is merged, I will also reopen #19434. ACKs for top commit: achow101: ACK 60978c8080ec13ff4571c8a89e742517b2aca692 stickies-v: re-ACK [60978c8](https://github.com/bitcoin/bitcoin/commit/60978c8080ec13ff4571c8a89e742517b2aca692) hebasto: ACK 60978c8080ec13ff4571c8a89e742517b2aca692 Tree-SHA512: eef0fe1081e9331b95cfafc71d82f2398abd1d3439dac5b2fa5c6d9c0a3f63ef19adde1c38c88d3b4e7fb41ce7c097943f1815c10e33d165918ccbdec512fe1c --- src/httpserver.cpp | 39 ++++++++++++++++++++++++---- test/functional/feature_abortnode.py | 3 +-- 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 5b062d65d97c..56bb6a440c66 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -23,12 +23,14 @@ #include #include #include +#include #include #include #include #include +#include #include #include #include @@ -161,6 +163,10 @@ static GlobalMutex g_httppathhandlers_mutex; static std::vector pathHandlers GUARDED_BY(g_httppathhandlers_mutex); //! Bound listening sockets static std::vector boundSockets; +//! 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); /** Check if a network address is allowed to access the HTTP server */ static bool ClientAllowed(const CNetAddr& netaddr) @@ -217,6 +223,17 @@ 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. + { + WITH_LOCK(g_requests_mutex, g_requests.insert(req)); + g_requests_cv.notify_all(); + 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(); + }, 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. @@ -496,15 +513,27 @@ void StopHTTPServer() evhttp_del_accept_socket(eventHTTP, socket); } boundSockets.clear(); - if (eventBase) { - LogPrint(BCLog::HTTP, "Waiting for HTTP event thread to exit\n"); - if (g_thread_http.joinable()) g_thread_http.join(); + { + 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()); + } + g_requests_cv.wait(lock, []() EXCLUSIVE_LOCKS_REQUIRED(g_requests_mutex) { + return g_requests.empty(); + }); } if (eventHTTP) { - evhttp_free(eventHTTP); - eventHTTP = nullptr; + // Schedule a callback to call evhttp_free in the event base thread, so + // that evhttp_free does not need to be called again after the handling + // of unfinished request connections that follows. + event_base_once(eventBase, -1, EV_TIMEOUT, [](evutil_socket_t, short, void*) { + evhttp_free(eventHTTP); + eventHTTP = nullptr; + }, nullptr, nullptr); } if (eventBase) { + LogPrint(BCLog::HTTP, "Waiting for HTTP event thread to exit\n"); + if (g_thread_http.joinable()) g_thread_http.join(); event_base_free(eventBase); eventBase = nullptr; } diff --git a/test/functional/feature_abortnode.py b/test/functional/feature_abortnode.py index 19431d2baaf8..3c4e03bb05ee 100755 --- a/test/functional/feature_abortnode.py +++ b/test/functional/feature_abortnode.py @@ -19,7 +19,6 @@ class AbortNodeTest(BitcoinTestFramework): def set_test_params(self): self.setup_clean_chain = True self.num_nodes = 2 - self.rpc_timeout = 240 def setup_network(self): self.setup_nodes() @@ -41,7 +40,7 @@ def run_test(self): # Check that node0 aborted self.log.info("Waiting for crash") - self.nodes[0].wait_until_stopped(timeout=200) + self.nodes[0].wait_until_stopped(timeout=5) self.log.info("Node crashed - now verifying restart fails") self.nodes[0].assert_start_raises_init_error() From 4998a3f128677d0857fda6854d383decce7cfe2d Mon Sep 17 00:00:00 2001 From: fanquake Date: Wed, 4 Oct 2023 09:54:13 +0100 Subject: [PATCH 2/2] Merge bitcoin/bitcoin#28551: http: bugfix: allow server shutdown in case of remote client disconnection 68f23f57d77bc172ed39ecdd4d2d5cd5e13cf483 http: bugfix: track closed connection (stickies-v) 084d0372311e658a486622f720d2b827d8416591 http: log connection instead of request count (stickies-v) 41f9027813f849a9fd6a1479bbb74b9037990c3c 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 https://github.com/bitcoin/bitcoin/issues/27722#issuecomment-1559453982 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](https://github.com/bitcoin/bitcoin/pull/27909#discussion_r1265614783) 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](https://github.com/bitcoin/bitcoin/pull/19420#issue-648067169) 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 68f23f57d77bc172ed39ecdd4d2d5cd5e13cf483 theStack: ACK 68f23f57d77bc172ed39ecdd4d2d5cd5e13cf483 Tree-SHA512: dfa711ff55ec75ba44d73e9e6fac16b0be25cf3c20868c2145a844a7878ad9fc6998d9ff62d72f3a210bfa411ef03d3757b73d68a7c22926e874c421e51444d6 --- src/httpserver.cpp | 83 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 66 insertions(+), 17 deletions(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 56bb6a440c66..fc0132133ff3 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -14,6 +14,7 @@ #include // For HTTP status codes #include #include +#include #include #include #include @@ -23,7 +24,7 @@ #include #include #include -#include +#include #include @@ -163,10 +164,61 @@ 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 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) @@ -223,14 +275,15 @@ 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); } @@ -238,7 +291,6 @@ static void http_request_cb(struct evhttp_request* req, void* arg) // 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) { @@ -514,13 +566,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