Exit bitcoind immediately on shutdown instead of 200ms later #10125

TheBlueMatt · 2017-03-30T18:05:37Z

This is especially useful for AbortNode(). I have no idea how to make the equivalent change in bitcoin-qt, maybe @jonasschnelli can add some signal thing for it?

JeremyRubin

utack, no reason to sleep.

JeremyRubin · 2017-03-30T18:25:24Z

src/init.cpp


 void StartShutdown()
 {
    fRequestShutdown = true;
+    std::unique_lock<std::mutex> lockShutdownWait(mutexShutdownWait);
+    cvShutdownWait.notify_all();


Should this get mirrored to HandleSIGTERM?

JeremyRubin · 2017-03-30T18:28:55Z

src/init.cpp

 }
 bool ShutdownRequested()
 {
    return fRequestShutdown;
 }

+void WaitForShutdownRequested() {
+    std::unique_lock<std::mutex> lockShutdownWait(mutexShutdownWait);
+    cvShutdownWait.wait(lockShutdownWait, []() { return ShutdownRequested(); });


not really worth doing, but you could also wrap this in an initial if (!ShutdownRequested() so as not to take the lock.

laanwj · 2017-03-30T18:48:08Z

src/init.cpp


 void StartShutdown()
 {
    fRequestShutdown = true;
+    std::unique_lock<std::mutex> lockShutdownWait(mutexShutdownWait);
+    cvShutdownWait.notify_all();


Are you sure this is allowed in signal handlers? From what I remember that is the reason for the circuitous polling. If not we need a special path for signals and one for the rest (like AbortNode).

TheBlueMatt · 2017-03-30T19:09:28Z

Made the wait still wake up every 200ms to address the sigTERM issue.

jonasschnelli · 2017-03-30T19:17:43Z

The QT part would require something like this (though incomplete): jonasschnelli@4ed60ff

TheBlueMatt · 2017-03-30T19:18:28Z

@jonasschnelli is there a way to do that without adding a new thread? Some kind of signal in addition to the current polling method?

laanwj · 2017-03-31T12:31:12Z

Yes, you could use a signal on the uiInterface that sends a Qt signal, similar to how the other signals for ClientModel work.

jonasschnelli · 2017-03-31T13:01:35Z

Yes, you could use a signal on the uiInterface that sends a Qt signal, similar to how the other signals for ClientModel work.

Yes. That would be much better. I'll write a commit soon.

laanwj · 2017-03-31T13:10:41Z

Yes. That would be much better. I'll write a commit soon.

Great!
I guess the uiInterface signal could be called in WaitForShutdown after the wait loop exits. This means we don't need to do any polling of fRequestShutdown ourselves in the GUI anymore. Much cleaner indeed.
never mind: we don't use WaitForShutdown when the GUI runs. Need to keep the polling for the same reason WaitForShutdown does, UNIX signal handling.

ryanofsky · 2017-03-31T14:15:30Z

src/init.cpp


 void StartShutdown()
 {
    fRequestShutdown = true;
+    std::unique_lock<std::mutex> lockShutdownWait(mutexShutdownWait);


Should acquire the lock before setting fRequestShutdown?

Could, doesnt matter though because fRequestShutdown is atomic.

TheBlueMatt · 2017-03-31T17:22:47Z

Honestly I have no idea why travis is failing here. If you remove the cv notify_all() it works fine, the second you add it back travis hangs and I cannot reproduce the issue locally.

theuni · 2017-03-31T19:24:15Z

src/init.cpp

@@ -125,16 +125,27 @@ static const char* FEE_ESTIMATES_FILENAME="fee_estimates.dat";

 std::atomic<bool> fRequestShutdown(false);
 std::atomic<bool> fDumpMempoolLater(false);
+static std::mutex mutexShutdownWait;
+static std::condition_variable cvShutdownWait;


Use a CThreadInterrupt here instead? This is exactly what it's meant to be used for :)

jnewbery · 2017-03-31T20:23:59Z

I did a run of travis with the create_cache.py script commented out (so the test_runner script just launches the test cases immediately) and the tests seem to fail intermittently. Travis logs here: https://travis-ci.org/jnewbery/bitcoin/builds/217310887

Failure is:

raceback (most recent call last):
  File "/home/travis/build/jnewbery/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/authproxy.py", line 125, in _request
    return self._get_response()
  File "/home/travis/build/jnewbery/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/authproxy.py", line 167, in _get_response
    http_response = self.__conn.getresponse()
  File "/usr/lib/python3.4/http/client.py", line 1208, in getresponse
    response.begin()
  File "/usr/lib/python3.4/http/client.py", line 380, in begin
    version, status, reason = self._read_status()
  File "/usr/lib/python3.4/http/client.py", line 350, in _read_status
    raise BadStatusLine(line)
http.client.BadStatusLine: ''
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "/home/travis/build/jnewbery/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/test_framework.py", line 151, in main
    self.run_test()
  File "/home/travis/build/jnewbery/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/fundrawtransaction.py", line 449, in run_test
    self.nodes[1].encryptwallet("test")
  File "/home/travis/build/jnewbery/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/coverage.py", line 46, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/home/travis/build/jnewbery/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/authproxy.py", line 151, in __call__
    response = self._request('POST', self.__url.path, postdata.encode('utf-8'))
  File "/home/travis/build/jnewbery/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/authproxy.py", line 129, in _request
    self.__conn.request(method, path, postdata, headers)
  File "/usr/lib/python3.4/http/client.py", line 1125, in request
    self._send_request(method, url, body, headers)
  File "/usr/lib/python3.4/http/client.py", line 1163, in _send_request
    self.endheaders(body)
  File "/usr/lib/python3.4/http/client.py", line 1121, in endheaders
    self._send_output(message_body)
  File "/usr/lib/python3.4/http/client.py", line 951, in _send_output
    self.send(msg)
  File "/usr/lib/python3.4/http/client.py", line 886, in send
    self.connect()
  File "/usr/lib/python3.4/http/client.py", line 863, in connect
    self.timeout, self.source_address)
  File "/usr/lib/python3.4/socket.py", line 512, in create_connection
    raise err
  File "/usr/lib/python3.4/socket.py", line 503, in create_connection
    sock.connect(sa)
ConnectionRefusedError: [Errno 111] Connection refused

This reproduces locally for me. Seems like there's a race between bitcoind shutting down and responding to the 'stop' RPC.

jonasschnelli · 2017-03-31T21:22:08Z

Maybe this Qt approach makes more sense: jonasschnelli@d356512

TheBlueMatt · 2017-04-01T05:21:58Z

@jonasschnelli cool, thanks. Sadly, in the signal handlers we can't call such a function, so we need to have both polling and a notification. I'll look into that later today.

…

On March 31, 2017 11:22:12 PM GMT+02:00, Jonas Schnelli ***@***.***> wrote: Maybe this Qt approach makes more sense: jonasschnelli@d356512

jonasschnelli · 2017-04-01T05:46:24Z

@TheBlueMatt: if you want to execute the function in the main GUI thread, add another Qt signal and use Q_EMIT (emit it from the uiInterface Signal listener).

TheBlueMatt · 2017-04-11T22:38:49Z

Closing. I spent some time debugging further but dont think its a huge deal to sleep 200ms and it looks annoying to fix, at best. Someone else is welcome to pick this up if they feel so inclined.

JeremyRubin reviewed Mar 30, 2017

View reviewed changes

laanwj reviewed Mar 30, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-03-faster-shutdown branch from a91883c to 8d790a8 Compare March 30, 2017 19:09

TheBlueMatt force-pushed the 2017-03-faster-shutdown branch from 8d790a8 to ec49648 Compare March 30, 2017 19:46

fanquake added the Refactoring label Mar 30, 2017

ryanofsky reviewed Mar 31, 2017

View reviewed changes

theuni reviewed Mar 31, 2017

View reviewed changes

Exit bitcoind immediately on shutdown instead of 200ms later

5638344

TheBlueMatt force-pushed the 2017-03-faster-shutdown branch from ec49648 to 5638344 Compare April 11, 2017 19:31

TheBlueMatt closed this Apr 11, 2017

laanwj mentioned this pull request Apr 22, 2017

[Qt] Remove shutdown poll timer and replace it with a signal #10254

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exit bitcoind immediately on shutdown instead of 200ms later #10125

Exit bitcoind immediately on shutdown instead of 200ms later #10125

TheBlueMatt commented Mar 30, 2017

JeremyRubin left a comment

JeremyRubin Mar 30, 2017

JeremyRubin Mar 30, 2017

laanwj Mar 30, 2017

TheBlueMatt commented Mar 30, 2017

jonasschnelli commented Mar 30, 2017

TheBlueMatt commented Mar 30, 2017

laanwj commented Mar 31, 2017

jonasschnelli commented Mar 31, 2017

laanwj commented Mar 31, 2017 •

edited

Loading

ryanofsky Mar 31, 2017

TheBlueMatt Mar 31, 2017

TheBlueMatt commented Mar 31, 2017 •

edited

Loading

theuni Mar 31, 2017

jnewbery commented Mar 31, 2017

jonasschnelli commented Mar 31, 2017

TheBlueMatt commented Apr 1, 2017 via email

jonasschnelli commented Apr 1, 2017

TheBlueMatt commented Apr 11, 2017

Exit bitcoind immediately on shutdown instead of 200ms later #10125

Exit bitcoind immediately on shutdown instead of 200ms later #10125

Conversation

TheBlueMatt commented Mar 30, 2017

JeremyRubin left a comment

Choose a reason for hiding this comment

JeremyRubin Mar 30, 2017

Choose a reason for hiding this comment

JeremyRubin Mar 30, 2017

Choose a reason for hiding this comment

laanwj Mar 30, 2017

Choose a reason for hiding this comment

TheBlueMatt commented Mar 30, 2017

jonasschnelli commented Mar 30, 2017

TheBlueMatt commented Mar 30, 2017

laanwj commented Mar 31, 2017

jonasschnelli commented Mar 31, 2017

laanwj commented Mar 31, 2017 • edited Loading

ryanofsky Mar 31, 2017

Choose a reason for hiding this comment

TheBlueMatt Mar 31, 2017

Choose a reason for hiding this comment

TheBlueMatt commented Mar 31, 2017 • edited Loading

theuni Mar 31, 2017

Choose a reason for hiding this comment

jnewbery commented Mar 31, 2017

jonasschnelli commented Mar 31, 2017

TheBlueMatt commented Apr 1, 2017 via email

jonasschnelli commented Apr 1, 2017

TheBlueMatt commented Apr 11, 2017

laanwj commented Mar 31, 2017 •

edited

Loading

TheBlueMatt commented Mar 31, 2017 •

edited

Loading