Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop upgrade-cancel callback registration for a generic "cancelable" #10770

Merged
merged 1 commit into from Sep 7, 2017

Conversation

Projects
None yet
7 participants
@TheBlueMatt
Copy link
Contributor

TheBlueMatt commented Jul 7, 2017

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.

src/qt/splashscreen.cpp Outdated
(cancel_possible ? (std::string("\n") + _("(press q to shutdown and continue later)"))
: std::string("")));

if (cancel_possible) {

This comment has been minimized.

@ryanofsky

ryanofsky Jul 7, 2017

Contributor

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.

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jul 7, 2017

Author Contributor

Nice.

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2017-07-upgrade-cancel-nits branch 2 times, most recently to 1d62f6e Jul 7, 2017

@fanquake fanquake added the GUI label Jul 7, 2017

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Jul 9, 2017

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

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

utACK 1d62f6e

@@ -36,8 +36,8 @@ public Q_SLOTS:
/** Show message and progress */
void showMessage(const QString &message, int alignment, const QColor &color);

/** Sets the break action */
void setBreakAction(const std::function<void(void)> &action);
/** Sets if cancel is currently possible */

This comment has been minimized.

@ryanofsky

ryanofsky Jul 10, 2017

Contributor

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?

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jul 10, 2017

Author Contributor

"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?

This comment has been minimized.

@ryanofsky

ryanofsky Jul 10, 2017

Contributor

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.

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jul 11, 2017

Author Contributor

Added some additional comments. I'm not really sure if thats a better name, though I agree cancel_possible is a bit strange.

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Jul 11, 2017

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.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

TheBlueMatt commented Jul 11, 2017

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

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

utACK d1db1f1

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Jul 15, 2017

Tested. Segfaults when cancelling during upgrade:

Thread 4 "bitcoin-shutoff" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffe117d700 (LWP 6034)]
CCoinsViewCache::DynamicMemoryUsage (this=0x0) at coins.cpp:40
40	    return memusage::DynamicUsage(cacheCoins) + cachedCoinsUsage;
(gdb) bt
#0  CCoinsViewCache::DynamicMemoryUsage (this=0x0) at coins.cpp:40
#1  0x0000555555850c4e in FlushStateToDisk (chainparams=..., state=..., mode=mode@entry=FLUSH_STATE_ALWAYS, nManualPruneHeight=nManualPruneHeight@entry=0) at validation.cpp:1899
#2  0x00005555558521bc in FlushStateToDisk () at validation.cpp:1969
#3  0x00005555556f6bf5 in Shutdown () at init.cpp:219
#4  0x00005555555d0ed0 in BitcoinCore::shutdown (this=0x55555698e330) at qt/bitcoin.cpp:309
#5  0x00007ffff5962359 in QObject::event(QEvent*) () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#6  0x00007ffff689835c in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#7  0x00007ffff689fb11 in QApplication::notify(QObject*, QEvent*) () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#8  0x00007ffff59358a0 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#9  0x00007ffff593802d in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#10 0x00007ffff5989b03 in ?? () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#11 0x00007ffff1644377 in g_main_context_dispatch () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#12 0x00007ffff16445e0 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#13 0x00007ffff164468c in g_main_context_iteration () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#14 0x00007ffff5989f0f in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#15 0x00007ffff593388a in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#16 0x00007ffff5760fe3 in QThread::exec() () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#17 0x00007ffff5765c98 in ?? () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#18 0x00007ffff504e6da in start_thread (arg=0x7fffe117d700) at pthread_create.c:456
#19 0x00007ffff387ed7f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:105
@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

TheBlueMatt commented Jul 15, 2017

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

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

TheBlueMatt commented Jul 16, 2017

@sipa yes, that appears to be another bug introduced in #10179 after d6af06d changed some semantics, I'll fix it in #10758 with my other pile of fixes that are gonna need to land for 15 anyway.

@jj1010

This comment has been minimized.

Copy link

jj1010 commented Jul 16, 2017

{
breakAction = action;
}
InitMessage(splash, title + strprintf("%d", nProgress) + "%" +

This comment has been minimized.

@sipa

sipa Jul 16, 2017

Member

Please put a space or a newline between the title and the progress.

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jul 16, 2017

Author Contributor

OK, cleaned it up.

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2017-07-upgrade-cancel-nits branch from d1db1f1 Jul 16, 2017

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

utACK 59147fa4d616e8f4ca05365957444cc74a30af51. Only change since last review is rearranged progress string.

@achow101

This comment has been minimized.

Copy link
Member

achow101 commented Aug 7, 2017

Having this would make fixing #10992 much easier.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Aug 15, 2017

@TheBlueMatt: can you rebase?

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2017-07-upgrade-cancel-nits branch Aug 16, 2017

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

TheBlueMatt commented Aug 16, 2017

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

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Aug 16, 2017

@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
{
InitMessage(splash, title + strprintf("%d", nProgress) + "%");
m_resume_possible = resume_possible;

This comment has been minimized.

@ryanofsky

ryanofsky Aug 16, 2017

Contributor

This seems to be set but not actually used anywhere?

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

TheBlueMatt commented Aug 16, 2017

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

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Aug 18, 2017

@achow101: for the rescan progress work, would having this PR be better or not having it?

@achow101

This comment has been minimized.

Copy link
Member

achow101 commented Aug 18, 2017

For rescan progress we needed the cancellation boolean, But since that has been dropped, it doesn't matter.

Drop upgrade-cancel callback registration for a generic "resumeable"
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().
@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

TheBlueMatt commented Aug 21, 2017

@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 TheBlueMatt force-pushed the TheBlueMatt:2017-07-upgrade-cancel-nits branch to ee4d149 Aug 21, 2017

@ryanofsky
Copy link
Contributor

ryanofsky left a 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).

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Sep 7, 2017

Tested ACK ee4d149

@jonasschnelli jonasschnelli merged commit ee4d149 into bitcoin:master Sep 7, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

jonasschnelli added a commit that referenced this pull request Sep 7, 2017

Merge #10770: Drop upgrade-cancel callback registration for a 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.