Tear down lost and closed client connections safely

When a connection dropped, the listener thread deleted all of that
server's controllers immediately, before anything else was notified, so
the GUI, ResourceManager, and plugins kept dereferencing freed
controllers. Clicking Disconnect had the same problem and also ran the
teardown inline on the GUI thread, where UpdateDeviceList's
DEVICE_LIST_UPDATED is a BlockingQueuedConnection back to that thread,
so the device pages were not torn down before the controllers were
freed and a later queued onDetectionEnded dereferenced them.

Neither path frees controllers inline now. NetworkClient moves them to
an orphaned list and a dedicated teardown thread frees them: plugins
are warned first, then the same DETECTION_STARTED, UpdateDeviceList,
DETECTION_COMPLETE sequence a rescan runs tears down the device pages
that still reference the controllers, and only then are they freed, so
a controller always outlives its page. Both the connection-loss path
and the disconnect button queue to that thread, so the blocking handler
runs on the GUI thread while the controllers are still alive, and
StopClient's join stays off the GUI and listener threads.

NetworkClient.cpp

@@ -106,6 +106,20 @@ NetworkClient::NetworkClient()
 NetworkClient::~NetworkClient()
 {
     StopClient();
+
+    /*-----------------------------------------------------*\
+    | Free the controllers set aside by StopClient, plus    |
+    | any left behind by a listener that never ran          |
+    \*-----------------------------------------------------*/
+    ControllerListMutex.lock();
+
+    orphaned_controllers.insert(orphaned_controllers.end(), server_controllers.begin(), server_controllers.end());
+
+    server_controllers.clear();
+
+    ControllerListMutex.unlock();
+
+    DeleteOrphanedControllers();
 }
 
 /*---------------------------------------------------------*\
@@ -326,6 +340,36 @@ void NetworkClient::StopClient()
     SignalNetworkClientUpdate(NETWORKCLIENT_UPDATE_REASON_CLIENT_STOPPED);
 }
 
+/*---------------------------------------------------------*\
+| Controller teardown functions                             |
+\*---------------------------------------------------------*/
+void NetworkClient::TakeOrphanedControllers(std::vector<RGBController*>& orphaned)
+{
+    ControllerListMutex.lock();
+
+    orphaned.insert(orphaned.end(), orphaned_controllers.begin(), orphaned_controllers.end());
+
+    orphaned_controllers.clear();
+
+    ControllerListMutex.unlock();
+}
+
+void NetworkClient::DeleteOrphanedControllers()
+{
+    /*-----------------------------------------------------*\
+    | Delete outside the mutex; the controller destructor   |
+    | waits for callbacks that are already running          |
+    \*-----------------------------------------------------*/
+    std::vector<RGBController*> orphaned;
+
+    TakeOrphanedControllers(orphaned);
+
+    for(std::size_t orphaned_idx = 0; orphaned_idx < orphaned.size(); orphaned_idx++)
+    {
+        delete orphaned[orphaned_idx];
+    }
+}
+
 void NetworkClient::SendRequest_ControllerData(unsigned int dev_id)
 {
     NetPacketHeader request_hdr;
@@ -1905,23 +1949,27 @@ listen_done:
     server_initialized                  = false;
     server_connected                    = false;
 
+    /*-----------------------------------------------------*\
+    | Do not delete the controllers here; the front end     |
+    | still holds pointers to them. Move them to            |
+    | orphaned_controllers for the teardown thread to free  |
+    \*-----------------------------------------------------*/
     ControllerListMutex.lock();
 
-    std::vector<RGBController *> server_controllers_copy = server_controllers;
+    orphaned_controllers.insert(orphaned_controllers.end(), server_controllers.begin(), server_controllers.end());
 
     server_controllers.clear();
 
-    for(size_t server_controller_idx = 0; server_controller_idx < server_controllers_copy.size(); server_controller_idx++)
-    {
-        delete server_controllers_copy[server_controller_idx];
-    }
-
     ControllerListMutex.unlock();
 
     /*-----------------------------------------------------*\
-    | Client info has changed, call the callbacks           |
+    | Signal only an unplanned connection loss; on a        |
+    | requested stop the caller runs the teardown           |
     \*-----------------------------------------------------*/
-    SignalNetworkClientUpdate(NETWORKCLIENT_UPDATE_REASON_CLIENT_DISCONNECTED);
+    if(client_active)
+    {
+        SignalNetworkClientUpdate(NETWORKCLIENT_UPDATE_REASON_CLIENT_DISCONNECTED);
+    }
 }
 
 /*---------------------------------------------------------*\

NetworkClient.h

@@ -100,6 +100,12 @@ public:
     void                                StartClient();
     void                                StopClient();
 
+    /*-----------------------------------------------------*\
+    | Controller teardown functions                         |
+    \*-----------------------------------------------------*/
+    void                                DeleteOrphanedControllers();
+    void                                TakeOrphanedControllers(std::vector<RGBController*>& orphaned);
+
     void                                SendRequest_ControllerData(unsigned int dev_id);
     void                                SendRequest_RescanDevices();
 
