From 81e4db3a90abb87e5ed4702c73073ad3a11db00e Mon Sep 17 00:00:00 2001 From: Michael Zanetti Date: Thu, 10 Jan 2019 00:05:13 +0100 Subject: [PATCH] Handle binding loops in rules properly --- libnymea-core/nymeacore.cpp | 7 ++ libnymea-core/nymeacore.h | 1 + libnymea-core/ruleengine.cpp | 6 +- plugins/mock/devicepluginmock.cpp | 1 - tests/auto/rules/testrules.cpp | 104 +++++++++++++++++++++++++++--- 5 files changed, 108 insertions(+), 11 deletions(-) diff --git a/libnymea-core/nymeacore.cpp b/libnymea-core/nymeacore.cpp index 76a43ca5..104b5290 100644 --- a/libnymea-core/nymeacore.cpp +++ b/libnymea-core/nymeacore.cpp @@ -598,6 +598,12 @@ void NymeaCore::gotEvent(const Event &event) QList actions; QList eventBasedActions; foreach (const Rule &rule, m_ruleEngine->evaluateEvent(event)) { + if (m_executingRules.contains(rule.id())) { + qCWarning(dcRuleEngine()) << "WARNING: Loop detected in rule execution for rule" << rule.id() << rule.name(); + break; + } + m_executingRules.append(rule.id()); + // Event based if (!rule.eventDescriptors().isEmpty()) { m_logger->logRuleTriggered(rule); @@ -652,6 +658,7 @@ void NymeaCore::gotEvent(const Event &event) } executeRuleActions(actions); + m_executingRules.clear(); } void NymeaCore::onDateTimeChanged(const QDateTime &dateTime) diff --git a/libnymea-core/nymeacore.h b/libnymea-core/nymeacore.h index c89b861d..f3aee522 100644 --- a/libnymea-core/nymeacore.h +++ b/libnymea-core/nymeacore.h @@ -131,6 +131,7 @@ private: UserManager *m_userManager; QHash m_pendingActions; + QList m_executingRules; private slots: void gotEvent(const Event &event); diff --git a/libnymea-core/ruleengine.cpp b/libnymea-core/ruleengine.cpp index a02268de..c085ecd8 100644 --- a/libnymea-core/ruleengine.cpp +++ b/libnymea-core/ruleengine.cpp @@ -268,7 +268,11 @@ QList RuleEngine::evaluateTime(const QDateTime &dateTime) } m_lastEvaluationTime = dateTime; - qCDebug(dcRuleEngine()) << "EvaluateTimeEvent evaluated" << rules.count() << "to be executed"; + + if (rules.count() > 0) { // Don't spam the log + qCDebug(dcRuleEngine()) << "EvaluateTimeEvent evaluated" << rules.count() << "to be executed"; + } + return rules; } diff --git a/plugins/mock/devicepluginmock.cpp b/plugins/mock/devicepluginmock.cpp index acff86d6..c21b1ddd 100644 --- a/plugins/mock/devicepluginmock.cpp +++ b/plugins/mock/devicepluginmock.cpp @@ -237,7 +237,6 @@ DeviceManager::DeviceError DevicePluginMock::executeAction(Device *device, const qCDebug(dcMockDevice()) << "Setting power to" << action.param(mockPowerActionPowerParamTypeId).value().toBool(); device->setStateValue(mockPowerStateTypeId, action.param(mockPowerActionPowerParamTypeId).value().toBool()); } - m_daemons.value(device)->actionExecuted(action.actionTypeId()); return DeviceManager::DeviceErrorNoError; } else if (device->deviceClassId() == mockDeviceAutoDeviceClassId) { diff --git a/tests/auto/rules/testrules.cpp b/tests/auto/rules/testrules.cpp index 81967cdd..bc839911 100644 --- a/tests/auto/rules/testrules.cpp +++ b/tests/auto/rules/testrules.cpp @@ -111,6 +111,8 @@ private slots: void testInterfaceBasedStateRule(); + void testLoopingRules(); + void testHousekeeping_data(); void testHousekeeping(); @@ -255,16 +257,26 @@ void TestRules::setWritableStateValue(const DeviceId &deviceId, const StateTypeI void TestRules::verifyRuleExecuted(const ActionTypeId &actionTypeId) { // Verify rule got executed - QNetworkAccessManager nam; - QSignalSpy spy(&nam, SIGNAL(finished(QNetworkReply*))); - QNetworkRequest request(QUrl(QString("http://localhost:%1/actionhistory").arg(QString::number(m_mockDevice1Port)))); - QNetworkReply *reply = nam.get(request); - spy.wait(); - QCOMPARE(spy.count(), 1); + bool actionFound = false; + QByteArray actionHistory; + int i = 0; + while (!actionFound && i < 50) { + QNetworkAccessManager nam; + QSignalSpy spy(&nam, SIGNAL(finished(QNetworkReply*))); + QNetworkRequest request(QUrl(QString("http://localhost:%1/actionhistory").arg(QString::number(m_mockDevice1Port)))); + QNetworkReply *reply = nam.get(request); + spy.wait(); + QCOMPARE(spy.count(), 1); - QByteArray actionHistory = reply->readAll(); - QVERIFY2(actionTypeId == ActionTypeId(actionHistory), "Action not triggered. Current action history: \"" + actionHistory + "\""); - reply->deleteLater(); + actionHistory = reply->readAll(); + actionFound = actionTypeId == ActionTypeId(actionHistory); + reply->deleteLater(); + if (!actionFound) { + QTest::qWait(100); + } + i++; + } + QVERIFY2(actionFound, "Action not triggered. Current action history: \"" + actionHistory + "\""); } void TestRules::verifyRuleNotExecuted() @@ -2808,6 +2820,80 @@ void TestRules::testInterfaceBasedStateRule() verifyRuleExecuted(mockActionIdPower); } +void TestRules::testLoopingRules() +{ + QVariantMap powerOnActionParam; + powerOnActionParam.insert("paramTypeId", mockPowerStateTypeId); + powerOnActionParam.insert("value", true); + + QVariantMap powerOffActionParam; + powerOffActionParam.insert("paramTypeId", mockPowerStateTypeId); + powerOffActionParam.insert("value", false); + + QVariantMap powerOnEventParam = powerOnActionParam; + powerOnEventParam.insert("operator", "ValueOperatorEquals"); + + QVariantMap powerOffEventParam = powerOffActionParam; + powerOffEventParam.insert("operator", "ValueOperatorEquals"); + + QVariantMap onEvent; + onEvent.insert("eventTypeId", mockPowerStateTypeId); + onEvent.insert("deviceId", m_mockDeviceId); + onEvent.insert("paramDescriptors", QVariantList() << powerOnEventParam); + + QVariantMap offEvent; + offEvent.insert("eventTypeId", mockPowerStateTypeId); + offEvent.insert("deviceId", m_mockDeviceId); + offEvent.insert("paramDescriptors", QVariantList() << powerOffEventParam); + + QVariantMap onAction; + onAction.insert("actionTypeId", mockPowerStateTypeId); + onAction.insert("deviceId", m_mockDeviceId); + onAction.insert("ruleActionParams", QVariantList() << powerOnActionParam); + + QVariantMap offAction; + offAction.insert("actionTypeId", mockPowerStateTypeId); + offAction.insert("deviceId", m_mockDeviceId); + offAction.insert("ruleActionParams", QVariantList() << powerOffActionParam); + + // Add rule 1 + QVariantMap addRuleParams; + addRuleParams.insert("name", "Rule off -> on"); + addRuleParams.insert("eventDescriptors", QVariantList() << offEvent); + addRuleParams.insert("actions", QVariantList() << onAction); + QVariant response = injectAndWait("Rules.AddRule", addRuleParams); + qWarning() << response; + verifyRuleError(response); + + // Add rule 1 + addRuleParams.clear(); + addRuleParams.insert("name", "Rule on -> off"); + addRuleParams.insert("eventDescriptors", QVariantList() << onEvent); + addRuleParams.insert("actions", QVariantList() << offAction); + response = injectAndWait("Rules.AddRule", addRuleParams); + verifyRuleError(response); + + cleanupMockHistory(); + + QVariantMap params; + params.insert("deviceId", m_mockDeviceId); + params.insert("actionTypeId", mockPowerStateTypeId); + params.insert("params", QVariantList() << powerOffActionParam); + response = injectAndWait("Actions.ExecuteAction", params); + verifyRuleExecuted(mockActionIdPower); + + cleanupMockHistory(); + + params.clear(); + params.insert("deviceId", m_mockDeviceId); + params.insert("actionTypeId", mockPowerStateTypeId); + params.insert("params", QVariantList() << powerOnActionParam); + response = injectAndWait("Actions.ExecuteAction", params); + verifyRuleExecuted(mockActionIdPower); + + // No need to check anything else. This test sets up a binding loop and if the core doesn't catch it it'll crash here. +} + void TestRules::testHousekeeping_data() { QTest::addColumn("testAction");