From 5e0c22135600fe36811da3b78216efc61ba765fb Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Thu, 24 Sep 2015 17:29:22 +0200 Subject: 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 #6717. --- src/httpserver.cpp | 27 +++++++++++++++++++++------ src/rpcserver.cpp | 3 ++- 2 files changed, 23 insertions(+), 7 deletions(-) (limited to 'src') diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 600e57b7cc..e967d3a4a8 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 */ @@ -410,8 +419,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 b831d3d3b2..dbee61efc8 100644 --- a/src/rpcserver.cpp +++ b/src/rpcserver.cpp @@ -243,7 +243,8 @@ UniValue stop(const UniValue& params, bool fHelp) throw runtime_error( "stop\n" "\nStop Bitcoin 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 "Bitcoin server stopping"; } -- cgit v1.2.3 From de9de2de361ab1355b976f17371d73e36fe3bf56 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Fri, 25 Sep 2015 13:49:08 +0200 Subject: 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". --- src/httpserver.cpp | 40 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 37 insertions(+), 3 deletions(-) (limited to 'src') diff --git a/src/httpserver.cpp b/src/httpserver.cpp index e967d3a4a8..9f079aedfe 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() @@ -434,7 +464,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; -- cgit v1.2.3 From ec908d5f7aa9ad7e3487018e06a24cb6449cc58b Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Fri, 25 Sep 2015 15:35:37 +0200 Subject: 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. --- src/httpserver.cpp | 7 +++++++ 1 file changed, 7 insertions(+) (limited to 'src') diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 9f079aedfe..0a7f903e9f 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -457,6 +457,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(); } -- cgit v1.2.3