gui: Fix boost::signals2::no_slots_error in early calls to InitWarning by promag · Pull Request #14783 · bitcoin/bitcoin

promag · 2018-11-22T12:24:12Z

Adding the following to bitcoin.conf

[xxx]
disablewallet=1

And running bitcoin-qt gives:

libc++abi.dylib: terminating with uncaught exception of type boost::exception_detail::clone_impl<boost::exception_detail::error_info_injector<boost::signals2::no_slots_error> >: boost::signals2::no_slots_error

Fixes regression in #14708.

src/qt/bitcoin.cpp

promag · 2018-11-22T12:31:31Z

Alternatively could call noui_connect in src/qt/bitcoin.cpp?

laanwj · 2018-11-22T12:34:56Z

I think the safest solution to this is to not call the message box at all for unrecognized sections. Simply call LogPrintf directly. That's what will happen after all!

promag · 2018-11-22T12:54:53Z

Updated, @MarcoFalke you might want to check this out since you reviewed #14708.

laanwj · 2018-11-22T12:57:02Z

Concept ACK

So to be clear, the problem is that at this stage of initialization, we're not yet able to show message boxes in Qt [1]. @promag's initial fix changed the slot handler to first register a message box handler that only logs, but, it seems safer to simply change these InitWarning calls to LogPrintf as that's what is going to happen both for the UI and bitcoind.

The reason is that there are some things concerning locale and language setup that only can run after configuration parsing, after which the main UI window is created. It might be possible to refactor this at some point but for fixing the issue, this is the safer option.

Empact · 2018-11-22T14:24:40Z

~~May as well match the form of the other warnings in this function, no? They print a leading LogPrintf("%s: ...", __func__, ...).~~

promag · 2018-11-22T14:26:00Z

Now using fprintf(stderr, ...) Instead of LogPrintf so that test/functional/feature_config_args.py passes.

maflcko

Slightly tend to NACK. This makes the code fragile in light of refactoring such as move-only commits, since it is not clear when InitWarning is safe to call. Wouldn't it be easier to always register the no_ui listener to the signal as soon as possible in the initialization sequence (regardless of bitcoind vs bitcoin-qt) and then optionally, if the gui is run, also register the gui message box listener?

Side-note: Previously we wouldn't even log nor print warnings when running the gui, but that is now fixed (see #14162)

maflcko · 2018-11-22T17:21:24Z

src/init.cpp

InitWarning also prints to the debug log, fprintf does not.

…arly calls to InitWarning

promag · 2018-11-22T17:46:10Z

Previous fix in 91ce5a2, which @MarcoFalke disapproved.

Current fix does what was suggested in #14783 (comment).

Concept ACK

promag · 2018-11-22T20:52:28Z

Not sure about this because I think it should go thru interfaces::Node?

maflcko

Should remove the now-redundant (?) static void InitMessage in src/qt/bitcoin.cpp?

laanwj · 2018-11-23T07:48:52Z

I'm still not sure about this, but ok…

For example:

Does noui_register register anything else that might interfere with the GUI?
What about the return argument of ThreadSafeMessageBox, will registering multiple handlers cause an issue here (similar for other handlers that noui_register registers)

promag · 2018-11-23T07:58:20Z

@laanwj I'm not ignoring them, I'm trying to address them.

promag · 2018-11-23T15:37:26Z

@laanwj you mean noui_connect instead of noui_register. Regarding:

Does noui_register register anything else that might interfere with the GUI?

noui_connect only connects 3 slots that only call LogPrintf and fprintf, so I think we are safe.

What about the return argument of ThreadSafeMessageBox, will registering multiple handlers cause an issue here (similar for other handlers that noui_register registers)

In boost::signals, slot results are combined to give the signal result. The default combiner is to give the last slot result. So the first connected slots are those in noui_connect and later are the UI slots, which means the signal result comes from the UI slots. So it's safe to say there is no behavior change.

