From 3601eabbf8d7b4e103437dec313859c65b93ed9e Mon Sep 17 00:00:00 2001 From: Ben Meadors Date: Tue, 3 Mar 2026 13:37:15 -0600 Subject: [PATCH] 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 | 2 ++ src/mesh/StreamAPI.h | 6 +++--- src/mesh/api/PacketAPI.h | 4 ++-- src/mesh/api/ServerAPI.cpp | 10 ++++++++-- src/mesh/api/ServerAPI.h | 9 +++++---- src/mesh/http/ContentHandler.h | 4 ++-- src/mesh/raspihttp/PiWebServer.h | 5 +++-- test/test_mqtt/MQTT.cpp | 13 +++++++++---- 8 files changed, 34 insertions(+), 19 deletions(-) diff --git a/src/mesh/PhoneAPI.cpp b/src/mesh/PhoneAPI.cpp index 9050ee89d..a02f96ac5 100644 --- a/src/mesh/PhoneAPI.cpp +++ b/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; diff --git a/src/mesh/StreamAPI.h b/src/mesh/StreamAPI.h index 97e231f23..c724871cb 100644 --- a/src/mesh/StreamAPI.h +++ b/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 */ diff --git a/src/mesh/api/PacketAPI.h b/src/mesh/api/PacketAPI.h index 357eb05c2..aececf85e 100644 --- a/src/mesh/api/PacketAPI.h +++ b/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 {} diff --git a/src/mesh/api/ServerAPI.cpp b/src/mesh/api/ServerAPI.cpp index 1a506421c..7bb1a8108 100644 --- a/src/mesh/api/ServerAPI.cpp +++ b/src/mesh/api/ServerAPI.cpp @@ -31,6 +31,7 @@ template int32_t ServerAPI::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 void APIServerPort::init() template int32_t APIServerPort::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 int32_t APIServerPort::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 diff --git a/src/mesh/api/ServerAPI.h b/src/mesh/api/ServerAPI.h index 111314476..2da77c8e9 100644 --- a/src/mesh/api/ServerAPI.h +++ b/src/mesh/api/ServerAPI.h @@ -1,6 +1,7 @@ #pragma once #include "StreamAPI.h" +#include #define SERVER_API_DEFAULT_PORT 4403 @@ -21,15 +22,15 @@ template 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 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 openAPI; #if defined(RAK_4631) || defined(RAK11310) // Track wait time for RAK13800 Ethernet requests int32_t waitTime = 100; diff --git a/src/mesh/http/ContentHandler.h b/src/mesh/http/ContentHandler.h index 6efdb59b7..ed182ad76 100644 --- a/src/mesh/http/ContentHandler.h +++ b/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 }; \ No newline at end of file diff --git a/src/mesh/raspihttp/PiWebServer.h b/src/mesh/raspihttp/PiWebServer.h index 5a4adedaa..74b094f8c 100644 --- a/src/mesh/raspihttp/PiWebServer.h +++ b/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 diff --git a/test/test_mqtt/MQTT.cpp b/test/test_mqtt/MQTT.cpp index afc8a399a..7982dcdb5 100644 --- a/test/test_mqtt/MQTT.cpp +++ b/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. - TEST_ASSERT_TRUE(loopUntil( - [] { return pubsub->subscriptions_.count("msh/2/e/test/+") && pubsub->subscriptions_.count("msh/2/e/PKI/+"); })); + // Clear any side effects from startup (e.g. map report triggered by runOnce) + mockMeshService->messages_.clear(); + mockMeshService->notifications_.clear(); + mockRouter->packets_.clear(); + mockRoutingModule->ackNacks_.clear(); } PubSubClient &getPubSub() { return pubSub; } };