Initialise DBus notifications in another thread #152

luke-jr commented at 8:38 PM on December 14, 2020: member

QDBusInterface's constructor can hang if org.freedesktop.Notifications is missing, so avoid delaying startup waiting for a timeout

luke-jr force-pushed on Dec 15, 2020

in src/qt/notificator.cpp:44 in 0530dd0156 outdated

  39 | +        return;
  40 | +    }
  41 | +    interface->moveToThread(m_notificator.thread());
  42 | +    m_notificator.interface = interface;
  43 | +    m_notificator.mode = Notificator::Freedesktop;
  44 | +}

icota commented at 11:09 AM on December 28, 2020:

Emit finished here and clean up instead of keeping the thread for the Notificator lifecycle? DBusInitThread name implies it's disposable as soon as init is done.

luke-jr commented at 2:02 AM on February 1, 2021:

Isn't that supposed to be an automatic part of Qt? The docs say the user can't emit it...

icota commented at 9:52 PM on February 19, 2021:

You're right. I think it used to be public signal but seems that is no longer the case.

Talkless commented at 1:38 PM on March 13, 2021:

I'd say move m_dbus_init_thread->wait(); call into ~DBusInitThread(), so it no longer can be possible to misuse (forget). Also, connecting finished() signal could perform cleanup:

m_dbus_init_thread = new DBusInitThread(*this);
connect(m_dbus_init_thread, &DBusInitThread::finished, this, [this]() {
   m_dbus_init_thread->deleteLater();
   m_dbus_init_thread = nullptr;
});
m_dbus_init_thread->start();

hebasto commented at 6:48 PM on January 10, 2021: member

QDBusInterface's constructor can hang if org.freedesktop.Notifications is missing

Qt docs says:

If the remote service service is not present ..., the object created will not be valid...

GUI: Initialise DBus notifications in another thread

QDBusInterface's constructor can hang if org.freedesktop.Notifications is missing, so avoid delaying startup waiting for a timeout

a9f3096e73

luke-jr force-pushed on Feb 1, 2021

in src/qt/notificator.h:92 in a9f3096e73

  88 |      QSystemTrayIcon *trayIcon;
  89 |  #ifdef USE_DBUS
  90 | +    QThread *m_dbus_init_thread{nullptr};
  91 | +protected:
  92 |      QDBusInterface *interface;
  93 | +    friend class DBusInitThread;

Talkless commented at 2:01 PM on March 13, 2021:

friend could be avoided if DBusInitThread would have interfaceCreated(QDBusInterface* interface) signal.

Signal can be invoked from within run() using:

QTimer::singelShot(0, this, [this, interface]() {
  emit interfaceCreated(interface);
});

singleShot will detect that this (context) lives in different thread and lambda will be invoked in that (receiver) context thread (where DBusInitThread was created).

In the result, std::atomic<Mode> mode; "atomicity" will not be needed, because

  this->interface = interface;
  mode = Notificator::Freedesktop;

..would be performed on thread Notificator lives, within a slot (or just lambda) that connected to the DBusInitThread::interfaceCreated(QDBusInterface*) signal, and so without any race condition, as we are setting two variables, and only one is atomic.

DBusInitThread constructor will have to get QThread* though for moveToThread().. Or jus use DBusInitThread::thread() to get that "destination" thread? But that's might break if interface-receiving thread is not the same that created DBusInitThread object...

Also, adding interface->setParent(this) after this->interface = interface; would remove need for manual delete, unless there's deletion ordering is involved...

Talkless commented at 2:02 PM on March 13, 2021: none

Concept ACK.

DrahtBot commented at 6:27 PM on April 6, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	Talkless

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#686 (clang-tidy: Force checks for headers in src/qt by hebasto)

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.

hebasto renamed this:
~~GUI: Initialise DBus notifications in another thread~~
Initialise DBus notifications in another thread
on Apr 15, 2022

hebasto commented at 9:26 PM on May 19, 2022: member

A related discussion on IRC:

<luke-jr> oh, I guess Core never merged the dbus timeout fix - that's probably why -qt is slower <luke-jr> #152 <hebasto> luke-jr: it is not obvious that bug exist, in first place -- #152 (comment) <luke-jr> hebasto: it definitely exists <hebasto> how to reproduce it reliably? <luke-jr> idk, happens every time for me <luke-jr> maybe running bitcoin-qt as another user? <hebasto> ok, could you describe your steps and observations in your PR? <luke-jr> there's no steps, it just happens <hebasto> at least, you could mention your system on which "org.freedesktop.Notifications is missing"

DrahtBot commented at 12:27 PM on January 17, 2023: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the label Needs rebase on Jan 17, 2023

DrahtBot commented at 12:26 AM on April 17, 2023: contributor

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

hebasto commented at 4:12 PM on April 17, 2023: member

Closing this due to lack of activity. Feel free to reopen.

hebasto closed this on Apr 17, 2023

bitcoin-core locked this on Apr 16, 2024