@@ -249,6 +255,7 @@ private:
     \*-----------------------------------------------------*/
     std::mutex                          ControllerListMutex;
     std::vector<RGBController*>         server_controllers;
+    std::vector<RGBController*>         orphaned_controllers;
     std::vector<unsigned int>           server_controller_ids;
 
     /*-----------------------------------------------------*\

ResourceManager.cpp

@@ -86,6 +86,19 @@ static void ResourceManagerNetworkClientCallback(void* this_ptr, unsigned int up
             this_obj->UpdateDeviceList();
             break;
 
+        case NETWORKCLIENT_UPDATE_REASON_CLIENT_CONNECTED:
+            this_obj->SignalResourceManagerUpdate(RESOURCEMANAGER_UPDATE_REASON_CLIENT_INFO_UPDATED);
+            break;
+
+        /*-------------------------------------------------*\
+        | Unplanned connection loss: free the client's      |
+        | orphaned controllers on the teardown thread       |
+        \*-------------------------------------------------*/
+        case NETWORKCLIENT_UPDATE_REASON_CLIENT_DISCONNECTED:
+            this_obj->SignalResourceManagerUpdate(RESOURCEMANAGER_UPDATE_REASON_CLIENT_INFO_UPDATED);
+            this_obj->QueueClientTeardown();
+            break;
+
         case NETWORKCLIENT_UPDATE_REASON_DETECTION_STARTED:
             this_obj->SignalResourceManagerUpdate(RESOURCEMANAGER_UPDATE_REASON_DETECTION_STARTED);
             break;
@@ -151,6 +164,13 @@ ResourceManager::ResourceManager()
     plugin_manager              = NULL;
     server                      = NULL;
 
+    /*-----------------------------------------------------*\
+    | Start the client teardown thread                      |
+    \*-----------------------------------------------------*/
+    ClientTeardownPending       = false;
+    ClientTeardownRunning       = true;
+    ClientTeardownThread        = new std::thread(&ResourceManager::ClientTeardownThreadFunction, this);
+
     SetupConfigurationDirectory();
 
     /*-----------------------------------------------------*\
@@ -274,7 +294,21 @@ ResourceManager::ResourceManager()
 
 ResourceManager::~ResourceManager()
 {
+    /*-----------------------------------------------------*\
+    | Stop the client teardown thread                       |
+    \*-----------------------------------------------------*/
+    ClientTeardownMutex.lock();
+    ClientTeardownRunning = false;
+    ClientTeardownMutex.unlock();
 
