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

TheBlueMatt commented at 6:05 PM on March 30, 2017: member

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?

in src/init.cpp:138 in a91883ce4e outdated

 130 |  
 131 |  void StartShutdown()
 132 |  {
 133 |      fRequestShutdown = true;
 134 | +    std::unique_lock<std::mutex> lockShutdownWait(mutexShutdownWait);
 135 | +    cvShutdownWait.notify_all();

JeremyRubin commented at 6:25 PM on March 30, 2017:

Should this get mirrored to HandleSIGTERM?

laanwj commented at 6:48 PM on March 30, 2017:

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).

in src/init.cpp:144 in a91883ce4e outdated

 139 |      return fRequestShutdown;
 140 |  }
 141 |  
 142 | +void WaitForShutdownRequested() {
 143 | +    std::unique_lock<std::mutex> lockShutdownWait(mutexShutdownWait);
 144 | +    cvShutdownWait.wait(lockShutdownWait, []() { return ShutdownRequested(); });

JeremyRubin commented at 6:28 PM on March 30, 2017:

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

JeremyRubin commented at 6:29 PM on March 30, 2017: contributor

utack, no reason to sleep.

TheBlueMatt force-pushed on Mar 30, 2017

TheBlueMatt commented at 7:09 PM on March 30, 2017: member

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

jonasschnelli commented at 7:17 PM on March 30, 2017: contributor

The QT part would require something like this (though incomplete): https://github.com/jonasschnelli/bitcoin/commit/4ed60ffcbb6e95703d6f0bf96d5f0d54e24b2fa7

TheBlueMatt commented at 7:18 PM on March 30, 2017: member

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

TheBlueMatt force-pushed on Mar 30, 2017

fanquake added the label Refactoring on Mar 30, 2017

laanwj commented at 12:31 PM on March 31, 2017: member

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

jonasschnelli commented at 1:01 PM on March 31, 2017: contributor

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 commented at 1:10 PM on March 31, 2017: member

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.

in src/init.cpp:134 in ec4964882f outdated

 129 | +static std::condition_variable cvShutdownWait;
 130 |  
 131 |  void StartShutdown()
 132 |  {
 133 |      fRequestShutdown = true;
 134 | +    std::unique_lock<std::mutex> lockShutdownWait(mutexShutdownWait);

ryanofsky commented at 2:15 PM on March 31, 2017:

Should acquire the lock before setting fRequestShutdown?

TheBlueMatt commented at 5:24 PM on March 31, 2017:

Could, doesnt matter though because fRequestShutdown is atomic.

TheBlueMatt commented at 5:22 PM on March 31, 2017: member

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.

in src/init.cpp:130 in ec4964882f outdated

 124 | @@ -125,16 +125,27 @@ static const char* FEE_ESTIMATES_FILENAME="fee_estimates.dat";
 125 |  
 126 |  std::atomic<bool> fRequestShutdown(false);
 127 |  std::atomic<bool> fDumpMempoolLater(false);
 128 | +static std::mutex mutexShutdownWait;
 129 | +static std::condition_variable cvShutdownWait;

theuni commented at 7:24 PM on March 31, 2017:

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

jnewbery commented at 8:23 PM on March 31, 2017: member

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 commented at 9:22 PM on March 31, 2017: contributor

Maybe this Qt approach makes more sense: https://github.com/jonasschnelli/bitcoin/commit/d356512c054da926fac5a8a3f5c23721e41eaf9d

TheBlueMatt commented at 5:21 AM on April 1, 2017: member

@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 notifications@github.com wrote:

Maybe this Qt approach makes more sense: https://github.com/jonasschnelli/bitcoin/commit/d356512c054da926fac5a8a3f5c23721e41eaf9d

jonasschnelli commented at 5:46 AM on April 1, 2017: contributor

@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).

Exit bitcoind immediately on shutdown instead of 200ms later 5638344513

TheBlueMatt force-pushed on Apr 11, 2017

TheBlueMatt commented at 10:38 PM on April 11, 2017: member

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.

TheBlueMatt closed this on Apr 11, 2017

MarcoFalke locked this on Sep 8, 2021