From c0fe29370a259dd3616226a4b6d27dbe8fd7a6e1 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Thu, 24 Sep 2015 17:29:22 +0200 Subject: [PATCH 1/6] Make HTTP server shutdown more graceful Shutting down the HTTP server currently breaks off all current requests. This can create a race condition with RPC `stop` command, where the calling process never receives confirmation. This change removes the listening sockets on shutdown so that no new requests can come in, but no longer breaks off requests in progress. Meant to fix bitcoin/#6717. Zcash: cherry-picked from commit 5e0c22135600fe36811da3b78216efc61ba765fb --- src/httpserver.cpp | 27 +++++++++++++++++++++------ src/rpcserver.cpp | 3 ++- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 4215a0f26..8acce2f0f 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -155,6 +155,8 @@ static std::vector rpc_allow_subnets; static WorkQueue* workQueue = 0; //! Handlers for (sub)paths std::vector pathHandlers; +//! Bound listening sockets +std::vector boundSockets; /** Check if a network address is allowed to access the HTTP server */ static bool ClientAllowed(const CNetAddr& netaddr) @@ -264,6 +266,13 @@ static void http_request_cb(struct evhttp_request* req, void* arg) } } +/** Callback to reject HTTP requests after shutdown. */ +static void http_reject_request_cb(struct evhttp_request* req, void*) +{ + LogPrint("http", "Rejecting request while shutting down\n"); + evhttp_send_error(req, HTTP_SERVUNAVAIL, NULL); +} + /** Event dispatcher thread */ static void ThreadHTTP(struct event_base* base, struct evhttp* http) { @@ -278,7 +287,6 @@ static void ThreadHTTP(struct event_base* base, struct evhttp* http) static bool HTTPBindAddresses(struct evhttp* http) { int defaultPort = GetArg("-rpcport", BaseParams().RPCPort()); - int nBound = 0; std::vector > endpoints; // Determine what addresses to bind to @@ -304,13 +312,14 @@ static bool HTTPBindAddresses(struct evhttp* http) // Bind addresses for (std::vector >::iterator i = endpoints.begin(); i != endpoints.end(); ++i) { LogPrint("http", "Binding RPC on address %s port %i\n", i->first, i->second); - if (evhttp_bind_socket(http, i->first.empty() ? NULL : i->first.c_str(), i->second) == 0) { - nBound += 1; + evhttp_bound_socket *bind_handle = evhttp_bind_socket_with_handle(http, i->first.empty() ? NULL : i->first.c_str(), i->second); + if (bind_handle) { + boundSockets.push_back(bind_handle); } else { LogPrintf("Binding RPC on address %s port %i failed.\n", i->first, i->second); } } - return nBound > 0; + return !boundSockets.empty(); } /** Simple wrapper to set thread name and run work queue */ @@ -414,8 +423,14 @@ bool StartHTTPServer(boost::thread_group& threadGroup) void InterruptHTTPServer() { LogPrint("http", "Interrupting HTTP server\n"); - if (eventBase) - event_base_loopbreak(eventBase); + if (eventHTTP) { + // Unlisten sockets + BOOST_FOREACH (evhttp_bound_socket *socket, boundSockets) { + evhttp_del_accept_socket(eventHTTP, socket); + } + // Reject requests on current connections + evhttp_set_gencb(eventHTTP, http_reject_request_cb, NULL); + } if (workQueue) workQueue->Interrupt(); } diff --git a/src/rpcserver.cpp b/src/rpcserver.cpp index c2c76a1e2..a01366f74 100644 --- a/src/rpcserver.cpp +++ b/src/rpcserver.cpp @@ -246,7 +246,8 @@ UniValue stop(const UniValue& params, bool fHelp) throw runtime_error( "stop\n" "\nStop Zcash server."); - // Shutdown will take long enough that the response should get back + // Event loop will exit after current HTTP requests have been handled, so + // this reply will get back to the client. StartShutdown(); return "Zcash server stopping"; } From dbf7057f7a61f6971f903c0e7fa9f3133ab22dbe Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Fri, 25 Sep 2015 13:49:08 +0200 Subject: [PATCH 2/6] http: Wait for worker threads to exit Add a WaitExit() call to http's WorkQueue to make it delete the work queue only when all worker threads stopped. This fixes a problem that was reproducable by pressing Ctrl-C during AppInit2: ``` /usr/include/boost/thread/pthread/condition_variable_fwd.hpp:81: boost::condition_variable::~condition_variable(): Assertion `!ret' failed. /usr/include/boost/thread/pthread/mutex.hpp:108: boost::mutex::~mutex(): Assertion `!posix::pthread_mutex_destroy(&m)' failed. ``` I was assuming that `threadGroup->join_all();` would always have been called when entering the Shutdown(). However this is not the case in bitcoind's AppInit2-non-zero-exit case "was left out intentionally here". Zcash: cherry-picked from commit de9de2de361ab1355b976f17371d73e36fe3bf56 Fixes #2334 and #2214. --- src/httpserver.cpp | 40 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 37 insertions(+), 3 deletions(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 8acce2f0f..6cf09974e 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -72,13 +72,35 @@ private: std::deque queue; bool running; size_t maxDepth; + int numThreads; + + /** RAII object to keep track of number of running worker threads */ + class ThreadCounter + { + public: + WorkQueue &wq; + ThreadCounter(WorkQueue &w): wq(w) + { + boost::lock_guard lock(wq.cs); + wq.numThreads += 1; + } + ~ThreadCounter() + { + boost::lock_guard lock(wq.cs); + wq.numThreads -= 1; + wq.cond.notify_all(); + } + }; public: WorkQueue(size_t maxDepth) : running(true), - maxDepth(maxDepth) + maxDepth(maxDepth), + numThreads(0) { } - /* Precondition: worker threads have all stopped */ + /*( Precondition: worker threads have all stopped + * (call WaitExit) + */ ~WorkQueue() { while (!queue.empty()) { @@ -100,6 +122,7 @@ public: /** Thread function */ void Run() { + ThreadCounter count(*this); while (running) { WorkItem* i = 0; { @@ -122,6 +145,13 @@ public: running = false; cond.notify_all(); } + /** Wait for worker threads to exit */ + void WaitExit() + { + boost::unique_lock lock(cs); + while (numThreads > 0) + cond.wait(lock); + } /** Return current depth of queue */ size_t Depth() @@ -438,7 +468,11 @@ void InterruptHTTPServer() void StopHTTPServer() { LogPrint("http", "Stopping HTTP server\n"); - delete workQueue; + if (workQueue) { + LogPrint("http", "Waiting for HTTP worker threads to exit\n"); + workQueue->WaitExit(); + delete workQueue; + } if (eventHTTP) { evhttp_free(eventHTTP); eventHTTP = 0; From 2abe8ef721ea7bdff5d908f5ee3a9bc34d54a1c8 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Fri, 25 Sep 2015 15:35:37 +0200 Subject: [PATCH 3/6] http: Force-exit event loop after predefined time This makes sure that the event loop eventually terminates, even if an event (like an open timeout, or a hanging connection) happens to be holding it up. Zcash: cherry-picked from commit ec908d5f7aa9ad7e3487018e06a24cb6449cc58b --- src/httpserver.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 6cf09974e..f2da2de13 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -461,6 +461,13 @@ void InterruptHTTPServer() // Reject requests on current connections evhttp_set_gencb(eventHTTP, http_reject_request_cb, NULL); } + if (eventBase) { + // Force-exit event loop after predefined time + struct timeval tv; + tv.tv_sec = 10; + tv.tv_usec = 0; + event_base_loopexit(eventBase, &tv); + } if (workQueue) workQueue->Interrupt(); } From c7f77e28f0e4e8cb4f5ca6ba3877ab75bb983930 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Wed, 11 Nov 2015 17:34:10 +0100 Subject: [PATCH 4/6] http: speed up shutdown This continues/fixes #6719. `event_base_loopbreak` was not doing what I expected it to, at least in libevent 2.0.21. What I expected was that it sets a timeout, given that no other pending events it would exit in N seconds. However, what it does was delay the event loop exit with 10 seconds, even if nothing is pending. Solve it in a different way: give the event loop thread time to exit out of itself, and if it doesn't, send loopbreak. This speeds up the RPC tests a lot, each exit incurred a 10 second overhead, with this change there should be no shutdown overhead in the common case and up to two seconds if the event loop is blocking. As a bonus this breaks dependency on boost::thread_group, as the HTTP server minds its own offspring. Zcash: cherry-picked from commit a264c32e3321ae909ca59cb8ce8bf5d812dbc4e1 --- src/httpserver.cpp | 30 ++++++++++++++++++++---------- src/httpserver.h | 2 +- src/init.cpp | 2 +- 3 files changed, 22 insertions(+), 12 deletions(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index f2da2de13..a7ca741cc 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -438,15 +438,17 @@ bool InitHTTPServer() return true; } -bool StartHTTPServer(boost::thread_group& threadGroup) +boost::thread threadHTTP; + +bool StartHTTPServer() { LogPrint("http", "Starting HTTP server\n"); int rpcThreads = std::max((long)GetArg("-rpcthreads", DEFAULT_HTTP_THREADS), 1L); LogPrintf("HTTP: starting %d worker threads\n", rpcThreads); - threadGroup.create_thread(boost::bind(&ThreadHTTP, eventBase, eventHTTP)); + threadHTTP = boost::thread(boost::bind(&ThreadHTTP, eventBase, eventHTTP)); for (int i = 0; i < rpcThreads; i++) - threadGroup.create_thread(boost::bind(&HTTPWorkQueueRun, workQueue)); + boost::thread(boost::bind(&HTTPWorkQueueRun, workQueue)); return true; } @@ -461,13 +463,6 @@ void InterruptHTTPServer() // Reject requests on current connections evhttp_set_gencb(eventHTTP, http_reject_request_cb, NULL); } - if (eventBase) { - // Force-exit event loop after predefined time - struct timeval tv; - tv.tv_sec = 10; - tv.tv_usec = 0; - event_base_loopexit(eventBase, &tv); - } if (workQueue) workQueue->Interrupt(); } @@ -480,6 +475,20 @@ void StopHTTPServer() workQueue->WaitExit(); delete workQueue; } + if (eventBase) { + LogPrint("http", "Waiting for HTTP event thread to exit\n"); + // Give event loop a few seconds to exit (to send back last RPC responses), then break it + // Before this was solved with event_base_loopexit, but that didn't work as expected in + // at least libevent 2.0.21 and always introduced a delay. In libevent + // master that appears to be solved, so in the future that solution + // could be used again (if desirable). + // (see discussion in https://github.com/bitcoin/bitcoin/pull/6990) + if (!threadHTTP.try_join_for(boost::chrono::milliseconds(2000))) { + LogPrintf("HTTP event loop did not exit within allotted time, sending loopbreak\n"); + event_base_loopbreak(eventBase); + threadHTTP.join(); + } + } if (eventHTTP) { evhttp_free(eventHTTP); eventHTTP = 0; @@ -488,6 +497,7 @@ void StopHTTPServer() event_base_free(eventBase); eventBase = 0; } + LogPrint("http", "Stopped HTTP server\n"); } struct event_base* EventBase() diff --git a/src/httpserver.h b/src/httpserver.h index 347e7c3ab..93fb5d8d6 100644 --- a/src/httpserver.h +++ b/src/httpserver.h @@ -28,7 +28,7 @@ bool InitHTTPServer(); * This is separate from InitHTTPServer to give users race-condition-free time * to register their handlers between InitHTTPServer and StartHTTPServer. */ -bool StartHTTPServer(boost::thread_group& threadGroup); +bool StartHTTPServer(); /** Interrupt HTTP server threads */ void InterruptHTTPServer(); /** Stop HTTP server */ diff --git a/src/init.cpp b/src/init.cpp index cb91a1c1d..4f1618e28 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -712,7 +712,7 @@ bool AppInitServers(boost::thread_group& threadGroup) return false; if (GetBoolArg("-rest", false) && !StartREST()) return false; - if (!StartHTTPServer(threadGroup)) + if (!StartHTTPServer()) return false; return true; } From c98b91b7c5fd9779c67529aa9c7f99609075357f Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Thu, 28 Jul 2016 18:21:00 -0400 Subject: [PATCH 5/6] httpserver: explicitly detach worker threads When using std::thread in place of boost::thread, letting the threads destruct results in a std::terminate. According to the docs, the same thing should be be happening in later boost versions: http://www.boost.org/doc/libs/1_55_0/doc/html/thread/thread_management.html#thread.thread_management.thread.destructor I'm unsure why this hasn't blown up already, but explicitly detaching can't hurt. Zcash: cherry-picked from commit d3773ca9aeb0d2f12dc0c5a0726778050c8cb455 This fixes #2554 (zcash-cli stop during getblocktemplate long poll causes 'Assertion `!pthread_mutex_unlock(&m)' failed.') --- src/httpserver.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index a7ca741cc..e2a6af6ad 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -447,8 +447,10 @@ bool StartHTTPServer() LogPrintf("HTTP: starting %d worker threads\n", rpcThreads); threadHTTP = boost::thread(boost::bind(&ThreadHTTP, eventBase, eventHTTP)); - for (int i = 0; i < rpcThreads; i++) - boost::thread(boost::bind(&HTTPWorkQueueRun, workQueue)); + for (int i = 0; i < rpcThreads; i++) { + boost::thread rpc_worker(HTTPWorkQueueRun, workQueue); + rpc_worker.detach(); + } return true; } From d3c8109b410e3538c6d85c13c01b8c7df8142980 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Barbosa?= Date: Tue, 8 Aug 2017 13:30:04 +0100 Subject: [PATCH 6/6] Improve shutdown process Zcash: cherry-picked from commit 793667af1c31835e0eefcdd283930bb89cfeda8f --- src/httpserver.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index e2a6af6ad..a1f27fc0d 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -479,6 +479,8 @@ void StopHTTPServer() } if (eventBase) { LogPrint("http", "Waiting for HTTP event thread to exit\n"); + // Exit the event loop as soon as there are no active events. + event_base_loopexit(eventBase, nullptr); // Give event loop a few seconds to exit (to send back last RPC responses), then break it // Before this was solved with event_base_loopexit, but that didn't work as expected in // at least libevent 2.0.21 and always introduced a delay. In libevent