+    ClientTeardownCv.notify_all();
+
+    if(ClientTeardownThread)
+    {
+        ClientTeardownThread->join();
+        delete ClientTeardownThread;
+        ClientTeardownThread = nullptr;
+    }
 }
 
 /*---------------------------------------------------------*\
@@ -535,38 +569,142 @@ void ResourceManager::RegisterNetworkClient(NetworkClient* new_client)
 {
     new_client->RegisterNetworkClientCallback(ResourceManagerNetworkClientCallback, this);
 
+    ClientListMutex.lock();
+
     clients.push_back(new_client);
+
+    ClientListMutex.unlock();
 }
 
 void ResourceManager::UnregisterNetworkClient(NetworkClient* network_client)
 {
     /*-----------------------------------------------------*\
-    | Stop the disconnecting client                         |
+    | Queue the removal for the teardown thread. This is    |
+    | called on the GUI thread, and the teardown rebuilds   |
+    | the device list, whose DEVICE_LIST_UPDATED handler    |
+    | is a BlockingQueuedConnection to the GUI thread. It   |
+    | also keeps StopClient's join off the GUI thread       |
     \*-----------------------------------------------------*/
-    network_client->StopClient();
+    ClientTeardownMutex.lock();
 
-    /*-----------------------------------------------------*\
-    | Clear callbacks from the client before removal        |
-    \*-----------------------------------------------------*/
-    network_client->ClearCallbacks();
-
-    /*-----------------------------------------------------*\
-    | Find the client to remove and remove it from the      |
-    | clients list                                          |
-    \*-----------------------------------------------------*/
-    std::vector<NetworkClient*>::iterator client_it = std::find(clients.begin(), clients.end(), network_client);
-
-    if(client_it != clients.end())
+    if(std::find(clients_to_remove.begin(), clients_to_remove.end(), network_client) == clients_to_remove.end())
     {
-        clients.erase(client_it);
+        clients_to_remove.push_back(network_client);
     }
 
-    /*-----------------------------------------------------*\
-    | Delete the client                                     |
-    \*-----------------------------------------------------*/
-    delete network_client;
+    ClientTeardownPending = true;
 
-    UpdateDeviceList();
+    ClientTeardownMutex.unlock();
+
+    ClientTeardownCv.notify_all();
+}
+
+void ResourceManager::QueueClientTeardown()
+{
+    ClientTeardownMutex.lock();
+    ClientTeardownPending = true;
+    ClientTeardownMutex.unlock();
+
+    ClientTeardownCv.notify_all();
+}
+
+void ResourceManager::ClientTeardownThreadFunction()
+{
+    std::unique_lock<std::mutex> lock(ClientTeardownMutex);
+
+    while(ClientTeardownRunning)
+    {
+        if(!ClientTeardownPending)
+        {
+            ClientTeardownCv.wait(lock);
+            continue;
+        }
+
+        ClientTeardownPending = false;
+
+        /*-------------------------------------------------*\
+        | Take the clients queued for removal off the       |
+        | pending list                                      |
+        \*-------------------------------------------------*/
+        std::vector<NetworkClient*> remove_list;
+
+        remove_list.swap(clients_to_remove);
+
+        lock.unlock();
+
+        /*-------------------------------------------------*\
+        | Stop the clients being removed and take them out  |
+        | of the clients list. StopClient joins the client  |
+        | threads and moves the client's controllers to     |
+        | orphaned_controllers; the delete below frees them |
+        \*-------------------------------------------------*/
+        for(std::size_t remove_idx = 0; remove_idx < remove_list.size(); remove_idx++)
+        {
+            remove_list[remove_idx]->StopClient();
+            remove_list[remove_idx]->ClearCallbacks();
+
+            ClientListMutex.lock();
+
+            std::vector<NetworkClient*>::iterator client_it = std::find(clients.begin(), clients.end(), remove_list[remove_idx]);
+
+            if(client_it != clients.end())
+            {
+                clients.erase(client_it);
+            }
+
+            ClientListMutex.unlock();
+        }
+
+        /*-------------------------------------------------*\
+        | Collect the orphaned controllers from the clients |
+        | that remain. Ownership moves here, so a client    |
+        | deleted later cannot free them a second time      |
+        \*-------------------------------------------------*/
+        std::vector<RGBController*> orphaned;
+
+        ClientListMutex.lock();
+
+        for(std::size_t client_idx = 0; client_idx < clients.size(); client_idx++)
+        {
+            clients[client_idx]->TakeOrphanedControllers(orphaned);
+        }
+
+        ClientListMutex.unlock();
+
+        /*-------------------------------------------------*\
+        | Run the same sequence a rescan does while the     |
+        | controllers are still allocated. Consumers        |
+        | release them on DETECTION_STARTED, the GUI        |
+        | deletes the device pages (whose destructors use   |
+        | the controller) on UpdateDeviceList, and plugins  |
+        | rebuild on DETECTION_COMPLETE. Free them last,    |
+        | once nothing references them                      |
+        \*-------------------------------------------------*/
+        if(!remove_list.empty() || !orphaned.empty())
+        {
+            SignalResourceManagerUpdate(RESOURCEMANAGER_UPDATE_REASON_DETECTION_STARTED);
+
+            UpdateDeviceList();
+
+            SignalResourceManagerUpdate(RESOURCEMANAGER_UPDATE_REASON_DETECTION_COMPLETE);
+
+            /*---------------------------------------------*\
+            | Deleting a removed client frees the           |
+            | controllers StopClient left on it             |
+            \*---------------------------------------------*/
+            for(std::size_t remove_idx = 0; remove_idx < remove_list.size(); remove_idx++)
+            {
+                delete remove_list[remove_idx];
+            }
+
+            for(std::size_t orphaned_idx = 0; orphaned_idx < orphaned.size(); orphaned_idx++)
+            {
+                delete orphaned[orphaned_idx];
+            }
+        }
+
+        lock.lock();
+    }
 }
 
 /*---------------------------------------------------------*\
@@ -742,6 +880,8 @@ void ResourceManager::UpdateDeviceList()
     /*-----------------------------------------------------*\
     | Insert client controllers into controller list        |
     \*-----------------------------------------------------*/
