GUI: allow a clean Bitcoin-Qt shutdown #1417

When using Bitcoin-Qt don't call exit() in Shutdown(), to allow a clean exit for Bitcoin-Qt - this ensures the removal of the tray-icon (in BitcoinGUI destructor) after the core is shutdown.

On Windows the tray-icon was still visible after a shutdown, which could lead to many orphan icons in the tray that only disappeared after hovering them with the mouse. This is now fixed.

Does this wait until the client is actually shutdown?

@luke-jr The function is called, after we hide the main Window in preparation of a shutdown, which is the right place.

No, the current behaviour showing the icon until the client has exited completely is by design. See bug #908

Alright, then the call is miss-placed, but it IS needed to fix the icon still shown after shutdown issue. Will update to find a better place :), thanks for pointing that out.

@luke-jr Okay, I took a look and come to the conclusion as it's not possible to pretend, when trayIcon will be deleted, we have to call trayIcon->hide() as long as we know it is controllable.

In the present code from master branch this will not succeed and so the icon is displayed, even if the app is closed already. Which one is worse, a tray-icon when there is no app or a missing tray-icon, while shutting down the app?

Edit: I think it's overkill to re-implement the class destructor from QSystemTrayIcon to call hide there.

Why can't you simply grab the pointer and hold onto it until shutdown completes?

Seems possible, but I'm not sure if I can do this in a clean and sufficient way. I'm sure other devs would say don't touch the Shutdown() function for hiding a system tray-icon and I think so too. If you have an easy and working solution for both problems here, you're welcome ;).

             Shutdown(NULL);
+            window.hideTrayIcon();

Well Shutdown(NULL) calls exit(), so it won't return and cannot reach window.hideTrayIcon();

@laanwj Any idea for this one ;)?

Yeah, good catch. Shutdown calls exit()... that kind of sucks. This means that execution flow never leaves the function, and thus the destructor is never called.

Luke-jr is right, though, that hiding the icon before the shutdown function also isn't desired behavior.

The reason that Shutdown calls exit is that, with bitcoind, any thread can call Shutdown(). No matter what thread calls it, it has to make sure that the process exits. With the UI, the UI thread is the only thread allowed to actually shutdown the core. Hence uiinterface->QueueShutdown().

As with the UI we have control over what happens after Shutdown, this means that you could #ifdef the final exit() from Shutdown to not be called when QT_GUI. This will fix the problem without needing any of the changes in this pull.

@laanwj Sounds good, will test and update this pull to only do that change.

Updated and verified to work (on Windows). Tray icon is removed, but is visible until core is shutdown.

ACK

NACK, bitcoin-qt never exits if you send it SIGTERM after this (though, to be fair, its not really any worse to how it used to SEGFAULT every once in a while, in any case, maybe we could fix that too)

SIGTERM is non Windows stuff, I need your help here :D.

When receiving that signal it is handled via HandleSIGTERM(), which simply sets fRequestShutdown to true. I could only find ThreadMessageHandler2(), which seems to initiate a Shutdown(), right?

Correct, after SIGTERM, we end up just calling Shutdown. I suppose we really just need to replace our Shutdown calls with uiInterface.QueueShutdown() (have I mentioned how ugly that looks when we are building without GUI?).

Wouldn't it work if I just extend my pull to modify Shutdown() to be like this in the end:

<pre> // ensure a clean exit for Bitcoin-Qt #ifndef QT_GUI printf("Bitcoin exited\n\n"); exit(0); #else uiInterface.QueueShutdown(); #endif </pre>

Sounds like a good solution. That would also likely fix the current crashes when sending sigterm to the ui executable.

Last comment was @bluematt.

Do not call queueshutdown from Shutdown that will cause big trouble! The qt event loop is long dead by that point and for non-ui you'd be calling shutdown recursively.

I will update, but I need one of you (if you use Linux / Unix) to test and verify this.

Personally, Id like to not have to see uiInterface.QueueShutdown everywhere, but be able to call Shutdown which calls uiInterface.QueueShutdown which can call FinishShutdown in its callback, or simply have Shutdown call FinishShutdown directly if we dont have GUI...but maybe Im the only one who doesnt like having to uiInterface in bitcoind...

@laanwj Damn, I was too quick ... and missunderstood your comment :-/.

@laanwj Well no, I don't get it ... calling uiInterface.QueueShutdown(); triggers QMetaObject::invokeMethod(QCoreApplication::instance(), "quit", Qt::QueuedConnection);, which puts a quit() command in the main event loop, right? So why should the event loop be gone when we are in Shutdown()? I can't find the connection between the two.

@laanwj Don't bother with my last question, I had a discussion on IRC with BlueMatt and will come up with a new pull-request tomorrow and close this one.

There is nothing wrong with the current sequence. QueueShutdown first winds down the Qt event loop, which, returning in the main function, properly calls Shutdown.

You cannot simply spawn Shutdown in a new thread when the UI is running, this results in crashes so it takes a safer way.

I really don't care about "seeing ui.QueueShutdown" everywhere. That's confounding bikeshedding with a real issue. Sure, if you can come up with a more proper name in non-ui context that's great but don't try to fix what's not broken!!!

See #1439, which I think is rather elegant.