Fixes #11777. Replaces #13492.
This PR tracks current HTTP connections in order to disable reading on shutdown while allowing all remaining events to process. This allows the event loop to exit gracefully.
This happens in the test framework because the RPC client never closes the connection to the node — the same connection is used for all RPC to that node — even when stop is called.
This is an alternative to #13492 that doesn’t require bumping the minimum libevent2 version. This was submitted in a different branch to keep the other version available.
To test apply
0diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp
1index 10040b125..57dad383c 100644
2--- a/src/rpc/server.cpp
3+++ b/src/rpc/server.cpp
4@@ -236,6 +236,7 @@ UniValue stop(const JSONRPCRequest& jsonRequest)
5 // Event loop will exit after current HTTP requests have been handled, so
6 // this reply will get back to the client.
7 StartShutdown();
8+ MilliSleep(2000);
9 return "Bitcoin server stopping";
10 }
and then
0bitcoind -regtest
1bitcoin-cli -regtest stop
event_base_loopexit is still requied though.
Going to close and reopen, to get a fresh travis run.
Previous run: https://travis-ci.org/bitcoin/bitcoin/builds/394134101
New run: https://travis-ci.org/bitcoin/bitcoin/builds/413803664
Hmm.. Still takes longer than on master, I think
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
MilliSleep(1000) at https://github.com/bitcoin/bitcoin/blob/3c8d1ae15352d5c92d5903536c8fe07f771e97a0/src/rpc/server.cpp#L238-L239
475@@ -464,21 +476,23 @@ void StopHTTPServer()
476 delete workQueue;
477 workQueue = nullptr;
478 }
479+ // Disable reading on current connections
480+ for (evhttp_connection* conn : g_http_connections) {
g_http_connections from multiple threads here? (the HTTP event thread as well as the thread in which StopHTTPServer is called)
if so, it needs a mutex
143@@ -144,6 +144,9 @@ struct evhttp* eventHTTP = nullptr;
144 static std::vector<CSubNet> rpc_allow_subnets;
145 //! Work queue for handling longer requests off the event loop thread
146 static WorkQueue<HTTPClosure>* workQueue = nullptr;
147+//! Set of current HTTP connections
148+static std::set<evhttp_connection*> g_http_connections;
static std::unordered_set<evhttp_connection*> g_http_connections GUARDED_BY(g_http_connections_cs);
143@@ -144,6 +144,9 @@ struct evhttp* eventHTTP = nullptr;
144 static std::vector<CSubNet> rpc_allow_subnets;
145 //! Work queue for handling longer requests off the event loop thread
146 static WorkQueue<HTTPClosure>* workQueue = nullptr;
147+//! Set of current HTTP connections
148+static std::set<evhttp_connection*> g_http_connections;
149+static std::mutex g_http_connections_cs;
Mutex and LOCK for easier debugging.
13@@ -14,6 +14,7 @@
14 #include <ui_interface.h>
15
16 #include <memory>
17+#include <set>
499- event_base_loopbreak(eventBase);
500- }
501+ event_base_loopbreak(eventBase);
502 threadHTTP.join();
503+ // Process remaining events.
504+ event_base_dispatch(eventBase);
The changes in this if (eventBase) section don’t seem directly related to the other changes in this PR. If I understand the eventlib documentation correctly, the behavior of the new code is identical to previous code except there is no longer a 2 second timeout.
So is the point of this change just cleanup? It seems like it might be safer to keep the 2 second timeout.
This is necessary in order to have a clean event loop quit, which means all active events processed and there’s no pending events. In some cases (long response/slow client) the timeout can cause errors on the client side because the client receives an unexpected connection close.
Ideally it would be enough:
0// Wait the event loop to quit
1threadHTTP.join();
but in windows only this works (sorry I don’t know why):
0// Wait the event loop to quit in *this* thread
1event_base_loopbreak(eventBase);
2threadHTTP.join();
3// Process remaining events.
4event_base_dispatch(eventBase);
This is necessary in order to have a clean event loop quit
It seems like only the part of this change needed to exit cleanly is removing the racy event_base_loopexit added in #11006. Aside from that, it looks like the previous code would also exit cleanly, unless it took longer than 2 seconds.
If the 2 second timeout causes problems, maybe it could be increased, but it doesn’t seem like seem like a great idea to me to delay shutdown forever writing data to clients. It also seems nicer that the previous code didn’t need an inexplicable windows workaround.
On the other hand, if we really do want to wait forever here instead of timing out, it would be good to delete the std::future/std::packaged_task code added in 755aa05174e06effd758eeb78c5af9fb465e9611, since now it is sitting there unused.
474@@ -457,21 +475,21 @@ void StopHTTPServer()
475 delete workQueue;
476 workQueue = nullptr;
477 }
478+ {
479+ LogPrint(BCLog::HTTP, "Disabling reading on current connections\n");
480+ LOCK(g_http_connections_cs);
481+ for (evhttp_connection* conn : g_http_connections) {
482+ bufferevent* bev = evhttp_connection_get_bufferevent(conn);
483+ if (bev) bufferevent_disable(bev, EV_READ);
bev just defensive here? Would it ever be expected to be null? It’d be good to have a sentence or shorter comment here to suggest what this does.
225+static void http_request_cb(struct evhttp_request* req, void* /* arg */)
226 {
227+ {
228+ LOCK(g_http_connections_cs);
229+ evhttp_connection* conn = evhttp_request_get_connection(req);
230+ if (g_http_connections.insert(conn).second) {
Won’t this be buggy if a client opens a connection but doesn’t send a complete request before the server starts to shut down? In that case this code won’t get called, so the connection pointer will never be added to g_http_connections, so event_base_dispatch would never return, and bitcoind would hang forever on shutdown.
I don’t see a way to be notified directly about new connections, so maybe there is nothing we can do about this, but it seems worth commenting about if there isn’t a better approach or workaround.
@ryanofsky tried the following:
0bitcoind -regtest
1nc localhost 18443
2bitcoin-cli -regtest stop
at this point the server waits until it timeouts the request (by default 30 seconds).
If a request is sent before the timeout the result is:
0POST / HTTP/1.1
1Authorization: Basic ...
2Content-Type: application/json
3Content-Length: 44
4
5{"jsonrpc": "2.0","method":"help","id":123}
6HTTP/1.1 503 Service Unavailable
7Content-Type: text/html
8Connection: close
9Date: Mon, 05 Nov 2018 11:58:34 GMT
10Content-Length: 110
11
12<HTML><HEAD>
13<TITLE>503 Service Unavailable</TITLE>
14</HEAD><BODY>
15<H1>Service Unavailable</H1>
16</BODY></HTML>
If we consider that stop RPC triggers a graceful shutdown then this LGTM.
144@@ -144,6 +145,9 @@ struct evhttp* eventHTTP = nullptr;
145 static std::vector<CSubNet> rpc_allow_subnets;
146 //! Work queue for handling longer requests off the event loop thread
147 static WorkQueue<HTTPClosure>* workQueue = nullptr;
148+//! Set of current HTTP connections
149+static Mutex g_http_connections_cs;
_mutex instead of _cs. I think people have generally moved on to calling critical sections mutexes.
474@@ -457,21 +475,21 @@ void StopHTTPServer()
475 delete workQueue;
476 workQueue = nullptr;
477 }
478+ {
479+ LogPrint(BCLog::HTTP, "Disabling reading on current connections\n");
Can you add a comment about what the effect of disabling reading on the current connections is?
If this just disables reading and doesn’t close the connections or interrupt pending writes, it seems like shutdown code could potentially get stuck forever (if a client isn’t reading and there is a pending write and data is backed up).
Thanks for explaining, but I keep rereading this and coming away confused, because it is leaving out a lot of specifics. I think the following would be clearer if it is accurate:
0// Disable read events so the event loop is able to exit even if there are
1// open connections. (The preceding InterruptHTTPServer call stops accepting new
2// connections and new requests on existing connections, but does not close
3// existing connections.)
4//
5// Leave write events enabled because there may still be response data waiting
6// to be written, and we want to avoid sending incomplete responses.
603@@ -586,11 +604,10 @@ void HTTPRequest::WriteReply(int nStatus, const std::string& strReply)
604 // workaround above.
605 if (event_get_version_number() >= 0x02010600 && event_get_version_number() < 0x02020001) {
606 evhttp_connection* conn = evhttp_request_get_connection(req_copy);
607- if (conn) {
608+ LOCK(g_http_connections_cs);
609+ if (conn && g_http_connections.count(conn)) {
Comment above “Re-enable reading…” is out of date, or at least incomplete now, and would be good to update.
I think I need to look more into the workaround here, but it’s not clear to me why this no longer enables write events during shutdown (or what the effect of that is). Hopefully it doesn’t mean that a pending writes would never complete…
g_http_connections.count(conn) == 0.
g_http_connections.count is relevant in this context.
498- // Before this was solved with event_base_loopexit, but that didn't work as expected in
499- // at least libevent 2.0.21 and always introduced a delay. In libevent
500- // master that appears to be solved, so in the future that solution
501- // could be used again (if desirable).
502- // (see discussion in https://github.com/bitcoin/bitcoin/pull/6990)
503- if (threadResult.valid() && threadResult.wait_for(std::chrono::milliseconds(2000)) == std::future_status::timeout) {
threadResult. Can you remove it entirely (currently declared at L437 and assigned at L446)
Tested ACK d14515001bb2bf87602af7553def5ebe0f6bcdbb. I’m not a libevent expert, so I can’t review the API calls, but the changes look reasonable to me, and the test suite runs fine locally for me.
The minimum version of libevent officially supported is 2.0.22 (https://github.com/bitcoin/bitcoin/blob/750415701cb1c9e4cc717be7c810a054dbc2ead0/doc/dependencies.md). I’ve tested this on libevent 2.0.21. We should make sure that this gets tested on other versions of libevent.
Commits should be squashed prior to merge.
I think it would help this PR to have a more complete description. I would describe it as follows (feel free to copy any of it if useful).
This PR does three things:
It removes an event_base_loopexit optimization added in #11006 that turns out to cause a race condition described in #13492#issue-195407424, where incomplete HTTP responses are sent to clients on shutdown.
It replaces the #11006 optimization with a new one. Without the optimization from #11006, the HTTP event loop would never exit cleanly if there were any open idle connections to the HTTP server, because it would be stuck waiting for read events on those connections. The new optimization implemented here disables these unnecessary waits on shutdown by calling bufferevent_disable, while still waiting for any pending writes to complete.
It removes a two-second shutdown timeout, and instead will wait an infinite time for HTTP writes to complete. With the optimizations above, the two second timeout isn’t strictly necessary, since the loop will no longer wait forever when there are idle connections.
After more digging in libevent source code I’ve realized that sending the header Connection: close is the correct way to trigger the connection close. Verified that this behavior exists since 2.0.22, see https://github.com/libevent/libevent/blob/c51b159cff9f5e86696f5b9a4c6f517276056258/http.c#L472.
It is also unnecessary to keep track of connections as evhttp already handles that.
sending the header Connection: close is the correct way to trigger the connection close
That only signals to the client that the connection should be closed (e.g. no connection reuse), it doesn’t actually trigger a close from our side. (or does it? I’m confused now; at the least it doesn’t provide a guarantee that the connection will be closed any time soon, only after the request fully finished)
From testing it does, see https://github.com/libevent/libevent/blob/c51b159cff9f5e86696f5b9a4c6f517276056258/http.c#L784-L786.
Even with a non-http client, like nc, when I send that header the server will then close the
connection.
Labels
Bug
RPC/REST/ZMQ
Milestone
0.18.0