diff options
author | David Faure <david.faure@kdab.com> | 2024-03-12 13:18:02 +0100 |
---|---|---|
committer | David Faure <david.faure@kdab.com> | 2024-03-19 00:55:15 +0100 |
commit | c837cd75936cbeeb898dd5808edb9dfaf716a76e (patch) | |
tree | f01ff889e20bb87f51516bf4c3fdce05ae0ea25c | |
parent | 2b2cd119e134699f5428f0eb5686fa9eaef67a57 (diff) |
QSignalSpy: fix data race between wait() and emit from another thread
Detected by TSAN in tst_QThread::terminateAndPrematureDestruction()
but better have a dedicated unittest, with values emitted by the signal
and recorded in the spy.
Pick-to: 6.7 6.6 6.5
Change-Id: I141d47b0ea0b63188f8a4f9d056f72df3457bda5
Reviewed-by: Marc Mutz <marc.mutz@qt.io>
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
-rw-r--r-- | src/testlib/qsignalspy.h | 27 | ||||
-rw-r--r-- | tests/auto/testlib/qsignalspy/tst_qsignalspy.cpp | 32 |
2 files changed, 52 insertions, 7 deletions
diff --git a/src/testlib/qsignalspy.h b/src/testlib/qsignalspy.h index b72c3b4b53..0c4ae347b8 100644 --- a/src/testlib/qsignalspy.h +++ b/src/testlib/qsignalspy.h @@ -10,6 +10,7 @@ #include <QtCore/qmetaobject.h> #include <QtTest/qtesteventloop.h> #include <QtCore/qvariant.h> +#include <QtCore/qmutex.h> QT_BEGIN_NAMESPACE @@ -43,11 +44,11 @@ public: return; } + initArgs(mo->method(sigIndex), obj); if (!connectToSignal(obj, sigIndex)) return; sig = ba; - initArgs(mo->method(sigIndex), obj); } #ifdef Q_QDOC @@ -73,21 +74,23 @@ public: if (!isSignalMetaMethodValid(signalMetaMethod)) return; + initArgs(mo->method(sigIndex), obj); if (!connectToSignal(obj, sigIndex)) return; sig = signalMetaMethod.methodSignature(); - initArgs(mo->method(sigIndex), obj); } #endif // Q_QDOC QSignalSpy(const QObject *obj, const QMetaMethod &signal) : m_waiting(false) { - if (isObjectValid(obj) && isSignalMetaMethodValid(signal) && - connectToSignal(obj, signal.methodIndex())) { - sig = signal.methodSignature(); + if (isObjectValid(obj) && isSignalMetaMethodValid(signal)) { initArgs(signal, obj); + if (!connectToSignal(obj, signal.methodIndex())) + return; + + sig = signal.methodSignature(); } } @@ -99,10 +102,15 @@ public: bool wait(std::chrono::milliseconds timeout = std::chrono::seconds{5}) { + QMutexLocker locker(&m_mutex); Q_ASSERT(!m_waiting); const qsizetype origCount = size(); m_waiting = true; + locker.unlock(); + m_loop.enterLoop(timeout); + + locker.relock(); m_waiting = false; return size() > origCount; } @@ -157,14 +165,17 @@ private: void initArgs(const QMetaMethod &member, const QObject *obj) { + QMutexLocker locker(&m_mutex); args.reserve(member.parameterCount()); for (int i = 0; i < member.parameterCount(); ++i) { QMetaType tp = member.parameterMetaType(i); if (!tp.isValid() && obj) { + locker.unlock(); void *argv[] = { &tp, &i }; QMetaObject::metacall(const_cast<QObject*>(obj), QMetaObject::RegisterMethodArgumentMetaType, member.methodIndex(), argv); + locker.relock(); } if (!tp.isValid()) { qWarning("QSignalSpy: Unable to handle parameter '%s' of type '%s' of method '%s'," @@ -179,6 +190,7 @@ private: void appendArgs(void **a) { + QMutexLocker locker(&m_mutex); QList<QVariant> list; list.reserve(args.size()); for (int i = 0; i < args.size(); ++i) { @@ -190,8 +202,10 @@ private: } append(list); - if (m_waiting) + if (m_waiting) { + locker.unlock(); m_loop.exitLoop(); + } } // the full, normalized signal name @@ -201,6 +215,7 @@ private: QTestEventLoop m_loop; bool m_waiting; + static inline QMutex m_mutex; // protects m_waiting, args and the QList base class, between appendArgs() and wait() }; QT_END_NAMESPACE diff --git a/tests/auto/testlib/qsignalspy/tst_qsignalspy.cpp b/tests/auto/testlib/qsignalspy/tst_qsignalspy.cpp index 40dd9b94de..847986a673 100644 --- a/tests/auto/testlib/qsignalspy/tst_qsignalspy.cpp +++ b/tests/auto/testlib/qsignalspy/tst_qsignalspy.cpp @@ -6,9 +6,11 @@ #include <QSignalSpy> #include <QTimer> - #include <qdatetime.h> +using namespace std::chrono_literals; +using namespace Qt::StringLiterals; + class tst_QSignalSpy : public QObject { Q_OBJECT @@ -48,6 +50,8 @@ private slots: void spyOnMetaMethod_invalid(); void spyOnMetaMethod_invalid_data(); + + void signalSpyDoesNotRaceOnCrossThreadSignal(); }; struct CustomType {}; @@ -485,5 +489,31 @@ void tst_QSignalSpy::spyOnMetaMethod_invalid_data() << QObject::staticMetaObject.method(QObject::staticMetaObject.indexOfMethod("deleteLater()")); } +class EmitSignal_Thread : public QThread +{ + Q_OBJECT +public: + void run() override + { + emit valueChanged(42, u"is the answer"_s); + } + +Q_SIGNALS: + void valueChanged(int value, const QString &str); +}; + +void tst_QSignalSpy::signalSpyDoesNotRaceOnCrossThreadSignal() +{ + EmitSignal_Thread thread; + QSignalSpy valueChangedSpy(&thread, &EmitSignal_Thread::valueChanged); + QVERIFY(valueChangedSpy.isValid()); + + thread.start(); + QVERIFY(valueChangedSpy.wait(5s)); + QCOMPARE(valueChangedSpy[0][0].toInt(), 42); + QCOMPARE(valueChangedSpy[0][1].toString(), u"is the answer"_s); + QVERIFY(thread.wait(5s)); +} + QTEST_MAIN(tst_QSignalSpy) #include "tst_qsignalspy.moc" |