Merge lp:~aacid/thumbnailer/fix_dbus_blocking into lp:thumbnailer

[Albert Astals Cid]

Use own QDBusConnection

Otherwise we're using the main one that has been created in the main thread and when doing blocking calls on it, it will block on the main thread instead of in the image provided thread as we want

To reproduce:
 * Have a considerable amount of Music in `xdg-user-dir MUSIC`
 * rm -rf ~/.cache/media-art/*
 * Open the My Music scope

See how UI blocks for various seconds without this patch and it's much smoother with it.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:120
http://jenkins.qa.ubuntu.com/job/thumbnailer-ci/140/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-vivid-amd64-ci/34
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-vivid-armhf-ci/32
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-vivid-armhf-ci/32/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-vivid-i386-ci/29

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/thumbnailer-ci/140/rebuild

review: Approve (continuous-integration)

[Jussi Pakkanen (jpakkane)]

Seems reasonable. Thanks.

review: Approve

[Pete Woods (pete-woods)]

The only question I have here, is whether we could just use an asynchronous DBus call, and a QDBusPendingCallWatcher. I thought that was the normal way to achieve async DBus, and doesn't introduce the overheads and risk of going multi-threaded.

[Michi Henning (michihenning)]

Albert, I have no objections for this as a short-term fix, if it gets you off the hook for the time being. I agree with Pete's sentiment though: if there are other requests made via the session bus from somewhere else, won't we be sending concurrent invocations into the library? (I don't know dbus well enough yet to be sure, can you help me out here?)

The library should be thread-safe, but I'm not sure whether that has been tested to any extent. Given that, an async call would probably be the more prudent way to go.

As a general remark: please don't use raw pointers. If the word "delete" appears anywhere in the code, it's a sure-fire sign that something is wrong somewhere, or will become wrong in the future (unless the code implements a resource manager or allocator). Instead, use unique_ptr or unity::util::ResourcePtr. This guarantees that the resource will be deallocated, no matter what. Without this, even if the code can't throw today, someone may unwittingly modify it later, and a single exception that escapes from the bowels of the code will wreak havoc.

Question: Is there a particular reason to have the constructor set the two pointers to null and then lazily initialise them later? Why not just initialise in the constructor? For example, the lazy initialisation isn't thread-safe. Even if thread-safety isn't a concern, I still prefer constructors that create fully-initialised instances. That way, there cannot be any surprises (such as someone modifying the code later and forgetting to check whether the member has been initialised), and things are thread-safe for free.

review: Needs Fixing

[James Henstridge (jamesh)]

Seconding Pete's suggestion, can we just use the PendingCallWatcher interface? Looking at the docs, the following should do the trick:

    QDBusPendingCallWatcher watcher(iface.asyncCall(GET_ALBUM_ART, ...));
    watcher.waitForFinished()
    QDBusPendingReply<QDBusUnixFileDescriptor> reply(watcher);

Assuming other code is playing nice with the main connection this should also solve the problem, right?

[Albert Astals Cid (aacid)]

> The only question I have here, is whether we could just use an asynchronous
> DBus call, and a QDBusPendingCallWatcher. I thought that was the normal way to
> achieve async DBus, and doesn't introduce the overheads and risk of going
> multi-threaded.

You can't do an asynchronous DBus call, you need to return the image in the requestImage function of the image provider.

[Albert Astals Cid (aacid)]

> Albert, I have no objections for this as a short-term fix, if it gets you off
> the hook for the time being. I agree with Pete's sentiment though: if there
> are other requests made via the session bus from somewhere else, won't we be
> sending concurrent invocations into the library? (I don't know dbus well
> enough yet to be sure, can you help me out here?)

Since there's no guarantee there's a single binary using this dbus service, how is this an issue? The dbus service should be able to handle multiple calls.

>
> The library should be thread-safe, but I'm not sure whether that has been
> tested to any extent. Given that, an async call would probably be the more
> prudent way to go.

Again, how is an async call more prudent? It would just exercise the library by doing multiple calls too, so don't see how it is a [new] problem

> As a general remark: please don't use raw pointers. If the word "delete"
> appears anywhere in the code, it's a sure-fire sign that something is wrong
> somewhere, or will become wrong in the future (unless the code implements a
> resource manager or allocator). Instead, use unique_ptr or
> unity::util::ResourcePtr. This guarantees that the resource will be
> deallocated, no matter what. Without this, even if the code can't throw today,
> someone may unwittingly modify it later, and a single exception that escapes
> from the bowels of the code will wreak havoc.

You must be joking right? You're telling me i can't call delete from a destructor? I'm not changing those pointers to silly pointers without a good reason for it.

> Question: Is there a particular reason to have the constructor set the two
> pointers to null and then lazily initialise them later? Why not just
> initialise in the constructor?

Because the constructor happens in the wrong thread and thus the dbus objects would belong to the main thread and still block on the main thread.

> For example, the lazy initialisation isn't
> thread-safe. Even if thread-safety isn't a concern, I still prefer
> constructors that create fully-initialised instances. That way, there cannot
> be any surprises (such as someone modifying the code later and forgetting to
> check whether the member has been initialised), and things are thread-safe for
> free.

[Albert Astals Cid (aacid)]

> Seconding Pete's suggestion, can we just use the PendingCallWatcher interface?
> Looking at the docs, the following should do the trick:
>
> QDBusPendingCallWatcher watcher(iface.asyncCall(GET_ALBUM_ART, ...));
> watcher.waitForFinished()
> QDBusPendingReply<QDBusUnixFileDescriptor> reply(watcher);
>
> Assuming other code is playing nice with the main connection this should also
> solve the problem, right?

This is exactly the same than the original code was wrottin, you're just blocking on waitForFinished() instead of in call(), what do you think this gives you?

[James Henstridge (jamesh)]

> This is exactly the same than the original code was wrottin, you're just
> blocking on waitForFinished() instead of in call(), what do you think this
> gives you?

Are you sure about that? I tried pulling the code out into a simple test app at http://paste.ubuntu.com/10525179/, and it looks like the event loop continues while the other thread waits on the async reply.

[Albert Astals Cid (aacid)]

James, your code still blocks dbus on the main thread, see http://paste.ubuntu.com/10525875/

Idle A 0x7f7e219d0780 106
Idle B 0x7f7e219d0780 107
Idle A 0x7f7e219d0780 211
Idle B 0x7f7e219d0780 211
Idle A 0x7f7e219d0780 316
Idle B 0x7f7e219d0780 316
Idle A 0x7f7e219d0780 421
Idle B 0x7f7e219d0780 423
Idle A 0x7f7e219d0780 526
Idle B 0x7f7e219d0780 527
Idle A 0x7f7e219d0780 631
Idle B 0x7f7e219d0780 632
Idle A 0x7f7e219d0780 736
Idle B 0x7f7e219d0780 738
Idle A 0x7f7e219d0780 841
Idle B 0x7f7e219d0780 843
Idle A 0x7f7e219d0780 943
Idle B 0x7f7e219d0780 945
Making async D-Bus call (thread=0x7f7e1cd89700)
Idle A 0x7f7e219d0780 1043
Got D-Bus reply (thread=0x7f7e1cd89700)
Idle B 0x7f7e219d0780 1838

[Albert Astals Cid (aacid)]

That's why we need our own connection http://paste.ubuntu.com/10525893/

Idle A 0x7ff9be0d3780 95
Idle B 0x7ff9be0d3780 96
Idle A 0x7ff9be0d3780 190
Idle B 0x7ff9be0d3780 191
Idle A 0x7ff9be0d3780 288
Idle B 0x7ff9be0d3780 289
Idle A 0x7ff9be0d3780 387
Idle B 0x7ff9be0d3780 389
Idle A 0x7ff9be0d3780 487
Idle B 0x7ff9be0d3780 489
Idle A 0x7ff9be0d3780 587
Idle B 0x7ff9be0d3780 588
Idle A 0x7ff9be0d3780 687
Idle B 0x7ff9be0d3780 688
Idle A 0x7ff9be0d3780 787
Idle B 0x7ff9be0d3780 788
Idle A 0x7ff9be0d3780 887
Idle B 0x7ff9be0d3780 889
Idle A 0x7ff9be0d3780 987
Idle B 0x7ff9be0d3780 988
Making async D-Bus call (thread=0x7ff9b948c700)
Idle A 0x7ff9be0d3780 1087
Idle B 0x7ff9be0d3780 1088
Idle A 0x7ff9be0d3780 1187
Idle B 0x7ff9be0d3780 1189
Idle A 0x7ff9be0d3780 1287
Idle B 0x7ff9be0d3780 1288
Idle A 0x7ff9be0d3780 1387
Idle B 0x7ff9be0d3780 1389
Idle A 0x7ff9be0d3780 1487
Idle B 0x7ff9be0d3780 1488
Idle A 0x7ff9be0d3780 1587
Idle B 0x7ff9be0d3780 1589
Idle A 0x7ff9be0d3780 1687
Idle B 0x7ff9be0d3780 1688
Idle A 0x7ff9be0d3780 1787
Idle B 0x7ff9be0d3780 1788
Got D-Bus reply (thread=0x7ff9b948c700)

[James Henstridge (jamesh)]

Okay. I still don't quite understand why this is causing D-Bus calls in the event loop thread to block, but it clearly is. So I guess having a second connection is necessary here.

[Michi Henning (michihenning)]

> You must be joking right?

No, not really.

> You're telling me i can't call delete from a
> destructor?

No. I'm suggesting that delete is almost always a bad idea and that it is almost never necessary.

> I'm not changing those pointers to silly pointers without a good
> reason for it.

Not sure why you called it a silly pointer? With unique_ptr, the constructor does not need to explicitly initialize the members, and no destructor is needed at all. That's less code, and the code is always safe.

> > Question: Is there a particular reason to have the constructor set the two
> > pointers to null and then lazily initialise them later? Why not just
> > initialise in the constructor?
>
> Because the constructor happens in the wrong thread and thus the dbus objects
> would belong to the main thread and still block on the main thread.

Ah, OK, thanks! A comment to that effect at the point of initialization would be great then.

[lp:~aacid/thumbnailer/fix_dbus_blocking]

121. By Albert Astals Cid on 2015-03-05

Comment++

122. By Albert Astals Cid on 2015-03-05

pointer nit picking

123. By Albert Astals Cid on 2015-03-05

more

[Albert Astals Cid (aacid)]

Updated

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:123
http://jenkins.qa.ubuntu.com/job/thumbnailer-ci/141/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-vivid-amd64-ci/35
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-vivid-armhf-ci/33
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-vivid-armhf-ci/33/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-vivid-i386-ci/30

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/thumbnailer-ci/141/rebuild

review: Approve (continuous-integration)

[Michi Henning (michihenning)]

Thank you!

review: Approve

[Victor Thompson (vthompson)]

This fixes a bug filed against the music-app (lp:1428944) that has since been marked as a dupe. As a community reviewer, I approve this change from a functional standpoint since it remedies the UI blocking seen in the music app.

review: Approve

=== modified file 'plugins/Ubuntu/Thumbnailer/albumartgenerator.cpp'
--- plugins/Ubuntu/Thumbnailer/albumartgenerator.cpp	2014-09-24 16:25:31 +0000
+++ plugins/Ubuntu/Thumbnailer/albumartgenerator.cpp	2015-03-05 08:47:43 +0000
@@ -34,8 +34,8 @@
 static const char GET_ALBUM_ART[] = "GetAlbumArt";
 
 AlbumArtGenerator::AlbumArtGenerator()
-    : QQuickImageProvider(QQuickImageProvider::Image, QQmlImageProviderBase::ForceAsynchronousImageLoading),
-      iface(BUS_NAME, BUS_PATH, THUMBNAILER_IFACE) {
+    : QQuickImageProvider(QQuickImageProvider::Image, QQmlImageProviderBase::ForceAsynchronousImageLoading)
+{
 }
 
 static QImage fallbackImage(QSize *realSize) {
@@ -53,13 +53,19 @@
         return fallbackImage(realSize);
     }
 
+    if (!connection) {
+        // Create them here and not them on the constrcutor so they belong to the proper thread
+        connection.reset(new QDBusConnection(QDBusConnection::connectToBus(QDBusConnection::SessionBus, "album_art_generator_dbus_connection")));
+        iface.reset(new QDBusInterface(BUS_NAME, BUS_PATH, THUMBNAILER_IFACE, *connection));
+    }
+
     const QString artist = query.queryItemValue("artist", QUrl::FullyDecoded);
     const QString album = query.queryItemValue("album", QUrl::FullyDecoded);
 
     QString desiredSize = sizeToDesiredSizeString(requestedSize);
 
     // perform dbus call
-    QDBusReply<QDBusUnixFileDescriptor> reply = iface.call(
+    QDBusReply<QDBusUnixFileDescriptor> reply = iface->call(
         GET_ALBUM_ART, artist, album, desiredSize);
     if (!reply.isValid()) {
         qWarning() << "D-Bus error: " << reply.error().message();

=== modified file 'plugins/Ubuntu/Thumbnailer/albumartgenerator.h'
--- plugins/Ubuntu/Thumbnailer/albumartgenerator.h	2014-03-28 04:08:24 +0000
+++ plugins/Ubuntu/Thumbnailer/albumartgenerator.h	2015-03-05 08:47:43 +0000
@@ -23,10 +23,13 @@
 #include <QDBusInterface>
 #include <QQuickImageProvider>
 
+#include <memory>
+
 class AlbumArtGenerator: public QQuickImageProvider
 {
 private:
-    QDBusInterface iface;
+    std::unique_ptr<QDBusConnection> connection;
+    std::unique_ptr<QDBusInterface> iface;
 
 public:
     AlbumArtGenerator();

=== modified file 'plugins/Ubuntu/Thumbnailer/artistartgenerator.cpp'
--- plugins/Ubuntu/Thumbnailer/artistartgenerator.cpp	2014-09-24 16:25:31 +0000
+++ plugins/Ubuntu/Thumbnailer/artistartgenerator.cpp	2015-03-05 08:47:43 +0000
@@ -35,8 +35,8 @@
 static const char GET_ARTIST_ART[] = "GetArtistArt";
 
 ArtistArtGenerator::ArtistArtGenerator()
-    : QQuickImageProvider(QQuickImageProvider::Image, QQmlImageProviderBase::ForceAsynchronousImageLoading),
-      iface(BUS_NAME, BUS_PATH, THUMBNAILER_IFACE) {
+    : QQuickImageProvider(QQuickImageProvider::Image, QQmlImageProviderBase::ForceAsynchronousImageLoading)
+{
 }
 
 static QImage fallbackImage(QSize *realSize) {
@@ -54,13 +54,19 @@
         return fallbackImage(realSize);
     }
 
+    if (!connection) {
+        // Create them here and not them on the constrcutor so they belong to the proper thread
+        connection.reset(new QDBusConnection(QDBusConnection::connectToBus(QDBusConnection::SessionBus, "artist_art_generator_dbus_connection")));
+        iface.reset(new QDBusInterface(BUS_NAME, BUS_PATH, THUMBNAILER_IFACE, *connection));
+    }
+
     const QString artist = query.queryItemValue("artist", QUrl::FullyDecoded);
     const QString album = query.queryItemValue("album", QUrl::FullyDecoded);
 
     QString desiredSize = sizeToDesiredSizeString(requestedSize);
 
     // perform dbus call
-    QDBusReply<QDBusUnixFileDescriptor> reply = iface.call(
+    QDBusReply<QDBusUnixFileDescriptor> reply = iface->call(
         GET_ARTIST_ART, artist, album, desiredSize);
     if (!reply.isValid()) {
         qWarning() << "D-Bus error: " << reply.error().message();

=== modified file 'plugins/Ubuntu/Thumbnailer/artistartgenerator.h'
--- plugins/Ubuntu/Thumbnailer/artistartgenerator.h	2014-08-01 10:44:40 +0000
+++ plugins/Ubuntu/Thumbnailer/artistartgenerator.h	2015-03-05 08:47:43 +0000
@@ -22,10 +22,13 @@
 #include <QDBusInterface>
 #include <QQuickImageProvider>
 
+#include <memory>
+
 class ArtistArtGenerator: public QQuickImageProvider
 {
 private:
-    QDBusInterface iface;
+    std::unique_ptr<QDBusConnection> connection;
+    std::unique_ptr<QDBusInterface> iface;
 
 public:
     ArtistArtGenerator();