Merge lp:~lukas-kde/gsettings-qt/queued-processing into lp:gsettings-qt

[Lukáš Tinkl]

Force handling deferred delete events to avoid memory leaks

DeferredDelete events that are posted from a handler directly dispatched by glib are not run until the program exits, because Qt doesn't see them as being in the right main loop level. Force handling them here to avoid leaks until there's a fix to https://bugreports.qt.io/browse/QTBUG-32859

[Lars Karlitski (larsu)]

Thanks for the patch.

Which two event loops are you talking about? Do we have a memory leak without this change? Please provide more detail.

> the latter doesn't work reliably with the in-memory backend

In what way is this unreliable? I added the changed signal because valueChanged is not emitted in all cases [1].

Also, please separate the unrelated changes into a separate commit.

[1] http://bazaar.launchpad.net/~system-settings-touch/gsettings-qt/trunk/revision/19

review: Needs Information

[Lukáš Tinkl (lukas-kde)]

> Thanks for the patch.
>
> Which two event loops are you talking about? Do we have a memory leak without
> this change? Please provide more detail.

There's a glib event loop handling integrated in Qt; we don't have a memory leak exactly but as
with mzanetti's previous work, the resulting QML Components are never freed when using a Loader.

> > the latter doesn't work reliably with the in-memory backend
>
> In what way is this unreliable? I added the changed signal because
> valueChanged is not emitted in all cases [1].

Try running the testcase, if you use the valueChanged() signal, it indeed does get emitted in all cases (unlike what the documentation says unfortunately)

> [1] http://bazaar.launchpad.net/~system-settings-touch/gsettings-
> qt/trunk/revision/19

I removed the updateKey() declaration because the implementation seems to be gone, was that an oversight?

[Lukáš Tinkl (lukas-kde)]

> In what way is this unreliable? I added the changed signal because

To clarify this a bit further, and this is just for the testcase, it seems that the changed signal is not emitted when using the in-memory backend gsettings backend.

[Michael Zanetti (mzanetti)]

Hi Lars, after some investigation we're quite sure that the changed events from gsettings are not posted to the normal Qt event loop but instead handled in a different context. This can cause all sorts of issues in Qt, one of them is the Loader issue we've been discussing in Belgium. Maybe you can think of a cleaner way to do it, but I really think we need this detaching of the signal emissions somehow.

[Lars Karlitski (larsu)]

I know this is on your plate right now and possibly blocking other stuff, but I can't in good conscience merge something based on conjecture.

There's a lot of integration points with glib. If there's some underlying issue with how qt uses the main loop, we ought to at least know what's going on.

Can you please provide that test case again and a more detailed explanation of what you've found so far? I might be able to have another look at this soon.

Sorry...

[Michael Zanetti (mzanetti)]

Here's the example: http://paste.ubuntu.com/11494214/

Run it and then use

gsettings set com.canonical.Unity8 usage-mode Windowed

and

gsettings set com.canonical.Unity8 usage-mode Staged

To switch between them. See how the loader leaks its item with every toggle. With everything else the Loader behaves correctly, it's really just an issue when it's hooked up to gsettings because the changed event happens in a different context than what Qt expects.

[Lars Karlitski (larsu)]

Thanks. I've investigated a bit and found that QEventLoop has a concept of "levels" and deferred delete events are only processed for when a given level finishes running. Calling deleteLater() from the gsettings changed signal (which runs in the level 0) makes the deferred event not run until the program exits.

There's a qt bug open about this, from ~nick-dedekind:

  https://bugreports.qt.io/browse/QTBUG-32859

Queuing the signal fixes the bug as that queues it onto the correct level. That's an ok workaround, but I'd like to be more explicit and call

  QCoreApplication::sendPostedEvents(NULL, QEvent::DeferredDelete);

in GSettingsQml::settingChanged, with a comment explaining why we need to do this and a link to the upstream bug.

Of course, I'd much prefer if someone fixed that upstream ;)

review: Needs Fixing

[Lukáš Tinkl (lukas-kde)]

