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

Murmur: fix bad interaction with QDBus and fork(). #2821

Merged
merged 1 commit into from Feb 7, 2017

Conversation

@mkrautz
Copy link
Member

commented Feb 5, 2017

Qt 5.6 changed QDBus to use a thread, QDBusConnectionManager for
DBus connections.

This caused a bad interaction with Murmur. It turns out that, prior
to this commit, the QDBusConnectionManager in Murmur was launched at
program start, because of a global static QDBusConnection in DBus.cpp.

This meant that there would be a QDBusConnectionManager thread around,
waiting to manage DBus connections. ...Until Murmur calls fork().

After fork()ing, the QDBusConnectionManager thread is gone -- fork only
keeps a 'main' thread, and isn't generally compatible with multi-threaded
programs.

Ouch!

Fortunately, the static global QDBusConnection (MurmurDBus::qdbc) was
only initialized this way because it worked in the past. Also, because
QDBusConnection has no default constructor, the 'qdbc' QDBusConnection
was statically initialized as a QDBusConnection to 'mainbus' on each
program launch. This wasn't strictly necessary -- it was only done because
QDBusConnection had no default constructor. However, there is no such thing
as an 'empty' QDBusConnection, so there not being a default constructor
does make sense.

To avoid the static global QDBusConnection, this change introduces a
new type: MurmurDBusConnectionHolder. It's a global static as well,
but it's a pointer that holds a concrete QDBusConnection.

This allows us to defer the creation of the QDBusConnectionManager thread
until the first 'real' QDBusConnection is created -- after Murmur has forked.
This avoids the global static 'fake' QDBusConnection, and ensures the
QDBusConnectionManager thread is created after Murmur has forked.

Fixes #2820

@mkrautz mkrautz requested review from Kissaki and hacst Feb 5, 2017

@mkrautz mkrautz force-pushed the mkrautz:dbus-connection-holder branch from e0c9748 to 5ebe478 Feb 6, 2017

@@ -103,6 +103,12 @@ struct LogEntry {
Q_DECLARE_METATYPE(LogEntry);
Q_DECLARE_METATYPE(QList<LogEntry>);

class MurmurDBusConnectionHolder {

This comment has been minimized.

Copy link
@hacst

hacst Feb 6, 2017

Member

I'm not quite seeing the point of MurmurDBusConnectionHolder. The way it is written it seems we can copy construct QDBusConnection so why not use a QDBusConnection* directly? If we do not care about the precise timing of the cleanup we can make it a smart pointer and don't have to care about cleanup either. Am I missing something?

This comment has been minimized.

Copy link
@mkrautz

mkrautz Feb 7, 2017

Author Member

I guess I just wanted to avoid having to deref a pointer qdbc directly (*qdbc) in the QDBusInterface constructors... I'll try.

@@ -511,29 +511,29 @@ int main(int argc, char **argv) {

if (! Meta::mp.qsDBus.isEmpty()) {

This comment has been minimized.

Copy link
@hacst

hacst Feb 6, 2017

Member

Probably not something for this issue but we should think about moving that initialization code into the DBus specific file. main is bloated enough as it is.

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2017

There's also something broken where when running w/o dbus enabled. I get a crash.

@mkrautz mkrautz force-pushed the mkrautz:dbus-connection-holder branch from 5ebe478 to a3e1fd5 Feb 7, 2017

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2017

@hacst PTAL

Murmur: fix bad interaction with QDBus and fork().
Qt 5.6 changed QDBus to use a thread, QDBusConnectionManager,
for managing DBus connections.

This caused a bad interaction with Murmur. It turns out that, prior
to this commit, the QDBusConnectionManager in Murmur was launched at
program start, because of a global static QDBusConnection in DBus.cpp.

This meant that there would be a QDBusConnectionManager thread around,
waiting to manage DBus connections. ...Until Murmur calls fork().

After fork()ing, the QDBusConnectionManager thread is gone -- fork only
keeps a 'main' thread, and isn't generally compatible with multi-threaded
programs.

Ouch!

Fortunately, the static global QDBusConnection (MurmurDBus::qdbc) was
only initialized this way because it worked in the past. Also, because
QDBusConnection has no default constructor, the 'qdbc' QDBusConnection
was statically initialized as a QDBusConnection to 'mainbus' on each
program launch. This wasn't strictly necessary -- it was only done because
QDBusConnection had no default constructor. However, there is no such thing
as an 'empty' QDBusConnection, so there not being a default constructor
does make sense.

To avoid the static global QDBusConnection, this commit changes MurmuDBus::qdbc
from a value to a pointer.

This allows us to defer the creation of the QDBusConnectionManager thread
until the first 'real' QDBusConnection is created -- after Murmur has forked.
This avoids the global static 'fake' QDBusConnection, and ensures the
QDBusConnectionManager thread is created after Murmur has forked.

Additionally, this commit also moves the registration of the 'MetaDBus'
object into the '!Meta::mp.qsDBus.isEmpty()' check in main.cpp.
This ensures the MetaDBus object is only registered if the user has
enabled DBus in murmur.ini. Without this change, the code would be
dereferencing a null MurmurDBus::qdbc.

Fixes #2820

@mkrautz mkrautz force-pushed the mkrautz:dbus-connection-holder branch from a3e1fd5 to 1e9d2b3 Feb 7, 2017

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2017

OK, added another paragraph to the commit message about the move of MetaDBus registration.

@hacst
hacst approved these changes Feb 7, 2017
Copy link
Member

left a comment

LGTM

@mkrautz mkrautz merged commit d15c3f9 into mumble-voip:master Feb 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.