Merge pull request #1017 from SignalK/fix/894-registry-cleanup

fix(core): clean up static registries on destruction (#894)

Author: mairas

src/sensesp/signalk/signalk_emitter.cpp

@@ -1,5 +1,7 @@
 #include "signalk_emitter.h"
 
+#include <algorithm>
+
 namespace sensesp {
 
 std::vector<SKEmitter*> SKEmitter::sources_;
@@ -8,6 +10,13 @@ SKEmitter::SKEmitter(const String& sk_path) : sk_path_{sk_path} {
   sources_.push_back(this);
 }
 
+SKEmitter::~SKEmitter() {
+  sources_.erase(std::remove(sources_.begin(), sources_.end(), this),
+                 sources_.end());
+}
+
+void SKEmitter::clear_registry() { sources_.clear(); }
+
 void SKEmitter::add_metadata(JsonArray& meta) {
   SKMetadata* my_meta = this->get_metadata();
   if (my_meta != NULL) {

src/sensesp/signalk/signalk_emitter.h

@@ -26,6 +26,12 @@ class SKEmitter : virtual public Observable {
    */
   SKEmitter(const String& sk_path);
 
+  /**
+   * Unregisters this emitter from the static source registry so the
+   * registry never holds a dangling pointer.
+   */
+  virtual ~SKEmitter();
+
   /**
    * Returns the data to be reported to the server as
    * a Signal K json object.
@@ -65,6 +71,12 @@ class SKEmitter : virtual public Observable {
 
   static const std::vector<SKEmitter*>& get_sources() { return sources_; }
 
+  /**
+   * Empties the source registry. Intended for clean app restart and test
+   * isolation; not for normal runtime use.
+   */
+  static void clear_registry();
+
  protected:
   String sk_path_{};
 

src/sensesp/signalk/signalk_listener.cpp

@@ -1,5 +1,7 @@
 #include "signalk_listener.h"
 
+#include <algorithm>
+
 #include "sensesp/system/saveable.h"
 
 namespace sensesp {
@@ -19,6 +21,19 @@ SKListener::SKListener(const String &sk_path, int listen_delay,
   this->load();
 }
 
+SKListener::~SKListener() {
+  take_semaphore();
+  listeners_.erase(std::remove(listeners_.begin(), listeners_.end(), this),
+                   listeners_.end());
+  release_semaphore();
+}
+
+void SKListener::clear_registry() {
+  take_semaphore();
+  listeners_.clear();
+  release_semaphore();
+}
+
 bool SKListener::take_semaphore(uint64_t timeout_ms) {
   if (timeout_ms == 0) {
     return xSemaphoreTakeRecursive(semaphore_, portMAX_DELAY) == pdTRUE;

src/sensesp/signalk/signalk_listener.h

@@ -35,6 +35,13 @@ class SKListener : virtual public Observable, public FileSystemSaveable {
   SKListener(const String& sk_path, int listen_delay,
              const String& config_path = "");
 
+  /**
+   * Unregisters this listener from the static registry so the registry never
+   * holds a dangling pointer. Takes the listener semaphore because the WS
+   * client task iterates the registry under the same lock.
+   */
+  virtual ~SKListener();
+
   /**
    * Returns the current Signal K path. An empty string
    * is returned if this particular source is not configured
@@ -66,6 +73,12 @@ class SKListener : virtual public Observable, public FileSystemSaveable {
 
   static const std::vector<SKListener*>& get_listeners() { return listeners_; }
 
+  /**
+   * Empties the listener registry. Intended for clean app restart and test
+   * isolation; not for normal runtime use. Takes the listener semaphore.
+   */
+  static void clear_registry();
+
   static bool take_semaphore(uint64_t timeout_ms = 0);
   static void release_semaphore();
 

src/sensesp/signalk/signalk_put_request_listener.cpp

@@ -1,5 +1,9 @@
 #include "signalk_put_request_listener.h"
 
+#include <algorithm>
+
+#include "signalk_listener.h"
+
 namespace sensesp {
 
 std::vector<SKPutListener*> SKPutListener::listeners_;
@@ -8,4 +12,19 @@ SKPutListener::SKPutListener(const String& sk_path) : sk_path{sk_path} {
   listeners_.push_back(this);
 }
 
+SKPutListener::~SKPutListener() {
+  // The WS client iterates this registry while holding the SKListener
+  // semaphore, so mutate it under the same lock.
+  SKListener::take_semaphore();
+  listeners_.erase(std::remove(listeners_.begin(), listeners_.end(), this),
+                   listeners_.end());
+  SKListener::release_semaphore();
+}
+
+void SKPutListener::clear_registry() {
+  SKListener::take_semaphore();
+  listeners_.clear();
+  SKListener::release_semaphore();
+}
+
 }  // namespace sensesp

src/sensesp/signalk/signalk_put_request_listener.h

@@ -26,6 +26,13 @@ class SKPutListener : virtual public Observable {
    */
   SKPutListener(const String& sk_path);
 
+  /**
+   * Unregisters this listener from the static registry so the registry never
+   * holds a dangling pointer. Takes the SKListener semaphore because the WS
+   * client task iterates this registry under that same lock.
+   */
+  virtual ~SKPutListener();
+
   String& get_sk_path() { return sk_path; }
 
   virtual void parse_value(const JsonObject& put) = 0;
@@ -34,6 +41,12 @@ class SKPutListener : virtual public Observable {
     return listeners_;
   }
 
+  /**
+   * Empties the listener registry. Intended for clean app restart and test
+   * isolation; not for normal runtime use. Takes the SKListener semaphore.
+   */
+  static void clear_registry();
+
  protected:
   String sk_path{};
 

src/sensesp/transforms/transform.cpp

@@ -17,4 +17,8 @@ TransformBase::TransformBase(const String& config_path)
   transforms_.insert(this);
 }
 
+TransformBase::~TransformBase() { transforms_.erase(this); }
+
+void TransformBase::clear_registry() { transforms_.clear(); }
+
 }  // namespace sensesp

src/sensesp/transforms/transform.h

@@ -31,13 +31,28 @@ class TransformBase : public FileSystemSaveable {
  public:
   TransformBase(const String& config_path);
 
-  // Primary purpose of this was to supply Signal K sources
-  // (now handled by SKEmitter::get_sources). Should
-  // this be deprecated?
+  /**
+   * Unregisters this transform from the static registry so the registry
+   * never holds a dangling pointer.
+   */
+  virtual ~TransformBase();
+
+  // The transform registry's original purpose (supplying Signal K sources) is
+  // now handled by SKEmitter::get_sources(); nothing in the framework consumes
+  // get_transforms() anymore. get_sources() is not a drop-in replacement -- it
+  // returns only the Signal K emitters, not the full transform set, for which
+  // there is no equivalent accessor.
+  [[deprecated("Deprecated and unused internally; no direct replacement")]]
   static const std::set<TransformBase*>& get_transforms() {
     return transforms_;
   }
 
+  /**
+   * Empties the transform registry. Intended for clean app restart and test
+   * isolation; not for normal runtime use.
+   */
+  static void clear_registry();
+
  private:
   static std::set<TransformBase*> transforms_;
 };

src/sensesp/ui/config_item.cpp

@@ -4,6 +4,8 @@ namespace sensesp {
 
 std::map<String, std::shared_ptr<ConfigItemBase>> ConfigItemBase::config_items_;
 
+void ConfigItemBase::clear_registry() { config_items_.clear(); }
+
 template <>
 const char* get_schema_type_string(const int /*dummy*/) {
   return "number";

src/sensesp/ui/config_item.h

@@ -70,6 +70,8 @@ std::shared_ptr<ConfigItemT<T>> ConfigItem(std::shared_ptr<T>);
 class ConfigItemBase
     : virtual public std::enable_shared_from_this<ConfigItemBase> {
  public:
+  virtual ~ConfigItemBase() = default;
+
   const String& get_title() const { return title_; }
   ConfigItemBase* set_title(const String& title) {
     title_ = title;
@@ -161,6 +163,20 @@ class ConfigItemBase
     return sorted_config_items;
   }
 
+  /**
+   * @brief Empty the config item registry.
+   *
+   * Releases the registry's shared_ptr references. Intended for clean app
+   * restart and test isolation; not for normal runtime use.
+   *
+   * Unlike the raw-pointer registries (SKEmitter, Transform, StatusPageItem),
+   * this registry owns its entries via shared_ptr, so there is no
+   * unregister-on-destruction: clearing the map releases the objects. A
+   * self-erasing destructor here would be re-entrant and is deliberately
+   * absent.
+   */
+  static void clear_registry();
+
   virtual bool load() = 0;
   virtual bool refresh() = 0;
   virtual bool save() = 0;