re-work Shutdown() function

Diapolo commented at 5:51 am on June 11, 2012: none

This pull is based on a discussion with @TheBlueMatt and @laanwj.

introduce a new StartShutdown() function, which starts a thread with Shutdown() if no GUI is used and calls uiInterface.QueueShutdown() if a GUI is used
all direct uiInterface.QueueShutdown() calls are replaced with StartShutdown() - this ensures a clean GUI shutdown, even when catching a SIGTERM and allows the BitcoinGUI destructor to get called (which fixes a tray-icon issue and keeps the tray-icon until Bitcoin-Qt exits)

The current implementation (current master branch) sometimes crashes Bitcoin-Qt (bad) and doesn’t allow GUI object destructors do their work as we call exit() in Shutdown(), which leads to an unremoved tray-icon (as visible bug).

I’m sure we could remove some #include "ui_interface.h" in the modified files, but I was not really sure about that, so input is welcome.

Fixes: #1182

laanwj commented at 6:13 am on June 11, 2012: member

I appreciate that you’re trying to improve the naming, but be careful that this does change behaviour in the case of bitcoind.

Previously, QueueShutdown (in non-ui context) started Shutdown in a new thread, to make sure that execution returns immediately. This means that an RPC command such as “quit” got the chance to return a reply. It now calls Shutdown directly, which shuts down bitcoin in the current thread and then exit()s.

in src/init.cpp: in ecef6722d4 outdated

47+#else
48+    FinishShutdown(NULL);
49+#endif
50+}
51+
52+void FinishShutdown(void* parg)

laanwj commented at 6:15 am on June 11, 2012:

I don’t like the name “FinishShutdown” much. This is not finishing the shutdown, it is the only real shutdown functionality, the other functions are just queuing it to happen.

Diapolo commented at 6:27 am on June 11, 2012:

If we call Shutdown() Start- or InitShutdown() this can be Shutdown() again, which would be fine then.

laanwj commented at 8:15 am on June 11, 2012:

Yes, that’s much more clear

in src/init.cpp: in ecef6722d4 outdated

43+{
44+#ifdef QT_GUI
45+    // this ensures we leave the Qt main loop for a clean GUI exit (FinishShutdown() is called in bitcoin.cpp afterwards)
46+    uiInterface.QueueShutdown();
47+#else
48+    FinishShutdown(NULL);

laanwj commented at 6:15 am on June 11, 2012:

See my comment about changed functionality, this should be

0// Without UI, FinishShutdown can simply be started in a new thread
1CreateThread(FinishShutdown, NULL);

Diapolo commented at 6:28 am on June 11, 2012:

ACK

in src/noui.cpp: in ecef6722d4 outdated

21@@ -22,8 +22,8 @@ static bool noui_ThreadSafeAskFee(int64 nFeeRequired, const std::string& strCapt
22 
23 static void noui_QueueShutdown()

laanwj commented at 6:17 am on June 11, 2012:

This would be no longer used

Diapolo commented at 6:26 am on June 11, 2012:

So when I change the final Shutdown-call for no GUI to create a thread this function can be removed then, right?

in src/init.cpp: in ecef6722d4 outdated

37@@ -38,7 +38,17 @@ void ExitTimeout(void* parg)
38 #endif
39 }
40 
41-void Shutdown(void* parg)
42+void Shutdown()

laanwj commented at 6:18 am on June 11, 2012:

Maybe StartShutdown()? The current name doesn’t make clear that the actual shutdown happens later / in another thread asynchronously.

Diapolo commented at 6:24 am on June 11, 2012:

ACK, the naming should be more clear StartShutdown or InitShutdown()?

Diapolo commented at 7:22 am on June 11, 2012: none

@laanwj Updated to reflect your suggestions. Can you explain, why noui.cpp is not added in bitcoin-qt.pro? I found it quite strange to not be able to edit that file directly somehow.

laanwj commented at 7:27 am on June 11, 2012: member

Looks good now.

Well, it used to be that noui.cpp conflicted with some functions in the UI. If this is no longer the case due to using boost::signals now, it could be added in bitcoin-qt.pro too… (or otherwise you could add it under “other” files)

Diapolo commented at 7:35 am on June 11, 2012: none

Fine and at least on my local build it compiles just fine with noui.cpp added to the .pro. Could you verify, if this would break the official Gitian build process before I update the pull?

laanwj commented at 7:43 am on June 11, 2012: member

I’m unable to test gitian right now.

Maybe it’s better to leave that for another (more trivial) pull. Focus this one on shutdown functionality, this is quite a big fish to fry, and needs heavy testing.

Diapolo commented at 7:56 am on June 11, 2012: none

Okay, so let’s see what other devs think.

laanwj commented at 8:08 am on June 11, 2012: member

BTW, does this solve the original issue with exit() in Shutdown? I don’t see it in the diff.

in src/init.cpp: in befeb262da outdated

158@@ -148,7 +159,7 @@ bool AppInit(int argc, char* argv[])
159         PrintException(NULL, "AppInit()");
160     }
161     if (!fRet)
162-        Shutdown(NULL);
163+        Shutdown();

