Merge bitcoin/bitcoin#26742: http: Track active requests and wait for last to finish - 2nd attempt

60978c8 test: Reduce extended timeout on abortnode test (Fabian Jahr)
660bdbf http: Release server before waiting for event base loop exit (João Barbosa)
8c6d007 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 60978c8
  stickies-v:
    re-ACK [60978c8](bitcoin/bitcoin@60978c8)
  hebasto:
    ACK 60978c8

Tree-SHA512: eef0fe1081e9331b95cfafc71d82f2398abd1d3439dac5b2fa5c6d9c0a3f63ef19adde1c38c88d3b4e7fb41ce7c097943f1815c10e33d165918ccbdec512fe1c

Author: achow101

src/httpserver.cpp

@@ -21,19 +21,22 @@
 #include <util/threadnames.h>
 #include <util/translation.h>
 
+#include <condition_variable>
 #include <cstdio>
 #include <cstdlib>
 #include <deque>
 #include <memory>
 #include <optional>
 #include <string>
+#include <unordered_set>
 
 #include <sys/types.h>
 #include <sys/stat.h>
 
 #include <event2/buffer.h>
 #include <event2/bufferevent.h>
 #include <event2/http.h>
+#include <event2/http_struct.h>
 #include <event2/keyvalq_struct.h>
 #include <event2/thread.h>
 #include <event2/util.h>
@@ -146,6 +149,10 @@ 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;
+//! 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);
 
 /** Check if a network address is allowed to access the HTTP server */
 static bool ClientAllowed(const CNetAddr& netaddr)
@@ -207,6 +214,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.2.0.
     if (event_get_version_number() >= 0x02010600 && event_get_version_number() < 0x02020001) {
         evhttp_connection* conn = evhttp_request_get_connection(req);
@@ -458,15 +476,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;
     }

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()