Improve resource cleanup on connection close (and make server API a unique pointer) (#9799)

* Improve resource cleanup on connection close

* Copilot had some good feedback. Let's just make the api a unique pointer

* Update src/mesh/api/ServerAPI.cpp

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Copilot stupidly suggesting we call protected methods

* Gotta do it in the superclasses as well

* Fix moar

* Refactor MQTT unit test to ensure proper subscription handling and clear side effects

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

src/mesh/PhoneAPI.cpp

@@ -122,6 +122,8 @@ void PhoneAPI::close()
         }
         packetForPhone = NULL;
         filesManifest.clear();
+        filesManifest.shrink_to_fit();
+        lastPortNumToRadio.clear();
         fromRadioNum = 0;
         config_nonce = 0;
         config_state = 0;

src/mesh/StreamAPI.h

@@ -52,6 +52,9 @@ class StreamAPI : public PhoneAPI
     virtual int32_t runOncePart();
     virtual int32_t runOncePart(char *buf, uint16_t bufLen);
 
+    /// Check the current underlying physical link to see if the client is currently connected
+    virtual bool checkIsConnected() override = 0;
+
   private:
     /**
      * Read any rx chars from the link and call handleToRadio
@@ -73,9 +76,6 @@ class StreamAPI : public PhoneAPI
 
     virtual void onConnectionChanged(bool connected) override;
 
-    /// Check the current underlying physical link to see if the client is currently connected
-    virtual bool checkIsConnected() override = 0;
-
     /**
      * Send the current txBuffer over our stream
      */

src/mesh/api/PacketAPI.h

@@ -15,11 +15,11 @@ class PacketAPI : public PhoneAPI, public concurrency::OSThread
     static PacketAPI *create(PacketServer *_server);
     virtual ~PacketAPI(){};
     virtual int32_t runOnce();
+    // Check the current underlying physical queue to see if the client is fetching packets
+    bool checkIsConnected() override;
 
   protected:
     explicit PacketAPI(PacketServer *_server);
-    // Check the current underlying physical queue to see if the client is fetching packets
-    bool checkIsConnected() override;
 
     void onNowHasData(uint32_t fromRadioNum) override {}
     void onConnectionChanged(bool connected) override {}

src/mesh/api/ServerAPI.cpp

@@ -31,6 +31,7 @@ template <class T> int32_t ServerAPI<T>::runOnce()
         return StreamAPI::runOncePart();
     } else {
         LOG_INFO("Client dropped connection, suspend API service");
+        close();
         enabled = false; // we no longer need to run
         return 0;
     }
@@ -45,6 +46,11 @@ template <class T, class U> void APIServerPort<T, U>::init()
 
 template <class T, class U> int32_t APIServerPort<T, U>::runOnce()
 {
+    // Clean up previous connection if its client already disconnected
+    if (openAPI && !openAPI->checkIsConnected()) {
+        openAPI.reset();
+    }
+
 #ifdef ARCH_ESP32
 #if ESP_ARDUINO_VERSION >= ESP_ARDUINO_VERSION_VAL(3, 0, 0)
     auto client = U::accept();
@@ -70,10 +76,10 @@ template <class T, class U> int32_t APIServerPort<T, U>::runOnce()
             }
 #endif
             LOG_INFO("Force close previous TCP connection");
-            delete openAPI;
+            openAPI.reset();
         }
 
-        openAPI = new T(client);
+        openAPI.reset(new T(client));
     }
 
 #if RAK_4631

src/mesh/api/ServerAPI.h

@@ -1,6 +1,7 @@
 #pragma once
 
 #include "StreamAPI.h"
+#include <memory>
 
 #define SERVER_API_DEFAULT_PORT 4403
 
@@ -21,15 +22,15 @@ template <class T> class ServerAPI : public StreamAPI, private concurrency::OSTh
     /// override close to also shutdown the TCP link
     virtual void close();
 
+    /// Check the current underlying physical link to see if the client is currently connected
+    virtual bool checkIsConnected() override;
+
   protected:
     /// We override this method to prevent publishing EVENT_SERIAL_CONNECTED/DISCONNECTED for wifi links (we want the board to
     /// stay in the POWERED state to prevent disabling wifi)
     virtual void onConnectionChanged(bool connected) override {}
 
     virtual int32_t runOnce() override; // Check for dropped client connections
-
-    /// Check the current underlying physical link to see if the client is currently connected
-    virtual bool checkIsConnected() override;
 };
 
 /**
@@ -42,7 +43,7 @@ template <class T, class U> class APIServerPort : public U, private concurrency:
      * FIXME: We currently only allow one open TCP connection at a time, because we depend on the loop() call in this class to
      * delegate to the worker.  Once coroutines are implemented we can relax this restriction.
      */
-    T *openAPI = NULL;
+    std::unique_ptr<T> openAPI;
 #if defined(RAK_4631) || defined(RAK11310)
     // Track wait time for RAK13800 Ethernet requests
     int32_t waitTime = 100;

src/mesh/http/ContentHandler.h

@@ -27,10 +27,10 @@ class HttpAPI : public PhoneAPI
   public:
     HttpAPI() { api_type = TYPE_HTTP; }
 
+    /// Check the current underlying physical link to see if the client is currently connected
+    virtual bool checkIsConnected() override { return true; } // FIXME, be smarter about this
   private:
     // Nothing here yet
 
   protected:
-    /// Check the current underlying physical link to see if the client is currently connected
-    virtual bool checkIsConnected() override { return true; } // FIXME, be smarter about this
 };

src/mesh/raspihttp/PiWebServer.h

@@ -29,12 +29,13 @@ class HttpAPI : public PhoneAPI
   public:
     HttpAPI() { api_type = TYPE_HTTP; }
 
+    /// Check the current underlying physical link to see if the client is currently connected
+    virtual bool checkIsConnected() override { return true; } // FIXME, be smarter about this
+
   private:
     // Nothing here yet
 
   protected:
-    /// Check the current underlying physical link to see if the client is currently connected
-    virtual bool checkIsConnected() override { return true; } // FIXME, be smarter about this
 };
 
 class PiWebServerThread

test/test_mqtt/MQTT.cpp

@@ -291,11 +291,16 @@ class MQTTUnitTest : public MQTT
 
         if (!moduleConfig.mqtt.enabled || moduleConfig.mqtt.proxy_to_client_enabled || *moduleConfig.mqtt.root) {
             loopUntil([] { return true; }); // Loop once
-            return;
+        } else {
+            // Wait for MQTT to subscribe to all topics.
+            TEST_ASSERT_TRUE(loopUntil(
+                [] { return pubsub->subscriptions_.count("msh/2/e/test/+") && pubsub->subscriptions_.count("msh/2/e/PKI/+"); }));
         }
-        // Wait for MQTT to subscribe to all topics.
+        // Clear any side effects from startup (e.g. map report triggered by runOnce)
-        TEST_ASSERT_TRUE(loopUntil(
+        mockMeshService->messages_.clear();
-            [] { return pubsub->subscriptions_.count("msh/2/e/test/+") && pubsub->subscriptions_.count("msh/2/e/PKI/+"); }));
+        mockMeshService->notifications_.clear();
+        mockRouter->packets_.clear();
+        mockRoutingModule->ackNacks_.clear();
     }
     PubSubClient &getPubSub() { return pubSub; }
 };