qt: Poll ShutdownTimer after init is done #12377
Conversation
src/qt/bitcoin.cpp
Outdated
| @@ -467,8 +467,7 @@ void BitcoinApplication::initializeResult(bool success) | |||
| qDebug() << __func__ << ": Initialization result: " << success; | |||
| // Set exit result. | |||
| returnValue = success ? EXIT_SUCCESS : EXIT_FAILURE; | |||
| if(success) | |||
| { | |||
| if (success && !ShutdownRequested()) { | |||
There was a problem hiding this comment.
I'm not convinced that this makes a difference at all. initialzeResult is emitted with the return value of appInitMain. The return value of appInitMain ("success") is supposed to be false when shutdown was requested during initialization.
There was a problem hiding this comment.
It can make a difference if shutdown is requested between AppInitMain() returns and when this slot is executed, it's a QueuedConnection.
There was a problem hiding this comment.
Maybe
diff --git a/src/init.cpp b/src/init.cpp
index 50643ff96b..1f46e5e3d2 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1736,5 +1736,5 @@ bool AppInitMain()
StartWallets(scheduler);
#endif
- return true;
+ return !ShutdownRequested();
}There was a problem hiding this comment.
Indeed AppInitMain() return was changed recently in #11831, but I still think that is correct. IMO this change is also correct, it avoids initialization but the application should not crash without this change, so I don't agree it's a fix.
There was a problem hiding this comment.
It can make a difference if shutdown is requested between AppInitMain() returns and when this slot is executed, it's a QueuedConnection.
Then after AppInitMain was successfully executed, a shutdown was requested (but not yet acted on), so I don't understand why going into qt initialization would crash. It could successfully shutdown after that.
But anyhow if this fixes a reproducible issue I'm okay with it, it's possible that there are interactions I don't follow.
There was a problem hiding this comment.
so I don't understand why going into qt initialization would crash
That's why I think this doesn't fix the problem, just avoids it.
There was a problem hiding this comment.
That's good enough for rc3, though.
|
If you have means to reproduce the underlying issue, what might also work is moving the shutdown detection to only start after the GUI side of the initialization. E.g. move from |
maflcko
force-pushed
the
Mf1802-qtInitShutdown
branch
from
fa3024b
to
fafebdc
Compare
maflcko
changed the title
maflcko
force-pushed
the
Mf1802-qtInitShutdown
branch
from
fafebdc
to
2222bf0
Compare
maflcko
changed the title
|
utACK 2222bf0 |
|
Changed to @laanwj suggestion |
|
utACK 2222bf0 |
2222bf0 qt: Poll ShutdownTimer after init is done (MarcoFalke) Pull request description: The shutdown process has started in `requestShutdown`, but initialize will happily continue with `initializeResult` and start threads late in the shutdown progess. Deleting this running thread will crash the application according to the qt docs: https://github.com/qt/qtbase/blob/e5033a5c9b769815112e922d0b224af860afd219/src/corelib/thread/qthread.cpp#L412-L415 Potential fix for #12372 (comment) This reverts #11831 for now and hopefully restores the previous behaviour. Tree-SHA512: 8e1706afe90ddf2d972aca12c12d4cb2a9a4f38646c59c5466fe5a1a67361896b93c43917d5ac283841ee2bcc62e6bb8dc2bc81dea9129c899b354e9a4ef241b
Github-Pull: bitcoin#12377 Rebased-From: 2222bf0 Tree-SHA512: 575c10d9f043e2843616b989daaecfb357482445bc44b9f3af2fe0d891bb36b5ccb572f326703ea85291d56b9f6e8f4828496099b05889daea2277ad38aa0d0e
Summary: Backport of Core PR12377 bitcoin/bitcoin#12377 Test Plan: `ninja check check-functional` Reviewers: deadalnix, Fabien, #bitcoin_abc, markblundeberg Reviewed By: #bitcoin_abc, markblundeberg Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2545
2222bf0 qt: Poll ShutdownTimer after init is done (MarcoFalke) Pull request description: The shutdown process has started in `requestShutdown`, but initialize will happily continue with `initializeResult` and start threads late in the shutdown progess. Deleting this running thread will crash the application according to the qt docs: https://github.com/qt/qtbase/blob/e5033a5c9b769815112e922d0b224af860afd219/src/corelib/thread/qthread.cpp#L412-L415 Potential fix for bitcoin#12372 (comment) This reverts bitcoin#11831 for now and hopefully restores the previous behaviour. Tree-SHA512: 8e1706afe90ddf2d972aca12c12d4cb2a9a4f38646c59c5466fe5a1a67361896b93c43917d5ac283841ee2bcc62e6bb8dc2bc81dea9129c899b354e9a4ef241b
2222bf0 qt: Poll ShutdownTimer after init is done (MarcoFalke) Pull request description: The shutdown process has started in `requestShutdown`, but initialize will happily continue with `initializeResult` and start threads late in the shutdown progess. Deleting this running thread will crash the application according to the qt docs: https://github.com/qt/qtbase/blob/e5033a5c9b769815112e922d0b224af860afd219/src/corelib/thread/qthread.cpp#L412-L415 Potential fix for bitcoin#12372 (comment) This reverts bitcoin#11831 for now and hopefully restores the previous behaviour. Tree-SHA512: 8e1706afe90ddf2d972aca12c12d4cb2a9a4f38646c59c5466fe5a1a67361896b93c43917d5ac283841ee2bcc62e6bb8dc2bc81dea9129c899b354e9a4ef241b
2222bf0 qt: Poll ShutdownTimer after init is done (MarcoFalke) Pull request description: The shutdown process has started in `requestShutdown`, but initialize will happily continue with `initializeResult` and start threads late in the shutdown progess. Deleting this running thread will crash the application according to the qt docs: https://github.com/qt/qtbase/blob/e5033a5c9b769815112e922d0b224af860afd219/src/corelib/thread/qthread.cpp#L412-L415 Potential fix for bitcoin#12372 (comment) This reverts bitcoin#11831 for now and hopefully restores the previous behaviour. Tree-SHA512: 8e1706afe90ddf2d972aca12c12d4cb2a9a4f38646c59c5466fe5a1a67361896b93c43917d5ac283841ee2bcc62e6bb8dc2bc81dea9129c899b354e9a4ef241b
c853471 init: shutdown and return early if ActivateBestChain() fails. (furszy) 1961163 qt: Poll ShutdownTimer after init is done (Marco) 21d56e7 [GUI] Topbar: don't try to poll for data if shutdown was requested. (furszy) Pull request description: First commit was decoupled from #2131, the topbar staking status polling should not try to refresh data if shutdown was requested. Second commit is coming from bitcoin#12377. Third commit is starting the shutdown process and returning early (without starting the import thread) if the best chain cannot be activated during the node's initialization. ACKs for top commit: random-zebra: utACK c853471 Fuzzbawls: utACK c853471 Tree-SHA512: 8d5b289df0ba30d5f654b702e49b01127aa2470550d1313ed63022b0e6c737b3d092a50bd9caf759ea1308f20b6bab65a2a01ff89e7758e50826d389b257e16b
The shutdown process has started in
requestShutdown, but initialize will happily continue withinitializeResultand start threads late in the shutdown progess. Deleting this running thread will crash the application according to the qt docs:https://github.com/qt/qtbase/blob/e5033a5c9b769815112e922d0b224af860afd219/src/corelib/thread/qthread.cpp#L412-L415
Potential fix for #12372 (comment)
This reverts #11831 for now and hopefully restores the previous behaviour.