TheBlueMatt commented at 1:00 pm on June 11, 2012:

This and the previous change do not compile.

Diapolo commented at 1:18 pm on June 11, 2012:

Ah I missed that, because it’s in an #ifdef, which my compiler ignores, because I compile the GUI version. It should be a Shutdown(NULL).

TheBlueMatt commented at 1:10 pm on June 11, 2012: member

If the one non-compiling change is reverted, ACK

Diapolo commented at 1:30 pm on June 11, 2012: none

I’m asking myself, if the change in net.cpp is correct, but as net.cpp is used by the Qt version and bitcoind it should be a StartShutdown() call. @TheBlueMatt I reverted the changes that don’t compile, they now use Shutdown(NULL); again, which should be fine for a non Qt version. @laanwj The exit() call in Shutdown() is not an issue anymore for the Qt version, as Shutdown() was moved down (see: https://github.com/bitcoin/bitcoin/pull/1439/files#L6R304) to ensure e.g. BitcoinGUI destructor get’s called and objects are not just “killed”.

TheBlueMatt commented at 1:36 pm on June 11, 2012: member

ACK

Diapolo commented at 1:47 pm on June 11, 2012: none

@TheBlueMatt One last thing that bothers me, should the 2 Shutdown(NULL); calls in init.cpp be converted to StartShutdown();, which could be considered the default function to call, when a shutdown is needed. It would end up with Shutdown() via a thread, but fits better to the rest of the shutdown code and usage.

TheBlueMatt commented at 1:51 pm on June 11, 2012: member

No, in those two cases, shutdown needs to happen immediately, so launching a shutdown thread instead could cause errors.

Diapolo commented at 1:53 pm on June 11, 2012: none

Understood, thanks :).

laanwj commented at 1:54 pm on June 11, 2012: member

@diapolo ok, that’s also a possibility, as it appears that nothing else can “fall through” to there

laanwj commented at 1:56 pm on June 11, 2012: member

It does create another issue, though: the icon disappears before the shutdown starts, instead of after it ends. That’s one of the things we were trying to prevent. It’s useful to keep the icon until the program actually exits, to tell the user that something is running.

Diapolo commented at 2:21 pm on June 11, 2012: none

@laanwj Seems rather hard to ensure a clean shutdown and wanting to keep the tray-icon till the very end. Leaving the Qt Shutdown(NULL) were it was before leads to killing Qt objects. this looks much more sane now. So any further idea (perhaps in another pull if this requires additional re-work on the GUI)?

Diapolo commented at 2:25 pm on June 11, 2012: none

Perhaps move the Shutdown() back to where it was and re-add the #ifdef QT_GUI around the exit() in Shutdown(). This would make Bitcoin-Qt exit via the last return 0 in bitcoin.cpp.

