From 4a756726ee874ff2ce496e1b113707c65c39a76b Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Mon, 18 May 2009 22:35:42 +0200 Subject: Use a per object lock for signal/slots That way we prevent the current possible race condition that may appears while touching signals slot while object are moved to different threads. For exemple, when we do sender->threadData->mutex->lock(); the sender can possibly be moved to another thread between the moment we get the pointer to the threadData and the moment we aquire the lock. We could check if we locked the right mutex and retry otherwise, but that would break if the threadData (and hence the mutex) get destroyed in between. The per object mutex is implemented with a thread pool. I'm not using the global QThreadPool because we might ends up locking two of their mutex, and that would be dangerous if something else holds a lock on the same mutex (possible deadlock) Putting the mutex pool in a Q_GLOBAL_STATIC doesn't work as it might result of the ppol being deleted before some other object in others Q_GLOBAL_STATIC structures There is no need to lock this mutex in moveToThread as this is safe. When emiting a signal, we do not need to lock the thread data, as the user must ensure that the object is not moved to a thread while emiting a AutoConnection signal. Reviewed-by: Brad --- src/corelib/kernel/qobject.cpp | 66 ++++++++++++++++++++---------- tests/auto/qobjectrace/tst_qobjectrace.cpp | 24 +++++++++-- 2 files changed, 65 insertions(+), 25 deletions(-) diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp index 9f803fa..cfd8493 100644 --- a/src/corelib/kernel/qobject.cpp +++ b/src/corelib/kernel/qobject.cpp @@ -58,6 +58,7 @@ #include #include +#include #include @@ -91,12 +92,35 @@ static int *queuedConnectionTypes(const QList &typeNames) return types; } +QBasicAtomicPointer signalSlotMutexes = Q_BASIC_ATOMIC_INITIALIZER(0); +QBasicAtomicInt objectCount = Q_BASIC_ATOMIC_INITIALIZER(0); + +/** \internal + * mutex to be locked when accessing the connectionlists or the senders list + */ +static QMutex *signalSlotLock(const QObject *o) +{ + if (!signalSlotMutexes) { + QMutexPool *mp = new QMutexPool; + if (!signalSlotMutexes.testAndSetOrdered(0, mp)) { + delete mp; + } + } + return signalSlotMutexes->get(o); +} + extern "C" Q_CORE_EXPORT void qt_addObject(QObject *) { + objectCount.ref(); } extern "C" Q_CORE_EXPORT void qt_removeObject(QObject *) { + if(!objectCount.deref()) { + QMutexPool *old = signalSlotMutexes; + signalSlotMutexes.testAndSetAcquire(old, 0); + delete old; + } } QObjectPrivate::QObjectPrivate(int version) @@ -223,7 +247,7 @@ bool QObjectPrivate::isSender(const QObject *receiver, const char *signal) const int signal_index = q->metaObject()->indexOfSignal(signal); if (signal_index < 0) return false; - QMutexLocker locker(&threadData->mutex); + QMutexLocker locker(signalSlotLock(q)); if (connectionLists) { if (signal_index < connectionLists->count()) { const ConnectionList &connectionList = connectionLists->at(signal_index); @@ -245,7 +269,7 @@ QObjectList QObjectPrivate::receiverList(const char *signal) const int signal_index = q->metaObject()->indexOfSignal(signal); if (signal_index < 0) return returnValue; - QMutexLocker locker(&threadData->mutex); + QMutexLocker locker(signalSlotLock(q)); if (connectionLists) { if (signal_index < connectionLists->count()) { const ConnectionList &connectionList = connectionLists->at(signal_index); @@ -263,7 +287,7 @@ QObjectList QObjectPrivate::receiverList(const char *signal) const QObjectList QObjectPrivate::senderList() const { QObjectList returnValue; - QMutexLocker locker(&threadData->mutex); + QMutexLocker locker(signalSlotLock(q_func())); for (int i = 0; i < senders.count(); ++i) returnValue << senders.at(i)->sender; return returnValue; @@ -712,7 +736,7 @@ QObject::~QObject() emit destroyed(this); { - QMutexLocker locker(&d->threadData->mutex); + QMutexLocker locker(signalSlotLock(this)); // set ref to zero to indicate that this object has been deleted if (d->currentSender != 0) @@ -731,7 +755,7 @@ QObject::~QObject() continue; } - QMutex *m = &c->receiver->d_func()->threadData->mutex; + QMutex *m = signalSlotLock(c->receiver); bool needToUnlock = QOrderedMutexLocker::relock(locker.mutex(), m); c = connectionList[i]; if (c->receiver) @@ -755,7 +779,7 @@ QObject::~QObject() for (int i = 0; i < d->senders.count(); ) { QObjectPrivate::Connection *s = d->senders[i]; - QMutex *m = &s->sender->d_func()->threadData->mutex; + QMutex *m = signalSlotLock(s->sender); bool needToUnlock = QOrderedMutexLocker::relock(locker.mutex(), m); if (m < locker.mutex()) { if (i >= d->senders.count() || s != d->senders[i]) { @@ -765,11 +789,9 @@ QObject::~QObject() } } s->receiver = 0; - if (s->sender) { - QObjectConnectionListVector *senderLists = s->sender->d_func()->connectionLists; - if (senderLists) - senderLists->dirty = true; - } + QObjectConnectionListVector *senderLists = s->sender->d_func()->connectionLists; + if (senderLists) + senderLists->dirty = true; if (needToUnlock) m->unlock(); @@ -2254,7 +2276,7 @@ QObject *QObject::sender() const { Q_D(const QObject); - QMutexLocker(&d->threadData->mutex); + QMutexLocker(signalSlotLock(this)); if (!d->currentSender) return 0; @@ -2310,7 +2332,7 @@ int QObject::receivers(const char *signal) const } Q_D(const QObject); - QMutexLocker locker(&d->threadData->mutex); + QMutexLocker locker(signalSlotLock(this)); if (d->connectionLists) { if (signal_index < d->connectionLists->count()) { const QObjectPrivate::ConnectionList &connectionList = @@ -2757,8 +2779,8 @@ bool QMetaObject::connect(const QObject *sender, int signal_index, c->connectionType = type; c->argumentTypes = types; - QOrderedMutexLocker locker(&s->d_func()->threadData->mutex, - &r->d_func()->threadData->mutex); + QOrderedMutexLocker locker(signalSlotLock(sender), + signalSlotLock(receiver)); s->d_func()->addConnection(signal_index, c); r->d_func()->senders.append(c); @@ -2783,8 +2805,8 @@ bool QMetaObject::disconnect(const QObject *sender, int signal_index, QObject *s = const_cast(sender); QObject *r = const_cast(receiver); - QMutex *senderMutex = &s->d_func()->threadData->mutex; - QMutex *receiverMutex = r ? &r->d_func()->threadData->mutex : 0; + QMutex *senderMutex = signalSlotLock(sender); + QMutex *receiverMutex = receiver ? signalSlotLock(receiver) : 0; QOrderedMutexLocker locker(senderMutex, receiverMutex); QObjectConnectionListVector *connectionLists = s->d_func()->connectionLists; @@ -2804,7 +2826,7 @@ bool QMetaObject::disconnect(const QObject *sender, int signal_index, if (c->receiver && (r == 0 || (c->receiver == r && (method_index < 0 || c->method == method_index)))) { - QMutex *m = &c->receiver->d_func()->threadData->mutex; + QMutex *m = signalSlotLock(c->receiver); bool needToUnlock = false; if (!receiverMutex && senderMutex != m) { // need to relock this receiver and sender in the correct order @@ -2831,7 +2853,7 @@ bool QMetaObject::disconnect(const QObject *sender, int signal_index, if (c->receiver && (r == 0 || (c->receiver == r && (method_index < 0 || c->method == method_index)))) { - QMutex *m = &c->receiver->d_func()->threadData->mutex; + QMutex *m = signalSlotLock(c->receiver); bool needToUnlock = false; if (!receiverMutex && senderMutex != m) { // need to relock this receiver and sender in the correct order @@ -2972,7 +2994,7 @@ static void blocking_activate(QObject *sender, int signal, QObjectPrivate::Conne #else QSemaphore semaphore; queued_activate(sender, signal, c, argv, &semaphore); - QMutex *mutex = &QThreadData::get2(sender->thread())->mutex; + QMutex *mutex = signalSlotLock(sender); mutex->unlock(); semaphore.acquire(); mutex->lock(); @@ -2992,7 +3014,7 @@ void QMetaObject::activate(QObject *sender, int from_signal_index, int to_signal argv ? argv : empty_argv); } - QMutexLocker locker(&sender->d_func()->threadData->mutex); + QMutexLocker locker(signalSlotLock(sender)); QThreadData *currentThreadData = QThreadData::current(); QObjectConnectionListVector *connectionLists = sender->d_func()->connectionLists; @@ -3344,7 +3366,7 @@ void QObject::dumpObjectInfo() objectName().isEmpty() ? "unnamed" : objectName().toLocal8Bit().data()); Q_D(QObject); - QMutexLocker locker(&d->threadData->mutex); + QMutexLocker locker(signalSlotLock(this)); // first, look for connections where this object is the sender qDebug(" SIGNALS OUT"); diff --git a/tests/auto/qobjectrace/tst_qobjectrace.cpp b/tests/auto/qobjectrace/tst_qobjectrace.cpp index fcfd528..aa80534 100644 --- a/tests/auto/qobjectrace/tst_qobjectrace.cpp +++ b/tests/auto/qobjectrace/tst_qobjectrace.cpp @@ -43,6 +43,7 @@ #include #include + enum { OneMinute = 60 * 1000, TwoMinutes = OneMinute * 2 }; class tst_QObjectRace: public QObject @@ -70,12 +71,18 @@ public: public slots: void theSlot() { - enum { step = 1000 }; + enum { step = 35 }; if ((++count % step) == 0) { QThread *nextThread = threads.at((count / step) % threads.size()); moveToThread(nextThread); } } + + void destroSlot() { + emit theSignal(); + } +signals: + void theSignal(); }; class RaceThread : public QThread @@ -120,6 +127,10 @@ private slots: if (stopWatch.elapsed() >= OneMinute / 2) #endif quit(); + + QObject o; + connect(&o, SIGNAL(destroyed()) , object, SLOT(destroSlot())); + connect(object, SIGNAL(destroyed()) , &o, SLOT(deleteLater())); } }; @@ -138,10 +149,17 @@ void tst_QObjectRace::moveToThreadRace() for (int i = 0; i < ThreadCount; ++i) threads[i]->start(); - QVERIFY(threads[0]->wait(TwoMinutes)); + + while(!threads[0]->isFinished()) { + QPointer foo (object); + QObject o; + connect(&o, SIGNAL(destroyed()) , object, SLOT(destroSlot())); + connect(object, SIGNAL(destroyed()) , &o, SLOT(deleteLater())); + QTest::qWait(10); + } // the other threads should finish pretty quickly now for (int i = 1; i < ThreadCount; ++i) - QVERIFY(threads[i]->wait(30000)); + QVERIFY(threads[i]->wait(300)); for (int i = 0; i < ThreadCount; ++i) delete threads[i]; -- cgit v0.12