OK, fixed using QCoreApplication::sendPostedEvents(), auto tests passing, and solves the original issue as well.

[Lars Karlitski (larsu)]

Thanks for the quick fix.

There's nothing to sync up with. Please amend the comment to something like this: "DeferredDelete events that are posted from a handler directly dispatched by glib are not run until the program exits, because Qt doesn't see them as being in the right main loop level. Force handling them here to avoid leaks until there's a fix to https://bugreports.qt.io/browse/QTBUG-32859"

And change the description and commit message of this merge request.

[Lars Karlitski (larsu)]

Thanks!

review: Approve

[Nick Dedekind (nick-dedekind)]

Hm. not entirely sure about this.

Doesn't sending posted DeleteLater events directly break the semantics of DeleteLater?
In unitymenumodel I fixed it by queuing the events from glib loop. I'm not sure if gsetting uses synchronous sets or not? (ie. a set synchronously causes an update which we could rely on?) maybe larsu can help here.

Maybe call QMetaObject::invoke(self, "settingChanged", Qt::QueuedConnection) in qgsettings.cpp?

review: Needs Information

[Lars Karlitski (larsu)]

> Hm. not entirely sure about this.
>
> Doesn't sending posted DeleteLater events directly break the semantics of
> DeleteLater?

You're right, it does. Thanks for pointing that out.

> Maybe call QMetaObject::invoke(self, "settingChanged", Qt::QueuedConnection)
> in qgsettings.cpp?

I didn't know you could force queued connections. Let's do that.

[lp:~lukas-kde/gsettings-qt/queued-processing]

75. By Lukáš Tinkl on 2015-06-02

add a C++ test for the deferred deletion case

76. By Lukáš Tinkl on 2015-06-02

rename C++ test to cpptest (both target and the source)

[Nick Dedekind (nick-dedekind)]

Code looks good.
Tests pass.

All fine my me.

review: Approve

[Lars Karlitski (larsu)]

Very cool. Thanks for chiming in Nick.

review: Approve

[lp:~lukas-kde/gsettings-qt/queued-processing]

77. By Lukáš Tinkl on 2015-07-28

fix building the Cpp test in CI

=== modified file 'GSettings/gsettings-qml.cpp'
--- GSettings/gsettings-qml.cpp	2014-06-23 14:55:54 +0000
+++ GSettings/gsettings-qml.cpp	2015-07-28 14:56:19 +0000
@@ -137,7 +137,7 @@
 
         connect(priv->settings, SIGNAL(changed(const QString &)), this, SLOT(settingChanged(const QString &)));
 
-        Q_FOREACH(QString key, priv->settings->keys())
+        Q_FOREACH(const QString &key, priv->settings->keys())
             this->insert(key, priv->settings->get(key));
 
         Q_EMIT(schemaChanged());

=== modified file 'GSettings/gsettings-qml.h'
--- GSettings/gsettings-qml.h	2014-04-08 09:28:52 +0000
+++ GSettings/gsettings-qml.h	2015-07-28 14:56:19 +0000
@@ -61,8 +61,6 @@
 
     GSettingsSchemaQml * schema() const;
 
-    void updateKey(const char *gkey, bool emitChanged);
-
     void classBegin();
     void componentComplete();
 

=== modified file 'gsettings-qt.pro'
--- gsettings-qt.pro	2013-07-19 14:22:57 +0000
+++ gsettings-qt.pro	2015-07-28 14:56:19 +0000
@@ -1,2 +1,2 @@
 TEMPLATE = subdirs
-SUBDIRS += src/gsettings-qt.pro GSettings/gsettings-qt.pro tests/tests.pro
+SUBDIRS += src/gsettings-qt.pro GSettings/gsettings-qt.pro tests/tests.pro tests/cpptest.pro

