From 02e1e4eff6cda0bfc24b455a7c1583394cbff6eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Barbosa?= Date: Tue, 20 Nov 2018 17:52:15 +0000 Subject: [PATCH 1/6] rpc: Add wait argument to stop --- src/rpc/client.cpp | 1 + src/rpc/server.cpp | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 2b99808c0..6f1bfb03d 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -162,6 +162,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "rescanblockchain", 1, "stop_height"}, { "createwallet", 1, "disable_private_keys"}, { "getnodeaddresses", 0, "count"}, + { "stop", 0, "wait" }, }; // clang-format on diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp index c565094a1..865d343eb 100644 --- a/src/rpc/server.cpp +++ b/src/rpc/server.cpp @@ -227,6 +227,9 @@ UniValue help(const JSONRPCRequest& jsonRequest) UniValue stop(const JSONRPCRequest& jsonRequest) { // Accept the deprecated and ignored 'detach' boolean argument + // Also accept the hidden 'wait' integer argument (milliseconds) + // For instance, 'stop 1000' makes the call wait 1 second before returning + // to the client (intended for testing) if (jsonRequest.fHelp || jsonRequest.params.size() > 1) throw std::runtime_error( RPCHelpMan{"stop", @@ -235,6 +238,9 @@ UniValue stop(const JSONRPCRequest& jsonRequest) // Event loop will exit after current HTTP requests have been handled, so // this reply will get back to the client. StartShutdown(); + if (jsonRequest.params[0].isNum()) { + MilliSleep(jsonRequest.params[0].get_int()); + } return "Bitcoin server stopping"; } @@ -264,7 +270,7 @@ static const CRPCCommand vRPCCommands[] = // --------------------- ------------------------ ----------------------- ---------- /* Overall control/query calls */ { "control", "help", &help, {"command"} }, - { "control", "stop", &stop, {} }, + { "control", "stop", &stop, {"wait"} }, { "control", "uptime", &uptime, {} }, }; // clang-format on From 18e968581697078c36a3c3818f8906cf134ccadd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Barbosa?= Date: Wed, 7 Nov 2018 09:08:56 +0000 Subject: [PATCH 2/6] http: Send "Connection: close" header if shutdown is requested Sending the header "Connection: close" makes libevent close persistent connections (implicit with HTTP 1.1) which cleans the event base when shutdown is requested. --- src/httpserver.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 00434169c..342956f7a 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -10,6 +10,7 @@ #include #include #include // For HTTP status codes +#include #include #include @@ -583,6 +584,9 @@ void HTTPRequest::WriteHeader(const std::string& hdr, const std::string& value) void HTTPRequest::WriteReply(int nStatus, const std::string& strReply) { assert(!replySent && req); + if (ShutdownRequested()) { + WriteHeader("Connection", "close"); + } // Send event to main http thread to send reply message struct evbuffer* evb = evhttp_request_get_output_buffer(req); assert(evb); From 6b13580f4e3842c11abd9b8bee7255fb2472b6fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Barbosa?= Date: Wed, 7 Nov 2018 09:10:07 +0000 Subject: [PATCH 3/6] http: Unlisten sockets after all workers quit This (almost) move only ensures the event base loop doesn't exit before HTTP worker threads exit. This way events registered by HTTP workers are processed and not discarded. --- src/httpserver.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 342956f7a..0ab9d966d 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -443,10 +443,6 @@ void InterruptHTTPServer() { LogPrint(BCLog::HTTP, "Interrupting HTTP server\n"); if (eventHTTP) { - // Unlisten sockets - for (evhttp_bound_socket *socket : boundSockets) { - evhttp_del_accept_socket(eventHTTP, socket); - } // Reject requests on current connections evhttp_set_gencb(eventHTTP, http_reject_request_cb, nullptr); } @@ -466,6 +462,12 @@ void StopHTTPServer() delete workQueue; workQueue = nullptr; } + // Unlisten sockets, these are what make the event loop running, which means + // that after this and all connections are closed the event loop will quit. + for (evhttp_bound_socket *socket : boundSockets) { + evhttp_del_accept_socket(eventHTTP, socket); + } + boundSockets.clear(); if (eventBase) { LogPrint(BCLog::HTTP, "Waiting for HTTP event thread to exit\n"); // Exit the event loop as soon as there are no active events. From e98a9eede2fb48ff33a020acc888cbcd83e24bbf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Barbosa?= Date: Wed, 7 Nov 2018 09:11:16 +0000 Subject: [PATCH 4/6] http: Remove unnecessary event_base_loopexit call Let event base loop exit cleanly by processing all active and pending events. The call is no longer necessary because closing persistent connections is now properly handled. --- src/httpserver.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 0ab9d966d..2cc83eecd 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -470,8 +470,6 @@ void StopHTTPServer() boundSockets.clear(); if (eventBase) { LogPrint(BCLog::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 From 8d3f46ec3938e2ba17654fecacd1d2629f9915fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Barbosa?= Date: Wed, 7 Nov 2018 09:13:36 +0000 Subject: [PATCH 5/6] http: Remove timeout to exit event loop Let HTTP connections to timeout due to inactivity. Let all remaning connections finish sending the response and close. --- src/httpserver.cpp | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 2cc83eecd..cb8578927 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -22,7 +22,6 @@ #include #include #include -#include #include #include @@ -422,7 +421,6 @@ bool UpdateHTTPServerLogging(bool enable) { } std::thread threadHTTP; -std::future threadResult; static std::vector g_thread_http_workers; void StartHTTPServer() @@ -430,9 +428,7 @@ void StartHTTPServer() LogPrint(BCLog::HTTP, "Starting HTTP server\n"); int rpcThreads = std::max((long)gArgs.GetArg("-rpcthreads", DEFAULT_HTTP_THREADS), 1L); LogPrintf("HTTP: starting %d worker threads\n", rpcThreads); - std::packaged_task task(ThreadHTTP); - threadResult = task.get_future(); - threadHTTP = std::thread(std::move(task), eventBase); + threadHTTP = std::thread(ThreadHTTP, eventBase); for (int i = 0; i < rpcThreads; i++) { g_thread_http_workers.emplace_back(HTTPWorkQueueRun, workQueue); @@ -470,16 +466,6 @@ void StopHTTPServer() boundSockets.clear(); if (eventBase) { LogPrint(BCLog::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 (threadResult.valid() && threadResult.wait_for(std::chrono::milliseconds(2000)) == std::future_status::timeout) { - LogPrintf("HTTP event loop did not exit within allotted time, sending loopbreak\n"); - event_base_loopbreak(eventBase); - } threadHTTP.join(); } if (eventHTTP) { From 28479f926f21f2a91bec5a06671c60e5b0c55532 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Barbosa?= Date: Tue, 20 Nov 2018 17:59:07 +0000 Subject: [PATCH 6/6] qa: Test bitcond shutdown --- test/functional/feature_shutdown.py | 28 +++++++++++++++++++ .../test_framework/test_framework.py | 8 +++--- test/functional/test_framework/test_node.py | 4 +-- test/functional/test_runner.py | 1 + 4 files changed, 35 insertions(+), 6 deletions(-) create mode 100755 test/functional/feature_shutdown.py diff --git a/test/functional/feature_shutdown.py b/test/functional/feature_shutdown.py new file mode 100755 index 000000000..b633fabb1 --- /dev/null +++ b/test/functional/feature_shutdown.py @@ -0,0 +1,28 @@ +#!/usr/bin/env python3 +# Copyright (c) 2018 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. +"""Test bitcoind shutdown.""" + +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import assert_equal, get_rpc_proxy +from threading import Thread + +def test_long_call(node): + block = node.waitfornewblock() + assert_equal(block['height'], 0) + +class ShutdownTest(BitcoinTestFramework): + + def set_test_params(self): + self.setup_clean_chain = True + self.num_nodes = 1 + + def run_test(self): + node = get_rpc_proxy(self.nodes[0].url, 1, timeout=600, coveragedir=self.nodes[0].coverage_dir) + Thread(target=test_long_call, args=(node,)).start() + # wait 1 second to ensure event loop waits for current connections to close + self.stop_node(0, wait=1000) + +if __name__ == '__main__': + ShutdownTest().main() diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index 44fc185e6..ded070540 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -325,16 +325,16 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): for node in self.nodes: coverage.write_all_rpc_commands(self.options.coveragedir, node.rpc) - def stop_node(self, i, expected_stderr=''): + def stop_node(self, i, expected_stderr='', wait=0): """Stop a bitcoind test node""" - self.nodes[i].stop_node(expected_stderr) + self.nodes[i].stop_node(expected_stderr, wait=wait) self.nodes[i].wait_until_stopped() - def stop_nodes(self): + def stop_nodes(self, wait=0): """Stop multiple bitcoind test nodes""" for node in self.nodes: # Issue RPC to stop nodes - node.stop_node() + node.stop_node(wait=wait) for node in self.nodes: # Wait for nodes to stop diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 9dcc0e6d0..be9053552 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -228,13 +228,13 @@ class TestNode(): wallet_path = "wallet/{}".format(urllib.parse.quote(wallet_name)) return self.rpc / wallet_path - def stop_node(self, expected_stderr=''): + def stop_node(self, expected_stderr='', wait=0): """Stop the node.""" if not self.running: return self.log.debug("Stopping node") try: - self.stop() + self.stop(wait=wait) except http.client.CannotSendRequest: self.log.exception("Unable to stop node.") diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index da55a3a15..2dc3abcfe 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -185,6 +185,7 @@ BASE_SCRIPTS = [ 'feature_config_args.py', 'rpc_help.py', 'feature_help.py', + 'feature_shutdown.py', # Don't append tests at the end to avoid merge conflicts # Put them in a random line within the section that fits their approximate run-time ]