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 <eskil.abrahamsen-blomfeldt@nokia.com>

diff --git a/src/corelib/statemachine/qstatemachine.cpp b/src/corelib/statemachine/qstatemachine.cpp
index ffe2e74..7b16126 100644 (file)
--- 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<int, DelayedEvent>::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<int, QEvent*>::const_iterator it;
+    QHash<int, DelayedEvent>::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 b3aeb41..bc3f2fa 100644 (file)
--- 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 d01b050..ae56607 100644 (file)
--- a/src/corelib/statemachine/qstatemachine_p.h
+++ b/src/corelib/statemachine/qstatemachine_p.h
@@ -62,6 +62,7 @@
 #include <QtCore/qpair.h>
 #include <QtCore/qset.h>
 #include <QtCore/qvector.h>
+#include <private/qfreelist_p.h>
 
 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<QObject*, QHash<QEvent::Type, int> > qobjectEvents;
 #endif
-    QHash<int, QEvent*> delayedEvents;
+    QFreeList<void> delayedEventIdFreeList;
+    struct DelayedEvent {
+        QEvent *event;
+        int timerId;
+        DelayedEvent(QEvent *e, int tid)
+            : event(e), timerId(tid) {}
+        DelayedEvent()
+            : event(0), timerId(0) {}
+    };
+    QHash<int, DelayedEvent> delayedEvents;
+    QHash<int, int> timerIdToDelayedEventId;
     QMutex delayedEventsMutex;
   
     typedef QEvent* (*f_cloneEvent)(QEvent*);

diff --git a/tests/auto/corelib/statemachine/qstatemachine/tst_qstatemachine.cpp b/tests/auto/corelib/statemachine/qstatemachine/tst_qstatemachine.cpp
index c5d33e7..14a0bed 100644 (file)
--- a/tests/auto/corelib/statemachine/qstatemachine/tst_qstatemachine.cpp
+++ b/tests/auto/corelib/statemachine/qstatemachine/tst_qstatemachine.cpp
@@ -101,6 +101,7 @@ private slots:
     void postEvent();
     void cancelDelayedEvent();
     void postDelayedEventAndStop();
+    void postDelayedEventFromThread();
     void stopAndPostEvent();
     void stateFinished();
     void parallelStates();
@@ -1731,6 +1732,55 @@ void tst_QStateMachine::postDelayedEventAndStop()
     QVERIFY(machine.configuration().contains(s1));
 }
 
+class DelayedEventPosterThread : public QThread
+{
+    Q_OBJECT
+public:
+    DelayedEventPosterThread(QStateMachine *machine, QObject *parent = 0)
+        : QThread(parent), firstEventWasCancelled(false),
+          m_machine(machine), m_count(0)
+    {
+        moveToThread(this);
+        QObject::connect(m_machine, SIGNAL(started()),
+                         this, SLOT(postEvent()));
+    }
+
+    mutable bool firstEventWasCancelled;
+
+private Q_SLOTS:
+    void postEvent()
+    {
+        int id = m_machine->postDelayedEvent(new QEvent(QEvent::User), 1000);
+        firstEventWasCancelled = m_machine->cancelDelayedEvent(id);
+
+        m_machine->postDelayedEvent(new QEvent(QEvent::User), 1);
+
+        quit();
+    }
+private:
+    QStateMachine *m_machine;
+    int m_count;
+};
+
+void tst_QStateMachine::postDelayedEventFromThread()
+{
+    QStateMachine machine;
+    QState *s1 = new QState(&machine);
+    QFinalState *f = new QFinalState(&machine);
+    s1->addTransition(new EventTransition(QEvent::User, f));
+    machine.setInitialState(s1);
+
+    DelayedEventPosterThread poster(&machine);
+    poster.start();
+
+    QSignalSpy finishedSpy(&machine, SIGNAL(finished()));
+    QVERIFY(finishedSpy.isValid());
+    machine.start();
+    QTRY_COMPARE(finishedSpy.count(), 1);
+
+    QVERIFY(poster.firstEventWasCancelled);
+}
+
 void tst_QStateMachine::stopAndPostEvent()
 {
     QStateMachine machine;