From 302e6968f1152d5dee8d5debafb313bd53fa55ff Mon Sep 17 00:00:00 2001 From: Kent Hansen Date: Mon, 4 Jun 2012 21:51:04 +0200 Subject: statemachine: Make delayed event posting work from secondary thread postDelayedEvent() and cancelDelayedEvent() are marked as thread-safe in the documentation. Unfortunately, they didn't actually work when called from another thread; they just produced some warnings: QObject::startTimer: timers cannot be started from another thread QObject::killTimer: timers cannot be stopped from another thread As the warnings indicate, the issue was that postDelayedEvent() (cancelDelayedEvent()) unconditionally called QObject::startTimer() (stopTimer()), i.e. without considering which thread the function was called from. If the function is called from a different thread, the actual starting/stopping of the associated timer is now done from the correct thread, by asynchronously calling a private slot on the state machine. This also means that the raw timer id can no longer be used as the id of the delayed event, since a valid event id must be returned before the timer has started. The state machine now manages those ids itself (using a QFreeList, just like startTimer() and killTimer() do), and also keeps a mapping from timer id to event id once the timer has been started. This is inherently more complex than before, but at least the API should work as advertised/intended now. Task-number: QTBUG-17975 Change-Id: I3a866d01dca23174c8841112af50b87141df0943 Reviewed-by: Eskil Abrahamsen Blomfeldt --- src/corelib/statemachine/qstatemachine.cpp | 101 ++++++++++++++++++++++++----- src/corelib/statemachine/qstatemachine.h | 2 + src/corelib/statemachine/qstatemachine_p.h | 15 ++++- 3 files changed, 101 insertions(+), 17 deletions(-) (limited to 'src/corelib/statemachine') diff --git a/src/corelib/statemachine/qstatemachine.cpp b/src/corelib/statemachine/qstatemachine.cpp index ffe2e74d61..7b1612655a 100644 --- a/src/corelib/statemachine/qstatemachine.cpp +++ b/src/corelib/statemachine/qstatemachine.cpp @@ -1322,6 +1322,36 @@ void QStateMachinePrivate::_q_process() } } +void QStateMachinePrivate::_q_startDelayedEventTimer(int id, int delay) +{ + Q_Q(QStateMachine); + QMutexLocker locker(&delayedEventsMutex); + QHash::iterator it = delayedEvents.find(id); + if (it != delayedEvents.end()) { + DelayedEvent &e = it.value(); + Q_ASSERT(!e.timerId); + e.timerId = q->startTimer(delay); + if (!e.timerId) { + qWarning("QStateMachine::postDelayedEvent: failed to start timer (id=%d, delay=%d)", id, delay); + delayedEvents.erase(it); + delayedEventIdFreeList.release(id); + } else { + timerIdToDelayedEventId.insert(e.timerId, id); + } + } else { + // It's been cancelled already + delayedEventIdFreeList.release(id); + } +} + +void QStateMachinePrivate::_q_killDelayedEventTimer(int id, int timerId) +{ + Q_Q(QStateMachine); + q->killTimer(timerId); + QMutexLocker locker(&delayedEventsMutex); + delayedEventIdFreeList.release(id); +} + void QStateMachinePrivate::postInternalEvent(QEvent *e) { QMutexLocker locker(&internalEventMutex); @@ -1384,12 +1414,17 @@ void QStateMachinePrivate::cancelAllDelayedEvents() { Q_Q(QStateMachine); QMutexLocker locker(&delayedEventsMutex); - QHash::const_iterator it; + QHash::const_iterator it; for (it = delayedEvents.constBegin(); it != delayedEvents.constEnd(); ++it) { - int id = it.key(); - QEvent *e = it.value(); - q->killTimer(id); - delete e; + const DelayedEvent &e = it.value(); + if (e.timerId) { + timerIdToDelayedEventId.remove(e.timerId); + q->killTimer(e.timerId); + delayedEventIdFreeList.release(it.key()); + } else { + // Cancellation will be detected in pending _q_startDelayedEventTimer() call + } + delete e.event; } delayedEvents.clear(); } @@ -1988,9 +2023,26 @@ int QStateMachine::postDelayedEvent(QEvent *event, int delay) qDebug() << this << ": posting event" << event << "with delay" << delay; #endif QMutexLocker locker(&d->delayedEventsMutex); - int tid = startTimer(delay); - d->delayedEvents[tid] = event; - return tid; + int id = d->delayedEventIdFreeList.next(); + bool inMachineThread = (QThread::currentThread() == thread()); + int timerId = inMachineThread ? startTimer(delay) : 0; + if (inMachineThread && !timerId) { + qWarning("QStateMachine::postDelayedEvent: failed to start timer with interval %d", delay); + d->delayedEventIdFreeList.release(id); + return -1; + } + QStateMachinePrivate::DelayedEvent delayedEvent(event, timerId); + d->delayedEvents.insert(id, delayedEvent); + if (timerId) { + d->timerIdToDelayedEventId.insert(timerId, id); + } else { + Q_ASSERT(!inMachineThread); + QMetaObject::invokeMethod(this, "_q_startDelayedEventTimer", + Qt::QueuedConnection, + Q_ARG(int, id), + Q_ARG(int, delay)); + } + return id; } /*! @@ -2010,11 +2062,25 @@ bool QStateMachine::cancelDelayedEvent(int id) return false; } QMutexLocker locker(&d->delayedEventsMutex); - QEvent *e = d->delayedEvents.take(id); - if (!e) + QStateMachinePrivate::DelayedEvent e = d->delayedEvents.take(id); + if (!e.event) return false; - killTimer(id); - delete e; + if (e.timerId) { + d->timerIdToDelayedEventId.remove(e.timerId); + bool inMachineThread = (QThread::currentThread() == thread()); + if (inMachineThread) { + killTimer(e.timerId); + d->delayedEventIdFreeList.release(id); + } else { + QMetaObject::invokeMethod(this, "_q_killDelayedEventTimer", + Qt::QueuedConnection, + Q_ARG(int, id), + Q_ARG(int, e.timerId)); + } + } else { + // Cancellation will be detected in pending _q_startDelayedEventTimer() call + } + delete e.event; return true; } @@ -2060,15 +2126,18 @@ bool QStateMachine::event(QEvent *e) if (d->state != QStateMachinePrivate::Running) { // This event has been cancelled already QMutexLocker locker(&d->delayedEventsMutex); - Q_ASSERT(!d->delayedEvents.contains(tid)); + Q_ASSERT(!d->timerIdToDelayedEventId.contains(tid)); return true; } d->delayedEventsMutex.lock(); - QEvent *ee = d->delayedEvents.take(tid); - if (ee != 0) { + int id = d->timerIdToDelayedEventId.take(tid); + QStateMachinePrivate::DelayedEvent ee = d->delayedEvents.take(id); + if (ee.event != 0) { + Q_ASSERT(ee.timerId == tid); killTimer(tid); + d->delayedEventIdFreeList.release(id); d->delayedEventsMutex.unlock(); - d->postExternalEvent(ee); + d->postExternalEvent(ee.event); d->processEvents(QStateMachinePrivate::DirectProcessing); return true; } else { diff --git a/src/corelib/statemachine/qstatemachine.h b/src/corelib/statemachine/qstatemachine.h index b3aeb41016..bc3f2fa27f 100644 --- a/src/corelib/statemachine/qstatemachine.h +++ b/src/corelib/statemachine/qstatemachine.h @@ -184,6 +184,8 @@ private: #ifndef QT_NO_ANIMATION Q_PRIVATE_SLOT(d_func(), void _q_animationFinished()) #endif + Q_PRIVATE_SLOT(d_func(), void _q_startDelayedEventTimer(int, int)) + Q_PRIVATE_SLOT(d_func(), void _q_killDelayedEventTimer(int, int)) }; #endif //QT_NO_STATEMACHINE diff --git a/src/corelib/statemachine/qstatemachine_p.h b/src/corelib/statemachine/qstatemachine_p.h index d01b050e15..ae5660719f 100644 --- a/src/corelib/statemachine/qstatemachine_p.h +++ b/src/corelib/statemachine/qstatemachine_p.h @@ -62,6 +62,7 @@ #include #include #include +#include QT_BEGIN_NAMESPACE @@ -120,6 +121,8 @@ public: #ifndef QT_NO_ANIMATION void _q_animationFinished(); #endif + void _q_startDelayedEventTimer(int id, int delay); + void _q_killDelayedEventTimer(int id, int timerId); QState *rootState() const; @@ -232,7 +235,17 @@ public: #ifndef QT_NO_STATEMACHINE_EVENTFILTER QHash > qobjectEvents; #endif - QHash delayedEvents; + QFreeList delayedEventIdFreeList; + struct DelayedEvent { + QEvent *event; + int timerId; + DelayedEvent(QEvent *e, int tid) + : event(e), timerId(tid) {} + DelayedEvent() + : event(0), timerId(0) {} + }; + QHash delayedEvents; + QHash timerIdToDelayedEventId; QMutex delayedEventsMutex; typedef QEvent* (*f_cloneEvent)(QEvent*); -- cgit v1.2.3