DrahtBot · 2018-11-23T19:10:19Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#11625 (Add BitcoinApplication & RPCConsole tests by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

src/qt/bitcoin.cpp

jonasschnelli · 2018-11-25T05:42:55Z

Concept ACK

promag · 2018-11-25T15:19:33Z

Should remove the now-redundant (?) static void InitMessage in src/qt/bitcoin.cpp?

@MarcoFalke done.

maflcko · 2018-11-26T17:36:45Z

src/qt/bitcoin.cpp

+    std::unique_ptr<interfaces::Handler> handler_question = node->handleQuestion(noui_ThreadSafeQuestion);
+    std::unique_ptr<interfaces::Handler> handler_init_message = node->handleInitMessage(noui_InitMessage);
+
    // Do not refer to data directory yet, this can be overridden by Intro::pickDataDirectory


What makes it safe to subscribe to them here already, when some of then log to the debug log, which is in the datadir?

If that was the case then boost::signals2::no_slots_error would have been raised before?

maflcko · 2018-11-27T19:37:33Z

utACK 6bbdb20. Going to test the next time I compile the gui

Sjors · 2018-12-06T15:38:52Z

I can reproduce the error on macOS 10.14.1 and confirm 6bbdb20 fixes it. It shows a Section [xxx] is not recognized. after the command, but someone launching QT via a desktop icon wouldn't see that.

Would be nice to have to test, though that might require #11625

promag · 2018-12-06T15:44:56Z

@Sjors thanks, and the improvement can come next.

…y calls to InitWarning 6bbdb20 squashme: connect thru node interface (João Barbosa) a0f8df3 qt: Call noui_connect to prevent boost::signals2::no_slots_error in early calls to InitWarning (João Barbosa) Pull request description: Adding the following to `bitcoin.conf` ``` [xxx] disablewallet=1 ``` And running `bitcoin-qt` gives: ``` libc++abi.dylib: terminating with uncaught exception of type boost::exception_detail::clone_impl<boost::exception_detail::error_info_injector<boost::signals2::no_slots_error> >: boost::signals2::no_slots_error ``` Fixes regression in bitcoin#14708. Tree-SHA512: 7c158376fad6ebcd80fc0dbe549d5b6e893fb82e7dc1e455825633d7f91b14dc34493487cab7642152e88f9eaf99bfa91988972d600e9fb289cf26afd64aff8a

…_slots_error in early calls to InitWarning Summary: **Merge bitcoin#14783: gui: Fix boost::signals2::no_slots_error in early calls to InitWarning** 6bbdb20 squashme: connect thru node interface (João Barbosa) a0f8df3 qt: Call noui_connect to prevent boost::signals2::no_slots_error in early calls to InitWarning (João Barbosa) **Pull request description:** Adding the following to `bitcoin.conf` ``` [xxx] disablewallet=1 ``` And running `bitcoin-qt` gives: ``` libc++abi.dylib: terminating with uncaught exception of type boost::exception_detail::clone_impl<boost::exception_detail::error_info_injector<boost::signals2::no_slots_error> >: boost::signals2::no_slots_error ``` Fixes regression in bitcoin#14708. --- This is a backport of Core [[bitcoin/bitcoin#14783 | PR14783]] Test Plan: ninja check BITCOIND={full_path_to_build_dir}/src/qt/bitcoin-qt ./test/functional/test_runner.py feature_config_args feature_includeconf Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D5794

…y calls to InitWarning 6bbdb20 squashme: connect thru node interface (João Barbosa) a0f8df3 qt: Call noui_connect to prevent boost::signals2::no_slots_error in early calls to InitWarning (João Barbosa) Pull request description: Adding the following to `bitcoin.conf` ``` [xxx] disablewallet=1 ``` And running `bitcoin-qt` gives: ``` libc++abi.dylib: terminating with uncaught exception of type boost::exception_detail::clone_impl<boost::exception_detail::error_info_injector<boost::signals2::no_slots_error> >: boost::signals2::no_slots_error ``` Fixes regression in bitcoin#14708. Tree-SHA512: 7c158376fad6ebcd80fc0dbe549d5b6e893fb82e7dc1e455825633d7f91b14dc34493487cab7642152e88f9eaf99bfa91988972d600e9fb289cf26afd64aff8a

