Merge PR #396: Don't call thingRemoved() when a thing didn't complete the setup

libnymea-core/integrations/thingmanagerimplementation.cpp

@@ -454,6 +454,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) {
@@ -824,6 +828,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;
@@ -831,7 +840,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::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);
     }
 
@@ -2070,6 +2083,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;
 }
 

libnymea-core/integrations/thingmanagerimplementation.h

@@ -194,6 +194,7 @@ private:
         QString thingName;
     };
     QHash<PairingTransactionId, PairingContext> m_pendingPairings;
+    QHash<ThingId, ThingSetupInfo*> m_pendingSetups;
 
     QHash<IOConnectionId, IOConnection> m_ioConnections;
 

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()

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;
 

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);
 }