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

gui: Fix boost::signals2::no_slots_error in early calls to InitWarning #14783

Merged
merged 2 commits into from Dec 6, 2018

Conversation

Projects
None yet
8 participants
@promag
Copy link
Member

commented Nov 22, 2018

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.

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

This comment has been minimized.

Copy link
Member Author

commented Nov 22, 2018

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

@laanwj

This comment has been minimized.

Copy link
Member

commented Nov 22, 2018

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!

@fanquake fanquake added the GUI label Nov 22, 2018

@promag promag force-pushed the promag:2018-11-fix-noslotserror branch Nov 22, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Nov 22, 2018

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

@laanwj

This comment has been minimized.

Copy link
Member

commented Nov 22, 2018

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.

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

@promag promag force-pushed the promag:2018-11-fix-noslotserror branch 2 times, most recently Nov 22, 2018

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

@Empact

This comment has been minimized.

Copy link
Member

commented Nov 22, 2018

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

@promag

This comment has been minimized.

Copy link
Member Author

commented Nov 22, 2018

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

@MarcoFalke
Copy link
Member

left a comment

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)

src/init.cpp Outdated
}

// Warn if unrecognized section name are present in the config file.
for (const auto& section : gArgs.GetUnrecognizedSections()) {
InitWarning(strprintf(_("Section [%s] is not recognized."), section));
fprintf(stderr, "Warning: Section [%s] is not recognized.\n", section.c_str());

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Nov 22, 2018

Member

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

@promag promag force-pushed the promag:2018-11-fix-noslotserror branch to a0f8df3 Nov 22, 2018

@promag

This comment has been minimized.

Copy link
Member Author

commented Nov 22, 2018

Previous fix in 91ce5a2, which @MarcoFalke disapproved.

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

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

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

Concept ACK

@promag

This comment has been minimized.

Copy link
Member Author

commented Nov 22, 2018

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

@MarcoFalke
Copy link
Member

left a comment

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

@laanwj

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Nov 23, 2018

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

@promag

This comment has been minimized.

Copy link
Member Author

commented Nov 23, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Nov 23, 2018

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.

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

@promag promag force-pushed the promag:2018-11-fix-noslotserror branch to 6bbdb20 Nov 24, 2018

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Nov 25, 2018

Concept ACK

@promag

This comment has been minimized.

Copy link
Member Author

commented Nov 25, 2018

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

@MarcoFalke done.

std::unique_ptr<interfaces::Handler> handler_message_box = node->handleMessageBox(noui_ThreadSafeMessageBox);
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

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Nov 26, 2018

Member

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

This comment has been minimized.

Copy link
@promag

promag Nov 27, 2018

Author Member

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

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

@Sjors

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2018

@Sjors thanks, and the improvement can come next.

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Dec 6, 2018

Merge bitcoin#14783: gui: Fix boost::signals2::no_slots_error in earl…
…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

@MarcoFalke MarcoFalke merged commit 6bbdb20 into bitcoin:master Dec 6, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.