+    ClientListMutex.lock();
+
     for(std::size_t client_idx = 0; client_idx < clients.size(); client_idx++)
     {
         std::vector<RGBController*> rgb_controllers_client  = clients[client_idx]->GetRGBControllers();
@@ -752,6 +892,8 @@ void ResourceManager::UpdateDeviceList()
         }
     }
 
+    ClientListMutex.unlock();
+
     /*-----------------------------------------------------*\
     | Update server list                                    |
     \*-----------------------------------------------------*/

ResourceManager.h

@@ -13,7 +13,9 @@
 
 #pragma once
 
+#include <condition_variable>
 #include <memory>
+#include <mutex>
 #include <vector>
 #include <functional>
 #include <thread>
@@ -102,6 +104,7 @@ public:
     \*-----------------------------------------------------*/
     void                                RegisterNetworkClient(NetworkClient* new_client);
     void                                UnregisterNetworkClient(NetworkClient* network_client);
+    void                                QueueClientTeardown();
 
     /*-----------------------------------------------------*\
     | Local Client Accessors                                |
@@ -139,6 +142,7 @@ public:
 
 private:
     bool                                AttemptLocalConnection();
+    void                                ClientTeardownThreadFunction();
     void                                SetupConfigurationDirectory();
 
     /*-----------------------------------------------------*\
@@ -214,6 +218,23 @@ private:
     | Network Clients                                       |
     \*-----------------------------------------------------*/
     std::vector<NetworkClient*>                 clients;
+    std::mutex                                  ClientListMutex;
+
+    /*-----------------------------------------------------*\
+    | Client teardown thread                                |
+    |                                                       |
+    | Frees controllers a lost connection left behind,      |
+    | inside a DETECTION_STARTED / DETECTION_COMPLETE       |
+    | sequence. Runs on its own thread: StopClient joins    |
+    | the client's listener thread, and the sequence's      |
+    | callbacks need the GUI thread free                    |
+    \*-----------------------------------------------------*/
+    std::thread*                                ClientTeardownThread;
+    std::mutex                                  ClientTeardownMutex;
+    std::condition_variable                     ClientTeardownCv;
+    bool                                        ClientTeardownPending;
+    bool                                        ClientTeardownRunning;
+    std::vector<NetworkClient*>                 clients_to_remove;
 
     /*-----------------------------------------------------*\
     | Device List Mutex                                     |