laanwj reviewed Nov 22, 2018

View reviewed changes

src/qt/bitcoin.cpp Outdated Show resolved Hide resolved

fanquake added the GUI label Nov 22, 2018

promag force-pushed the 2018-11-fix-noslotserror branch from efb9276 to 3a6a489 Compare November 22, 2018 12:52

promag changed the title ~~qt: Fix boost::signals2::no_slots_error in early calls to InitWarning~~ Replace early calls to InitWarning by LogPrintf Nov 22, 2018

promag force-pushed the 2018-11-fix-noslotserror branch 2 times, most recently from 1d75da3 to 91ce5a2 Compare November 22, 2018 14:22

promag changed the title ~~Replace early calls to InitWarning by LogPrintf~~ Replace early calls to InitWarning with fprintf(stderr, ...) Nov 22, 2018

maflcko previously requested changes Nov 22, 2018

View reviewed changes

src/init.cpp Outdated

Copy link

Member

maflcko Nov 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InitWarning also prints to the debug log, fprintf does not.

qt: Call noui_connect to prevent boost::signals2::no_slots_error in e…

a0f8df3

…arly calls to InitWarning

promag force-pushed the 2018-11-fix-noslotserror branch from 91ce5a2 to a0f8df3 Compare November 22, 2018 17:44

promag changed the title ~~Replace early calls to InitWarning with fprintf(stderr, ...)~~ Fix boost::signals2::no_slots_error in early calls to InitWarning Nov 22, 2018

maflcko changed the title ~~Fix boost::signals2::no_slots_error in early calls to InitWarning~~ gui: Fix boost::signals2::no_slots_error in early calls to InitWarning Nov 22, 2018

maflcko reviewed Nov 22, 2018

View reviewed changes

maflcko reviewed Nov 23, 2018

View reviewed changes

src/qt/bitcoin.cpp Outdated Show resolved Hide resolved

squashme: connect thru node interface

6bbdb20

promag force-pushed the 2018-11-fix-noslotserror branch from bdf4d55 to 6bbdb20 Compare November 24, 2018 22:17

maflcko reviewed Nov 26, 2018

View reviewed changes

maflcko merged commit 6bbdb20 into bitcoin:master Dec 6, 2018

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

promag commented Nov 22, 2018 • edited by laanwj Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

promag commented Nov 22, 2018

Uh oh!

laanwj commented Nov 22, 2018

Uh oh!

promag commented Nov 22, 2018

Uh oh!

laanwj commented Nov 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented Nov 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Nov 22, 2018

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Nov 22, 2018

Choose a reason for hiding this comment

Uh oh!

promag commented Nov 22, 2018

Uh oh!

promag commented Nov 22, 2018

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Nov 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Nov 23, 2018

Uh oh!

promag commented Nov 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 23, 2018

Conflicts

Uh oh!

Uh oh!

jonasschnelli commented Nov 25, 2018

Uh oh!

promag commented Nov 25, 2018

Uh oh!

maflcko Nov 26, 2018

Choose a reason for hiding this comment

Uh oh!

promag Nov 27, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko commented Nov 27, 2018

Uh oh!

Sjors commented Dec 6, 2018

Uh oh!

promag commented Dec 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

promag commented Nov 22, 2018 •

edited by laanwj

Loading

laanwj commented Nov 22, 2018 •

edited

Loading

Empact commented Nov 22, 2018 •

edited

Loading

laanwj commented Nov 23, 2018 •

edited

Loading

promag commented Nov 23, 2018 •

edited

Loading