Drop upgrade-cancel callback registration for a generic "cancelable" #10770
Conversation
src/qt/splashscreen.cpp
Outdated
There was a problem hiding this comment.
Don't you need to invoke setBreakAction(nullptr) here when cancel_possible is false?
Maybe better would be to get rid of this std::function madness and replace the SplashScreen::breakAction pointer with a simpler boolean std::atomic<bool> m_cancel_possible which you can just assign directly.
TheBlueMatt
force-pushed
the
2017-07-upgrade-cancel-nits
branch
2 times, most recently
from
20e66f7 to
1d62f6e
Compare
|
Makes sense. I guess I over-engineered the flexible callback (I thought we should have a way to not only shutdown and have the future option to "continue" but this really makes little sense). utACK 1d62f6e |
src/qt/splashscreen.h
Outdated
|
|
||
| /** Sets the break action */ | ||
| void setBreakAction(const std::function<void(void)> &action); | ||
| /** Sets if cancel is currently possible */ |
There was a problem hiding this comment.
I'd consider replacing "cancel possible" with "shutdown possible" everywhere in this PR. The function this is controlling is called StartShutdown. The display text is press q to shutdown. Why should this thing be named differently?
There was a problem hiding this comment.
"shutdown" is always possible (just may be slow), what you care about is cancelling the currnent "in progress" thing (and shutting down). I dont really care, but also dont think its particularly unclear?
There was a problem hiding this comment.
I dont really care, but also dont think its particularly unclear?
It's unclear to me what the flag is supposed to indicate or why it wouldn't always be true if shutdown is always possible. Maybe it should be called fast_shutdown_possible if that's what it actually means. Acked this because the PR is definitely a simplification, but I don't think the code or the reason for preventing the quit option from working after the upgrade is clear. I think better naming could help.
There was a problem hiding this comment.
Added some additional comments. I'm not really sure if thats a better name, though I agree cancel_possible is a bit strange.
|
This begs the question whether we shouldn't just have 'q for shutdown' always available? Maybe in some cases there is some initializationstep-specific cleanup, but there shouldn't be much as we're always allowed to crash anyway. |
|
@sipa yea, I thought about that. We should maybe move the boolean to just change the message to "will complete later if you shutdown now". I still think this is a nice cleanup, and given we're already in freeze for 0.15 it'd be too late to make that change for 0.15. If this sits for the next month without any movement I'll make that change for 16. |
|
Tested. Segfaults when cancelling during upgrade: |
|
@sipa hmm, I could be wrong but that strikes me as an issue that would also be present prior to the changes in this PR. |
|
Como faço para criar minha própria moeda virtual
Em sábado, 15 de julho de 2017, Matt Corallo <[email protected]>
escreveu:
… @sipa <https://github.com/sipa> yes, that appears to be another bug
introduced in d6af06d
<d6af06d>,
I'll fix it in #10758 <#10758>
with my other pile of fixes that are gonna need to land for 15 anyway.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#10770 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AcTu4glrwOGKax3kzh1ChzF8nR621hhHks5sOVLmgaJpZM4ORcVD>
.
|
src/qt/splashscreen.cpp
Outdated
| { | ||
| breakAction = action; | ||
| } | ||
| InitMessage(splash, title + strprintf("%d", nProgress) + "%" + |
There was a problem hiding this comment.
Please put a space or a newline between the title and the progress.
There was a problem hiding this comment.
OK, cleaned it up.
TheBlueMatt
force-pushed
the
2017-07-upgrade-cancel-nits
branch
from
d1db1f1 to
59147fa
Compare
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
utACK 59147fa4d616e8f4ca05365957444cc74a30af51. Only change since last review is rearranged progress string.
|
Having this would make fixing #10992 much easier. |
|
@TheBlueMatt: can you rebase? |
TheBlueMatt
force-pushed
the
2017-07-upgrade-cancel-nits
branch
from
59147fa to
e7797eb
Compare
|
"Rebased", but also just dropped the "cancelable" idea (as @sipa originally suggested) and always allow shutdown, using the passd-up boolean just to change the message to inform the user the current action will resume on restart. |
|
@TheBlueMatt There may be a use for keeping the cancellation boolean, so the interface can be reused to make the "rescanning" dialog in the GUI interruptible (cc @achow101). |
src/qt/splashscreen.cpp
Outdated
There was a problem hiding this comment.
This seems to be set but not actually used anywhere?
|
@sipa that is going to need a very different interface anyway, as StartShutdown() is not appropriate there (better to separate the interface between during-init progress and other-things progress IMO). |
|
@achow101: for the rescan progress work, would having this PR be better or not having it? |
|
For rescan progress we needed the cancellation boolean, But since that has been dropped, it doesn't matter. |
Instead of passing a StartShutdown reference all the way up from txdb, give ShowProgress a "resumeable" boolean, which is used to inform the user if the action will be resumed, but cancel is always allowed by just calling StartShutdown().
|
@achow101 well the cancellation boolean wouldn't have really helped, because the cancellation boolean in the previous version of this PR indicated to the GUI that it can call StartShutdown() to cancel, which is most definitely not what you want to do when its a rescan we're talking about. @ryanofsky thanks, fixed. |
TheBlueMatt
force-pushed
the
2017-07-upgrade-cancel-nits
branch
from
e7797eb to
ee4d149
Compare
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
utACK ee4d149. Changes since previous ACK are getting rid of cancel_possible bool to make 'q' shortcut to work unconditionally, and adding resume_possible bool to give an indication of when efficient resumes are possible.
@achow101 implemented rescan cancellation in #11200 which is compatible with this PR (though doesn't merge cleanly).
|
Tested ACK ee4d149 |
… "cancelable" ee4d149 Drop upgrade-cancel callback registration for a generic "resumeable" (Matt Corallo) Pull request description: Instead of passing a StartShutdown reference all the way up from txdb, give ShowProgress a "cancelable" boolean, as StartShutdown is pretty much always what you'll want to use to cancel. Use the same boolean to allow cancel during initial block verification. Tree-SHA512: 515817aaa4b9e3e856200e00be9c2d44ecfa2d4f288fe3e02116105fe85de2650c13076ee7e45396ec1ce6ab45e53b0477cddda7cfdee5b3bd0589cb81a4c346
…generic "cancelable" ee4d149 Drop upgrade-cancel callback registration for a generic "resumeable" (Matt Corallo) Pull request description: Instead of passing a StartShutdown reference all the way up from txdb, give ShowProgress a "cancelable" boolean, as StartShutdown is pretty much always what you'll want to use to cancel. Use the same boolean to allow cancel during initial block verification. Tree-SHA512: 515817aaa4b9e3e856200e00be9c2d44ecfa2d4f288fe3e02116105fe85de2650c13076ee7e45396ec1ce6ab45e53b0477cddda7cfdee5b3bd0589cb81a4c346
…generic "cancelable" ee4d149 Drop upgrade-cancel callback registration for a generic "resumeable" (Matt Corallo) Pull request description: Instead of passing a StartShutdown reference all the way up from txdb, give ShowProgress a "cancelable" boolean, as StartShutdown is pretty much always what you'll want to use to cancel. Use the same boolean to allow cancel during initial block verification. Tree-SHA512: 515817aaa4b9e3e856200e00be9c2d44ecfa2d4f288fe3e02116105fe85de2650c13076ee7e45396ec1ce6ab45e53b0477cddda7cfdee5b3bd0589cb81a4c346
7 participants
Instead of passing a StartShutdown reference all the way up from
txdb, give ShowProgress a "cancelable" boolean, as StartShutdown
is pretty much always what you'll want to use to cancel. Use the
same boolean to allow cancel during initial block verification.