From b2f6406b6f3c6374df2751a4669fbe51f04d09e7 Mon Sep 17 00:00:00 2001 From: Erik Verbruggen Date: Wed, 18 Mar 2015 12:44:22 +0100 Subject: [PATCH] QStateMachine: remove conflicting transitions after selection. After selecting all (enabled) transitions for a microstep, filter out any conflicting transition. The actual conflict resulution is done by ordering the transitions in order of the states that selected them. For example: if an event would trigger two transitions in a parallel state where one would exit that state and the other would not, this filtering prevents the state machine from selecting both states (as this case is an invalid state of the whole machine). This also fixes the exit set calculation for parallel states when one of its substates is exited and subsequently re-entered in the same transition. Previously, the parallel state was not exited, and subsequent re-entry was ignored (because it was still active). Now it is correctly exited and re-entered. A side-effect of the transition ordering mentioned above is it also fixes the non-deterministic behavior of which of the conflicting transitions is taken. [ChangeLog][QtCore] Fixed an issue where the state machine could end up in an invalid state when transitions from a parallel state were not checked for conflicts. [ChangeLog][QtCore] Fixed a case where a parallel state was not exited and re-entered when one of its substates was exited and subsequently re-entered. [ChangeLog][QtCore] Fixed the non-deterministic behavior of picking a transition from a set of conflicting transitions. Task-number: QTBUG-44783 Change-Id: I2ee72b6a2f552077bfa7aa4d369474ab62f4c2f0 Reviewed-by: Eskil Abrahamsen Blomfeldt Reviewed-by: Kevin Funk --- src/corelib/statemachine/qstatemachine.cpp | 230 ++++++++++++------ src/corelib/statemachine/qstatemachine_p.h | 10 +- .../qstatemachine/tst_qstatemachine.cpp | 99 +++++++- 3 files changed, 254 insertions(+), 85 deletions(-) diff --git a/src/corelib/statemachine/qstatemachine.cpp b/src/corelib/statemachine/qstatemachine.cpp index 477fa83705..eec3febbfe 100644 --- a/src/corelib/statemachine/qstatemachine.cpp +++ b/src/corelib/statemachine/qstatemachine.cpp @@ -251,10 +251,17 @@ static QSet getEffectiveTargetStates(QAbstractTransition *tran foreach (QAbstractState *s, transition->targetStates()) { if (QHistoryState *historyState = QStateMachinePrivate::toHistoryState(s)) { QList historyConfiguration = QHistoryStatePrivate::get(historyState)->configuration; - if (!historyConfiguration.isEmpty()) + if (!historyConfiguration.isEmpty()) { + // There is a saved history, so apply that. targets.unite(historyConfiguration.toSet()); - else if (QAbstractState *defaultState = historyState->defaultState()) - targets.insert(defaultState); // Qt does not support initial transitions, but uses the default state of the history state for this. + } else if (QAbstractState *defaultState = historyState->defaultState()) { + // Qt does not support initial transitions, but uses the default state of the history state for this. + targets.insert(defaultState); + } else { + // Woops, we found a history state without a default state. That's not valid! + QStateMachinePrivate *m = QStateMachinePrivate::get(historyState->machine()); + m->setError(QStateMachine::NoDefaultStateInHistoryStateError, historyState); + } } else { targets.insert(s); } @@ -338,6 +345,15 @@ static int indexOfDescendant(QState *s, QAbstractState *desc) return -1; } +bool QStateMachinePrivate::transitionStateEntryLessThan(QAbstractTransition *t1, QAbstractTransition *t2) +{ + QState *s1 = t1->sourceState(), *s2 = t2->sourceState(); + if (s1 == s2) + return QStatePrivate::get(s1)->transitions().indexOf(t1) < QStatePrivate::get(s2)->transitions().indexOf(t2); + else + return stateEntryLessThan(t1->sourceState(), t2->sourceState()); +} + bool QStateMachinePrivate::stateEntryLessThan(QAbstractState *s1, QAbstractState *s2) { if (s1->parent() == s2->parent()) { @@ -401,40 +417,21 @@ QState *QStateMachinePrivate::findLCCA(const QList &states) con return findLCA(states, true); } -bool QStateMachinePrivate::isPreempted(const QAbstractState *s, const QSet &transitions) const -{ - QSet::const_iterator it; - for (it = transitions.constBegin(); it != transitions.constEnd(); ++it) { - QAbstractTransition *t = *it; - QList lst = t->targetStates(); - if (!lst.isEmpty()) { - lst.prepend(t->sourceState()); - QAbstractState *lca = findLCA(lst); - if (isDescendant(s, lca)) { -#ifdef QSTATEMACHINE_DEBUG - qDebug() << q_func() << ':' << transitions << "preempts selection of a transition from" - << s << "because" << s << "is a descendant of" << lca; -#endif - return true; - } - } - } - return false; -} - -QSet QStateMachinePrivate::selectTransitions(QEvent *event) const +QList QStateMachinePrivate::selectTransitions(QEvent *event) { Q_Q(const QStateMachine); - QSet enabledTransitions; - QSet::const_iterator it; + + QVarLengthArray configuration_sorted; + foreach (QAbstractState *s, configuration) { + if (isAtomic(s)) + configuration_sorted.append(s); + } + std::sort(configuration_sorted.begin(), configuration_sorted.end(), stateEntryLessThan); + + QList enabledTransitions; const_cast(q)->beginSelectTransitions(event); - for (it = configuration.constBegin(); it != configuration.constEnd(); ++it) { - QAbstractState *state = *it; - if (!isAtomic(state)) - continue; - if (isPreempted(state, enabledTransitions)) - continue; - QVector lst = getProperAncestors(state, rootState()->parentState()); + foreach (QAbstractState *state, configuration_sorted) { + QVector lst = getProperAncestors(state, Q_NULLPTR); if (QState *grp = toStandardState(state)) lst.prepend(grp); bool found = false; @@ -447,29 +444,94 @@ QSet QStateMachinePrivate::selectTransitions(QEvent *event #ifdef QSTATEMACHINE_DEBUG qDebug() << q << ": selecting transition" << t; #endif - enabledTransitions.insert(t); + enabledTransitions.append(t); found = true; break; } } } } + + if (!enabledTransitions.isEmpty()) { + removeConflictingTransitions(enabledTransitions); +#ifdef QSTATEMACHINE_DEBUG + qDebug() << q << ": enabled transitions after removing conflicts:" << enabledTransitions; +#endif + } const_cast(q)->endSelectTransitions(event); return enabledTransitions; } +/* The function as described in http://www.w3.org/TR/2014/WD-scxml-20140529/ : + +function removeConflictingTransitions(enabledTransitions): + filteredTransitions = new OrderedSet() + // toList sorts the transitions in the order of the states that selected them + for t1 in enabledTransitions.toList(): + t1Preempted = false; + transitionsToRemove = new OrderedSet() + for t2 in filteredTransitions.toList(): + if computeExitSet([t1]).hasIntersection(computeExitSet([t2])): + if isDescendant(t1.source, t2.source): + transitionsToRemove.add(t2) + else: + t1Preempted = true + break + if not t1Preempted: + for t3 in transitionsToRemove.toList(): + filteredTransitions.delete(t3) + filteredTransitions.add(t1) + + return filteredTransitions +*/ +void QStateMachinePrivate::removeConflictingTransitions(QList &enabledTransitions) +{ + QList filteredTransitions; + filteredTransitions.reserve(enabledTransitions.size()); + std::sort(enabledTransitions.begin(), enabledTransitions.end(), transitionStateEntryLessThan); + + Q_FOREACH (QAbstractTransition *t1, enabledTransitions) { + bool t1Preempted = false; + QVarLengthArray transitionsToRemove; + QSet exitSetT1 = computeExitSet_Unordered(QList() << t1); + Q_FOREACH (QAbstractTransition *t2, filteredTransitions) { + QSet exitSetT2 = computeExitSet_Unordered(QList() << t2); + if (!exitSetT1.intersect(exitSetT2).isEmpty()) { + if (isDescendant(t1->sourceState(), t2->sourceState())) { + transitionsToRemove.append(t2); + } else { + t1Preempted = true; + break; + } + } + } + if (!t1Preempted) { + Q_FOREACH (QAbstractTransition *t3, transitionsToRemove) + filteredTransitions.removeAll(t3); + filteredTransitions.append(t1); + } + } + + enabledTransitions = filteredTransitions; +} + void QStateMachinePrivate::microstep(QEvent *event, const QList &enabledTransitions) { #ifdef QSTATEMACHINE_DEBUG qDebug() << q_func() << ": begin microstep( enabledTransitions:" << enabledTransitions << ')'; qDebug() << q_func() << ": configuration before exiting states:" << configuration; #endif - QList exitedStates = computeStatesToExit(enabledTransitions); + QList exitedStates = computeExitSet(enabledTransitions); QHash pendingRestorables = computePendingRestorables(exitedStates); QSet statesForDefaultEntry; QList enteredStates = computeEntrySet(enabledTransitions, statesForDefaultEntry); +#ifdef QSTATEMACHINE_DEBUG + qDebug() << q_func() << ": computed exit set:" << exitedStates; + qDebug() << q_func() << ": computed entry set:" << enteredStates; +#endif + QHash > assignmentsForEnteredStates = computePropertyAssignments(enteredStates, pendingRestorables); if (!pendingRestorables.isEmpty()) { @@ -502,40 +564,65 @@ void QStateMachinePrivate::microstep(QEvent *event, const QList QStateMachinePrivate::computeStatesToExit(const QList &enabledTransitions) +/* The function as described in http://www.w3.org/TR/2014/WD-scxml-20140529/ : + +procedure computeExitSet(enabledTransitions) + +For each transition t in enabledTransitions, if t is targetless then do nothing, else compute the +transition's domain. (This will be the source state in the case of internal transitions) or the +least common compound ancestor state of the source state and target states of t (in the case of +external transitions. Add to the statesToExit set all states in the configuration that are +descendants of the domain. + +function computeExitSet(transitions) + statesToExit = new OrderedSet + for t in transitions: + if (t.target): + domain = getTransitionDomain(t) + for s in configuration: + if isDescendant(s,domain): + statesToExit.add(s) + return statesToExit +*/ +QList QStateMachinePrivate::computeExitSet(const QList &enabledTransitions) { - QSet statesToExit; -// QSet statesToSnapshot; - for (int i = 0; i < enabledTransitions.size(); ++i) { - QAbstractTransition *t = enabledTransitions.at(i); - QList lst = t->targetStates(); - if (lst.isEmpty()) - continue; - lst.prepend(t->sourceState()); - QAbstractState *lca = findLCA(lst); - if (lca == 0) { - setError(QStateMachine::NoCommonAncestorForTransitionError, t->sourceState()); - lst = pendingErrorStates.toList(); - lst.prepend(t->sourceState()); - - lca = findLCA(lst); - Q_ASSERT(lca != 0); - } - - { - QSet::const_iterator it; - for (it = configuration.constBegin(); it != configuration.constEnd(); ++it) { - QAbstractState *s = *it; - if (isDescendant(s, lca)) - statesToExit.insert(s); - } - } - } - QList statesToExit_sorted = statesToExit.toList(); + QList statesToExit_sorted = computeExitSet_Unordered(enabledTransitions).toList(); std::sort(statesToExit_sorted.begin(), statesToExit_sorted.end(), stateExitLessThan); return statesToExit_sorted; } +QSet QStateMachinePrivate::computeExitSet_Unordered(const QList &enabledTransitions) +{ + QSet statesToExit; + for (int i = 0; i < enabledTransitions.size(); ++i) { + QAbstractTransition *t = enabledTransitions.at(i); + QList effectiveTargetStates = getEffectiveTargetStates(t).toList(); + QAbstractState *domain = getTransitionDomain(t, effectiveTargetStates); + if (domain == Q_NULLPTR && !t->targetStates().isEmpty()) { + // So we didn't find the least common ancestor for the source and target states of the + // transition. If there were not target states, that would be fine: then the transition + // will fire any events or signals, but not exit the state. + // + // However, there are target states, so it's either a node without a parent (or parent's + // parent, etc), or the state belongs to a different state machine. Either way, this + // makes the state machine invalid. + if (error == QStateMachine::NoError) + setError(QStateMachine::NoCommonAncestorForTransitionError, t->sourceState()); + QList lst = pendingErrorStates.toList(); + lst.prepend(t->sourceState()); + + domain = findLCCA(lst); + Q_ASSERT(domain != 0); + } + + Q_FOREACH (QAbstractState* s, configuration) { + if (isDescendant(s, domain)) + statesToExit.insert(s); + } + } + return statesToExit; +} + void QStateMachinePrivate::exitStates(QEvent *event, const QList &statesToExit_sorted, const QHash > &assignmentsForEnteredStates) { @@ -602,9 +689,7 @@ QList QStateMachinePrivate::computeEntrySet(const QListtargetStates()) { addDescendantStatesToEnter(s, statesToEnter, statesForDefaultEntry); } -#ifdef QSTATEMACHINE_DEBUG - qDebug() << "computed entry set after descendants:" << statesToEnter; -#endif + QList effectiveTargetStates = getEffectiveTargetStates(t).toList(); QAbstractState *ancestor = getTransitionDomain(t, effectiveTargetStates); Q_FOREACH (QAbstractState *s, effectiveTargetStates) { @@ -643,7 +728,7 @@ function getTransitionDomain(t) else: return findLCCA([t.source].append(tstates)) */ -QAbstractState *QStateMachinePrivate::getTransitionDomain(QAbstractTransition *t, const QList &effectiveTargetStates) +QAbstractState *QStateMachinePrivate::getTransitionDomain(QAbstractTransition *t, const QList &effectiveTargetStates) const { if (effectiveTargetStates.isEmpty()) return 0; @@ -1285,6 +1370,9 @@ void QStateMachinePrivate::setError(QStateMachine::Error errorCode, QAbstractSta Q_ASSERT(currentErrorState != rootState()); if (currentErrorState != 0) { +#ifdef QSTATEMACHINE_DEBUG + qDebug() << q << ": entering error state" << currentErrorState << "from" << currentContext; +#endif QState *lca = findLCA(QList() << currentErrorState << currentContext); addStatesToEnter(currentErrorState, lca, pendingErrorStates, pendingErrorStatesForDefaultEntry); } else { @@ -1640,7 +1728,7 @@ void QStateMachinePrivate::_q_process() processing = false; break; } - QSet enabledTransitions; + QList enabledTransitions; QEvent *e = new QEvent(QEvent::None); enabledTransitions = selectTransitions(e); if (enabledTransitions.isEmpty()) { @@ -1676,7 +1764,7 @@ void QStateMachinePrivate::_q_process() } if (!enabledTransitions.isEmpty()) { q->beginMicrostep(e); - microstep(e, enabledTransitions.toList()); + microstep(e, enabledTransitions); q->endMicrostep(e); } #ifdef QSTATEMACHINE_DEBUG diff --git a/src/corelib/statemachine/qstatemachine_p.h b/src/corelib/statemachine/qstatemachine_p.h index a8f0745730..a88f95f1f5 100644 --- a/src/corelib/statemachine/qstatemachine_p.h +++ b/src/corelib/statemachine/qstatemachine_p.h @@ -103,6 +103,7 @@ public: QState *findLCA(const QList &states, bool onlyCompound = false) const; QState *findLCCA(const QList &states) const; + static bool transitionStateEntryLessThan(QAbstractTransition *t1, QAbstractTransition *t2); static bool stateEntryLessThan(QAbstractState *s1, QAbstractState *s2); static bool stateExitLessThan(QAbstractState *s1, QAbstractState *s2); @@ -123,12 +124,13 @@ public: void clearHistory(); QAbstractTransition *createInitialTransition() const; + void removeConflictingTransitions(QList &enabledTransitions); void microstep(QEvent *event, const QList &transitionList); - bool isPreempted(const QAbstractState *s, const QSet &transitions) const; - QSet selectTransitions(QEvent *event) const; + QList selectTransitions(QEvent *event); void exitStates(QEvent *event, const QList &statesToExit_sorted, const QHash > &assignmentsForEnteredStates); - QList computeStatesToExit(const QList &enabledTransitions); + QList computeExitSet(const QList &enabledTransitions); + QSet computeExitSet_Unordered(const QList &enabledTransitions); void executeTransitionContent(QEvent *event, const QList &transitionList); void enterStates(QEvent *event, const QList &exitedStates_sorted, const QList &statesToEnter_sorted, @@ -141,7 +143,7 @@ public: QList computeEntrySet(const QList &enabledTransitions, QSet &statesForDefaultEntry); QAbstractState *getTransitionDomain(QAbstractTransition *t, - const QList &effectiveTargetStates); + const QList &effectiveTargetStates) const; void addDescendantStatesToEnter(QAbstractState *state, QSet &statesToEnter, QSet &statesForDefaultEntry); diff --git a/tests/auto/corelib/statemachine/qstatemachine/tst_qstatemachine.cpp b/tests/auto/corelib/statemachine/qstatemachine/tst_qstatemachine.cpp index 605d1e7393..96d0a62f6b 100644 --- a/tests/auto/corelib/statemachine/qstatemachine/tst_qstatemachine.cpp +++ b/tests/auto/corelib/statemachine/qstatemachine/tst_qstatemachine.cpp @@ -246,6 +246,7 @@ private slots: void childModeConstructor(); void qtbug_44963(); + void qtbug_44783(); }; class TestState : public QState @@ -910,8 +911,11 @@ void tst_QStateMachine::historyStateHasNowhereToGo() QStateMachine machine; QState *initialState = new QState(&machine); + initialState->setObjectName("initialState"); machine.setInitialState(initialState); - machine.setErrorState(new QState(&machine)); // avoid warnings + QState *errorState = new QState(&machine); + errorState->setObjectName("errorState"); + machine.setErrorState(errorState); // avoid warnings QState *brokenState = new QState(&machine); brokenState->setObjectName("brokenState"); @@ -919,7 +923,9 @@ void tst_QStateMachine::historyStateHasNowhereToGo() QHistoryState *historyState = new QHistoryState(brokenState); historyState->setObjectName("historyState"); - initialState->addTransition(new EventTransition(QEvent::User, historyState)); + EventTransition *t = new EventTransition(QEvent::User, historyState); + t->setObjectName("initialState->historyState"); + initialState->addTransition(t); machine.start(); QCoreApplication::processEvents(); @@ -4274,6 +4280,11 @@ void tst_QStateMachine::transitionsFromParallelStateWithNoChildren() void tst_QStateMachine::parallelStateTransition() { + // This test checks if the parallel state is exited and re-entered if one compound state + // is exited and subsequently re-entered. When the parallel state is exited, the other compound + // state in the parallel state has to be exited too. When the parallel state is re-entered, the + // other state also needs to be re-entered. + QStateMachine machine; QState *parallelState = new QState(QState::ParallelStates, &machine); @@ -4302,12 +4313,16 @@ void tst_QStateMachine::parallelStateTransition() s1OtherChild->setObjectName("s1OtherChild"); DEFINE_ACTIVE_SPY(s1OtherChild); + // The following transition will exit s1 (which means that parallelState is also exited), and + // subsequently re-entered (which means that parallelState is also re-entered). EventTransition *et = new EventTransition(QEvent::User, s1OtherChild); et->setObjectName("s1->s1OtherChild"); s1->addTransition(et); machine.start(); QCoreApplication::processEvents(); + + // Initial entrance of the parallel state and its sub-states: TEST_ACTIVE_CHANGED(parallelState, 1); TEST_ACTIVE_CHANGED(s1, 1); TEST_ACTIVE_CHANGED(s1InitialChild, 1); @@ -4325,22 +4340,22 @@ void tst_QStateMachine::parallelStateTransition() machine.postEvent(new QEvent(QEvent::User)); QCoreApplication::processEvents(); - TEST_ACTIVE_CHANGED(parallelState, 1); - TEST_ACTIVE_CHANGED(s1, 3); - TEST_ACTIVE_CHANGED(s1InitialChild, 2); - TEST_ACTIVE_CHANGED(s2, 3); - TEST_ACTIVE_CHANGED(s2InitialChild, 3); - TEST_ACTIVE_CHANGED(s1OtherChild, 1); + TEST_ACTIVE_CHANGED(parallelState, 3); // initial + exit + entry + TEST_ACTIVE_CHANGED(s1, 3); // initial + exit + entry + TEST_ACTIVE_CHANGED(s1InitialChild, 2); // initial + exit + TEST_ACTIVE_CHANGED(s2, 3); // initial + exit due to parent exit + entry due to parent re-entry + TEST_ACTIVE_CHANGED(s2InitialChild, 3); // initial + exit due to parent exit + re-entry due to parent re-entry + TEST_ACTIVE_CHANGED(s1OtherChild, 1); // entry due to transition QVERIFY(machine.isRunning()); + // Check that s1InitialChild is not in the configuration, because although s1 is re-entered, + // another child state (s1OtherChild) is active, so the initial child should not be activated. QVERIFY(machine.configuration().contains(parallelState)); - QVERIFY(machine.configuration().contains(s1)); QVERIFY(machine.configuration().contains(s2)); QVERIFY(machine.configuration().contains(s1OtherChild)); QVERIFY(machine.configuration().contains(s2InitialChild)); QCOMPARE(machine.configuration().size(), 5); - } void tst_QStateMachine::nestedRestoreProperties() @@ -6256,5 +6271,69 @@ void tst_QStateMachine::qtbug_44963() QVERIFY(machine.isRunning()); } +void tst_QStateMachine::qtbug_44783() +{ + SignalEmitter emitter; + + QStateMachine machine; + QState s(&machine); + QState p(QState::ParallelStates, &s); + QState p1(&p); + QState p1_1(&p1); + QState p1_2(&p1); + QState p2(&p); + QState s1(&machine); + + machine.setInitialState(&s); + s.setInitialState(&p); + p1.setInitialState(&p1_1); + p1_1.addTransition(&emitter, SIGNAL(signalWithNoArg()), &p1_2)->setObjectName("p1_1->p1_2"); + p2.addTransition(&emitter, SIGNAL(signalWithNoArg()), &s1)->setObjectName("p2->s1"); + + s.setObjectName("s"); + p.setObjectName("p"); + p1.setObjectName("p1"); + p1_1.setObjectName("p1_1"); + p1_2.setObjectName("p1_2"); + p2.setObjectName("p2"); + s1.setObjectName("s1"); + + machine.start(); + + QTRY_COMPARE(machine.configuration().contains(&s), true); + QTRY_COMPARE(machine.configuration().contains(&p), true); + QTRY_COMPARE(machine.configuration().contains(&p1), true); + QTRY_COMPARE(machine.configuration().contains(&p1_1), true); + QTRY_COMPARE(machine.configuration().contains(&p1_2), false); + QTRY_COMPARE(machine.configuration().contains(&p2), true); + QTRY_COMPARE(machine.configuration().contains(&s1), false); + + emitter.emitSignalWithNoArg(); + + // Only one of the following two can be true, because the two possible transitions conflict. + if (machine.configuration().contains(&s1)) { + // the transition p2 -> s1 was taken, not p1_1 -> p1_2, so: + // the parallel state exited, so none of the states inside it are active + QTRY_COMPARE(machine.configuration().contains(&s), false); + QTRY_COMPARE(machine.configuration().contains(&p), false); + QTRY_COMPARE(machine.configuration().contains(&p1), false); + QTRY_COMPARE(machine.configuration().contains(&p1_1), false); + QTRY_COMPARE(machine.configuration().contains(&p1_2), false); + QTRY_COMPARE(machine.configuration().contains(&p2), false); + } else { + // the transition p1_1 -> p1_2 was taken, not p2 -> s1, so: + // the parallel state was not exited and the state is the same as the start state with one + // difference: p1_1 inactive and p1_2 active: + QTRY_COMPARE(machine.configuration().contains(&s), true); + QTRY_COMPARE(machine.configuration().contains(&p), true); + QTRY_COMPARE(machine.configuration().contains(&p1), true); + QTRY_COMPARE(machine.configuration().contains(&p1_1), false); + QTRY_COMPARE(machine.configuration().contains(&p1_2), true); + QTRY_COMPARE(machine.configuration().contains(&p2), true); + } + + QVERIFY(machine.isRunning()); +} + QTEST_MAIN(tst_QStateMachine) #include "tst_qstatemachine.moc"