=== modified file 'src/qgsettings.cpp'
--- src/qgsettings.cpp	2014-06-23 13:29:16 +0000
+++ src/qgsettings.cpp	2015-07-28 14:56:19 +0000
@@ -36,7 +36,8 @@
 {
     QGSettings *self = (QGSettings *)user_data;
 
-    Q_EMIT(self->changed(qtify_name(key)));
+    // work around https://bugreports.qt.io/browse/QTBUG-32859 and http://pad.lv/1460970
+    QMetaObject::invokeMethod(self, "changed", Qt::QueuedConnection, Q_ARG(QString, qtify_name(key)));
 }
 
 QGSettings::QGSettings(const QByteArray &schema_id, const QByteArray &path, QObject *parent):

=== added file 'tests/cpptest.cpp'
--- tests/cpptest.cpp	1970-01-01 00:00:00 +0000
+++ tests/cpptest.cpp	2015-07-28 14:56:19 +0000
@@ -0,0 +1,35 @@
+#include <QtTest/QtTest>
+
+#include <QGSettings>
+
+class TestDeferredDelete: public QObject
+{
+    Q_OBJECT
+private slots:
+    void initTestCase();
+    void test_deferredDelete();
+private:
+    QGSettings * settings;
+    QPointer<QObject> dummy;
+};
+
+void TestDeferredDelete::initTestCase()
+{
+    settings = new QGSettings("com.canonical.gsettings.Test", QByteArray(), this);
+    dummy = new QObject;
+    connect(settings, &QGSettings::changed, dummy.data(), &QObject::deleteLater); // delete the dummy object upon any gsettings change
+}
+
+void TestDeferredDelete::test_deferredDelete()
+{
+    QSignalSpy spy(dummy.data(), &QObject::destroyed); // watch the dummy object get destroyed
+    settings->set("testString", "bar");
+
+    QVERIFY(spy.wait(1));
+    QVERIFY(dummy.isNull()); // verify dummy got destroyed for real
+    QCOMPARE(settings->get("testString").toString(), QStringLiteral("bar")); // also verify the setting got written by reading it back
+}
+
+QTEST_MAIN(TestDeferredDelete)
+
+#include "cpptest.moc"

=== added file 'tests/cpptest.pro'
--- tests/cpptest.pro	1970-01-01 00:00:00 +0000
+++ tests/cpptest.pro	2015-07-28 14:56:19 +0000
@@ -0,0 +1,18 @@
+TEMPLATE = app
+QT += testlib
+QT -= gui
+CONFIG += testcase link_pkgconfig
+TARGET = cpptest
+IMPORTPATH = $$PWD/..
+SOURCES = cpptest.cpp
+INCLUDEPATH += $$(PWD)/../src
+LIBS += -L$$(PWD)/../src -lgsettings-qt
+
+schema.target = gschemas.compiled
+schema.commands = glib-compile-schemas $$PWD
+schema.depends = com.canonical.gsettings.test.gschema.xml
+QMAKE_EXTRA_TARGETS += schema
+PRE_TARGETDEPS = gschemas.compiled
+
+# qmake prepends this to the command line executed by `make check`
+check.commands = QT_QPA_PLATFORM=minimal LD_LIBRARY_PATH=$$PWD/../src GSETTINGS_BACKEND=memory GSETTINGS_SCHEMA_DIR=$$PWD

=== modified file 'tests/tst_GSettings.qml'
--- tests/tst_GSettings.qml	2014-08-01 16:47:07 +0000
+++ tests/tst_GSettings.qml	2015-07-28 14:56:19 +0000
@@ -11,8 +11,14 @@
   GSettings {
     id: settings
     schema.id: "com.canonical.gsettings.Test"
+    // has to be "valueChanged" signal, not "changed"; the latter doesn't work reliably with the in-memory gsettings backend
+    onValueChanged: changes.push([key, value]);
+  }
 
-    onChanged: changes.push([key, value]);
+  SignalSpy {
+    id: changesSpy
+    target: settings
+    signalName: "changed"
   }
 
   GSettings {
@@ -90,6 +96,7 @@
   function test_reset() {
     settings.testInteger = 4;
     settings.schema.reset('testInteger');
+    tryCompare(changesSpy, "count", 1); // this merely exists only to connect to the "changed" signal; resetting changes is async in gsettings
     compare(settings.testInteger, 42);
   }