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.
but no longer breaks off requests in progress.
Possible attack vector by denying the service the ability to shut down / restart by keeping a connection open?
After -rpcservertimeout current connections will get booted, unless there is activity. If they do any requests they get a server unavailable error. But yes, if your application software is truly malicious they could keep the server busy by sending e.g. one header at a time, every timeout-1 seconds.
Note that this solution is cleaner in any case, it doesn’t rule out queuing a event_base_loopbreak(eventBase);, say, 30 seconds in the future to force a shutdown.
utACK.
I agree that the attack vectors are out of scope for this PR maybe for the whole http-json-rpc-daemon (it’s not hardened enough to expose to potential malicious environments).
@dcousens It’s great that you mentioned it. It reminded me of potential issue of this: what happens if a timer is stilll running, will it block the event queue exit until it triggers? Will test this scenario by stopping with an unlocked wallet.
Pushed another related fix:
Add a WaitExit() call to http’s WorkQueue to make StopHTTPServer delete the work queue only when all worker threads stopped.
This fixes a problem that was reproducable by pressing Ctrl-C during AppInit2:
0/usr/include/boost/thread/pthread/condition_variable_fwd.hpp:81: boost::condition_variable::~condition_variable(): Assertion `!ret' failed.
1/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”.
(now that the http module takes care of waiting for its own worker threads, we could even make it stop using threadGroup completely, but I leave that for a future refactor)
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.
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".
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.