From b2f9e911ad9c8cae57d853c1c235df5792025b12 Mon Sep 17 00:00:00 2001 From: Michael Zanetti Date: Sat, 20 Feb 2021 22:56:57 +0100 Subject: [PATCH 1/2] Don't cal thingRemoved() when a thing didn't complete the setup --- .../integrations/thingmanagerimplementation.cpp | 14 +++++++++++++- .../integrations/thingmanagerimplementation.h | 1 + 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/libnymea-core/integrations/thingmanagerimplementation.cpp b/libnymea-core/integrations/thingmanagerimplementation.cpp index 694fd9a3..992005d6 100644 --- a/libnymea-core/integrations/thingmanagerimplementation.cpp +++ b/libnymea-core/integrations/thingmanagerimplementation.cpp @@ -455,6 +455,10 @@ ThingSetupInfo *ThingManagerImplementation::reconfigureThingInternal(Thing *thin // try to setup the thing with the new params ThingSetupInfo *info = new ThingSetupInfo(thing, this, 30000); plugin->setupThing(info); + connect(info, &ThingSetupInfo::destroyed, thing, [=](){ + m_pendingSetups.remove(thing->id()); + }); + connect(info, &ThingSetupInfo::finished, this, [this, info](){ if (info->status() != Thing::ThingErrorNoError) { @@ -775,8 +779,11 @@ Thing::ThingError ThingManagerImplementation::removeConfiguredThing(const ThingI IntegrationPlugin *plugin = m_integrationPlugins.value(thing->pluginId()); if (!plugin) { qCWarning(dcThingManager()).nospace() << "Plugin not loaded for thing " << thing->name() << ". Not calling thingRemoved on plugin."; - } else { + } else if (thing->setupStatus() == Thing::ThingSetupStatusComplete) { plugin->thingRemoved(thing); + } else if (thing->setupStatus() == Thing::ThingSetupStatusInProgress) { + ThingSetupInfo *setupInfo = m_pendingSetups.value(thingId); + emit setupInfo->aborted(); } thing->deleteLater(); @@ -1989,6 +1996,11 @@ ThingSetupInfo* ThingManagerImplementation::setupThing(Thing *thing) plugin->setupThing(info); + m_pendingSetups.insert(thing->id(), info); + connect(info, &ThingSetupInfo::destroyed, thing, [=](){ + m_pendingSetups.remove(thing->id()); + }); + return info; } diff --git a/libnymea-core/integrations/thingmanagerimplementation.h b/libnymea-core/integrations/thingmanagerimplementation.h index e8628e81..7e6e357f 100644 --- a/libnymea-core/integrations/thingmanagerimplementation.h +++ b/libnymea-core/integrations/thingmanagerimplementation.h @@ -191,6 +191,7 @@ private: QString thingName; }; QHash m_pendingPairings; + QHash m_pendingSetups; QHash m_ioConnections; From dab87645842b79c0c702f910966bb368719576a7 Mon Sep 17 00:00:00 2001 From: Michael Zanetti Date: Wed, 24 Feb 2021 17:40:10 +0100 Subject: [PATCH 2/2] Refresh setup status before proceeding --- .../thingmanagerimplementation.cpp | 10 +++++-- plugins/mock/integrationpluginmock.cpp | 26 ++++++++++++------- plugins/mock/integrationpluginmock.h | 4 +-- tests/auto/devices/testdevices.cpp | 3 ++- 4 files changed, 28 insertions(+), 15 deletions(-) diff --git a/libnymea-core/integrations/thingmanagerimplementation.cpp b/libnymea-core/integrations/thingmanagerimplementation.cpp index 992005d6..859ff5d6 100644 --- a/libnymea-core/integrations/thingmanagerimplementation.cpp +++ b/libnymea-core/integrations/thingmanagerimplementation.cpp @@ -772,6 +772,11 @@ ThingSetupInfo* ThingManagerImplementation::addConfiguredThingInternal(const Thi Thing::ThingError ThingManagerImplementation::removeConfiguredThing(const ThingId &thingId) { + // We're checking thingSetupStatus and abort any pending setup here. As setup finished() + // comes in as a QueuedConnection, make sure to process all events before going on so we + // don't end up aborting an already finished setup instead of calling thingRemoved() on it. + qApp->processEvents(); + Thing *thing = m_configuredThings.take(thingId); if (!thing) { return Thing::ThingErrorThingNotFound; @@ -779,11 +784,12 @@ Thing::ThingError ThingManagerImplementation::removeConfiguredThing(const ThingI IntegrationPlugin *plugin = m_integrationPlugins.value(thing->pluginId()); if (!plugin) { qCWarning(dcThingManager()).nospace() << "Plugin not loaded for thing " << thing->name() << ". Not calling thingRemoved on plugin."; - } else if (thing->setupStatus() == Thing::ThingSetupStatusComplete) { - plugin->thingRemoved(thing); } else if (thing->setupStatus() == Thing::ThingSetupStatusInProgress) { + qCWarning(dcThingManager()).nospace() << "Thing " << thing->name() << " is still being set up. Aborting setup."; ThingSetupInfo *setupInfo = m_pendingSetups.value(thingId); emit setupInfo->aborted(); + } else if (thing->setupStatus() == Thing::ThingSetupStatusComplete) { + plugin->thingRemoved(thing); } thing->deleteLater(); diff --git a/plugins/mock/integrationpluginmock.cpp b/plugins/mock/integrationpluginmock.cpp index 7acca96f..f1ae5b7d 100644 --- a/plugins/mock/integrationpluginmock.cpp +++ b/plugins/mock/integrationpluginmock.cpp @@ -134,7 +134,7 @@ void IntegrationPluginMock::setupThing(ThingSetupInfo *info) { if (info->thing()->thingClassId() == mockThingClassId || info->thing()->thingClassId() == autoMockThingClassId) { if (m_daemons.contains(info->thing())) { - // We already have a daemon, seem's we're reconfiguring + // We already have a daemon, seems we're reconfiguring delete m_daemons.take(info->thing()); } @@ -174,8 +174,7 @@ void IntegrationPluginMock::setupThing(ThingSetupInfo *info) if (async) { - Thing *device = info->thing(); - QTimer::singleShot(1000, device, [info](){ + QTimer::singleShot(1000, info, [info](){ qCDebug(dcMock()) << "Finishing thing setup for mocked thing" << info->thing()->name(); if (info->thing()->paramValue(mockThingBrokenParamTypeId).toBool()) { info->finish(Thing::ThingErrorSetupFailed, QT_TR_NOOP("This mocked thing is intentionally broken.")); @@ -183,6 +182,12 @@ void IntegrationPluginMock::setupThing(ThingSetupInfo *info) info->finish(Thing::ThingErrorNoError); } }); + + connect(info, &ThingSetupInfo::aborted, this, [=](){ + qCDebug(dcMock()) << "Setup aborted. Destroying webserver" << info->thing()->name(); + delete m_daemons.take(info->thing()); + }); + return; } qCDebug(dcMock()) << "Setup complete" << info->thing()->name(); @@ -260,24 +265,25 @@ void IntegrationPluginMock::setupThing(ThingSetupInfo *info) info->finish(Thing::ThingErrorThingClassNotFound); } -void IntegrationPluginMock::postSetupThing(Thing *device) +void IntegrationPluginMock::postSetupThing(Thing *thing) { - qCDebug(dcMock()) << "Postsetup mock" << device->name(); - if (device->thingClassId() == parentMockThingClassId) { + qCDebug(dcMock()) << "Postsetup mock" << thing->name(); + if (thing->thingClassId() == parentMockThingClassId) { foreach (Thing *d, myThings()) { - if (d->thingClassId() == childMockThingClassId && d->parentId() == device->id()) { + if (d->thingClassId() == childMockThingClassId && d->parentId() == thing->id()) { return; } } - ThingDescriptor mockDescriptor(childMockThingClassId, "Mocked Thing Child (Auto created)", "Mocked Thing Child (Auto created)", device->id()); + ThingDescriptor mockDescriptor(childMockThingClassId, "Mocked Thing Child (Auto created)", "Mocked Thing Child (Auto created)", thing->id()); emit autoThingsAppeared(ThingDescriptors() << mockDescriptor); } } -void IntegrationPluginMock::thingRemoved(Thing *device) +void IntegrationPluginMock::thingRemoved(Thing *thing) { - delete m_daemons.take(device); + qCDebug(dcMock()) << "Thing removed" << thing->name(); + delete m_daemons.take(thing); } void IntegrationPluginMock::startMonitoringAutoThings() diff --git a/plugins/mock/integrationpluginmock.h b/plugins/mock/integrationpluginmock.h index b91d45a6..8e09c13e 100644 --- a/plugins/mock/integrationpluginmock.h +++ b/plugins/mock/integrationpluginmock.h @@ -51,8 +51,8 @@ public: void discoverThings(ThingDiscoveryInfo *info) override; void setupThing(ThingSetupInfo *info) override; - void postSetupThing(Thing *device) override; - void thingRemoved(Thing *device) override; + void postSetupThing(Thing *thing) override; + void thingRemoved(Thing *thing) override; void startMonitoringAutoThings() override; diff --git a/tests/auto/devices/testdevices.cpp b/tests/auto/devices/testdevices.cpp index 1c65300b..f310edf2 100644 --- a/tests/auto/devices/testdevices.cpp +++ b/tests/auto/devices/testdevices.cpp @@ -160,7 +160,7 @@ void TestDevices::initTestCase() NymeaTestBase::initTestCase(); QLoggingCategory::setFilterRules("*.debug=false\n" "Tests.debug=true\n" - "MockDevice.debug=true\n" + "Mock.debug=true\n" ); // Adding an async mock device to be used in tests below @@ -619,6 +619,7 @@ void TestDevices::storedDevices() params.clear(); params.insert("deviceId", addedDeviceId); + qCDebug(dcTests()) << "Cleaning up mock device again"; response = injectAndWait("Devices.RemoveConfiguredDevice", params); verifyDeviceError(response); }