laanwj commented at 2:36 pm on June 11, 2012: member

Yeah - IMO, the idea to not call exit() at the end of the Shutdown() function in the case of the UI was good. This allows it to wind things down afterward instead of before. (which will eventually also allow it to show some “Warning - do not shutdown computer while Bitcoin is shutting down” warning).

Edit: or even better: make it explicit and save an #ifdef. Create a ShutdownAndExit() function that shuts down and exits (calls Shutdown then exit), and a normal Shutdown() that just shuts down, then use each one at the appropriate place.

Diapolo commented at 8:12 pm on June 11, 2012: none

@laanwj I see 2 problems with your last suggestion.

Shutdown() calls CreateThread(ExitTimeout, NULL); which seems to be a 5 second timer, before the process get’s killed anyway, so the Shutdown() function still somehow exits / kills the client.
I think an #ifdef is easier / cleaner than another function in this case.

Diapolo commented at 8:36 pm on June 11, 2012: none

Updated to ensure tray-icon is kept until Bitcoin-Qt exits, no further changes for no UI client.

TheBlueMatt commented at 8:39 pm on June 11, 2012: member

Well, it ended up with more ifdefs than I thought it would need, but it still looks better than it was imho, ACK.

in src/qt/bitcoin.cpp: in 4da1883905 outdated

301@@ -301,6 +302,7 @@ int main(int argc, char *argv[])
302     } catch (...) {
303         handleRunawayException(NULL);
304     }
305+    // this exits Bitcoin-Qt

laanwj commented at 5:18 am on June 12, 2012:

This comment is no longer needed :)

Diapolo commented at 5:21 am on June 12, 2012:

A final update, which only removes this line was pushed ^^.

laanwj commented at 5:18 am on June 12, 2012: member

ACK

introduce a new StartShutdown() function, which starts a thread with Shutdown() if no GUI is used and calls uiInterface.QueueShutdown() if a GUI is used / all direct uiInterface.QueueShutdown() calls are replaced with Shutdown() - this ensures a clean GUI shutdown, even when catching a SIGTERM and allows the BitcoinGUI destructor to get called (which fixes a tray-icon issue and keeps the tray-icon until Bitcoin-Qt exits) 9247134eab

TheBlueMatt commented at 0:10 am on June 13, 2012: member

Note for whoever merges, close #1182, as this fixes it.

laanwj commented at 7:18 am on June 13, 2012: member

I think we should aim to merge this asap, but as this affects critical functionality Gavin or Sipa should probably take a look at it for a sanity check.

sipa commented at 12:10 pm on June 13, 2012: member

ACK on the changes to core; The exit(0) in Shutdown inside ifndef qt is a bit ugly though, imho.

laanwj commented at 12:32 pm on June 13, 2012: member

Well, IMO the problem is that “shutdown bitcoin” and “exit program” are essentially different actions.

That’s why I proposed adding a ShutdownAndExit function, and using that where appropriate: in StartShutdown [ifndef QT_GUI] and AppInit. The name would also clearly signal to the programmer that the function never returns. The normal Shutdown function would shut down bitcoin and return.

Diapolo thought it’d be less clear, but I think it’d be better to be explicit.

laanwj referenced this in commit 0f10b21719 on Jun 14, 2012

laanwj merged this on Jun 14, 2012

laanwj closed this on Jun 14, 2012

coblee referenced this in commit ed2a782abe on Jul 17, 2012

lateminer referenced this in commit 2ed53ff627 on Jan 22, 2019

lateminer referenced this in commit eb1ed66c2e on May 6, 2020

lateminer referenced this in commit cb6d15a6ac on May 6, 2020

lateminer referenced this in commit 1fa5156a97 on May 6, 2020

lateminer referenced this in commit b26dbc4e55 on May 6, 2020

lateminer referenced this in commit c94f2412af on May 6, 2020

DrahtBot locked this on Sep 8, 2021

re-work Shutdown() function #1439