From 224a2a98024c81170616a3b34ab20b512b1fed3c Mon Sep 17 00:00:00 2001
From: Nathan Brei <nbrei@jlab.org>
Date: Wed, 1 May 2024 13:03:07 -0400
Subject: [PATCH] Bugfix: Components not being destroyed when they should

We are using shared_ptrs to manage the lifetimes of all JServices. JServiceLocator was holding onto some of these shared_ptrs and wasn't being deleted. The fix for now is to make sure JServiceLocator gets deleted when JApplication does. The longer-term fix is to stop holding on to JServices using shared_ptrs, since users may trivially create cycles. An even better option is to use Service<T> helpers with weak or raw pointers internally.

This addresses issue #284.
---
 src/libraries/JANA/JApplication.cc            |  2 +-
 src/libraries/JANA/JApplicationFwd.h          |  3 +-
 .../Components/JEventProcessorTests.cc        | 41 ++++++++++++-------
 3 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/src/libraries/JANA/JApplication.cc b/src/libraries/JANA/JApplication.cc
index e3beba4bc..9dd5d4e53 100644
--- a/src/libraries/JANA/JApplication.cc
+++ b/src/libraries/JANA/JApplication.cc
@@ -39,7 +39,7 @@ JApplication::JApplication(JParameterManager* params) {
     }
     m_component_manager = std::make_shared<JComponentManager>();
     m_plugin_loader = std::make_shared<JPluginLoader>();
-    m_service_locator = new JServiceLocator;
+    m_service_locator = std::make_unique<JServiceLocator>();
 
     ProvideService(m_params);
     ProvideService(m_component_manager);
diff --git a/src/libraries/JANA/JApplicationFwd.h b/src/libraries/JANA/JApplicationFwd.h
index 42f95f3b4..678d7fc3d 100644
--- a/src/libraries/JANA/JApplicationFwd.h
+++ b/src/libraries/JANA/JApplicationFwd.h
@@ -135,7 +135,8 @@ class JApplication {
 private:
 
     JLogger m_logger;
-    JServiceLocator* m_service_locator;
+
+    std::unique_ptr<JServiceLocator> m_service_locator;
 
     std::shared_ptr<JParameterManager> m_params;
     std::shared_ptr<JPluginLoader> m_plugin_loader;
diff --git a/src/programs/unit_tests/Components/JEventProcessorTests.cc b/src/programs/unit_tests/Components/JEventProcessorTests.cc
index 1a6ee21b3..cba55e739 100644
--- a/src/programs/unit_tests/Components/JEventProcessorTests.cc
+++ b/src/programs/unit_tests/Components/JEventProcessorTests.cc
@@ -8,11 +8,15 @@ struct MyEventProcessor : public JEventProcessor {
     int init_count = 0;
     int process_count = 0;
     int finish_count = 0;
+    int* destroy_count = nullptr;
 
     MyEventProcessor() {
         SetCallbackStyle(CallbackStyle::ExpertMode);
         SetTypeName(NAME_OF_THIS);
     }
+    ~MyEventProcessor() {
+        (*destroy_count)++;
+    }
     void Init() override {
         LOG_INFO(GetLogger()) << "Init() called" << LOG_END;
         init_count++;
@@ -32,20 +36,27 @@ TEST_CASE("JEventProcessor_ExpertMode_ProcessCount") {
     LOG << "Running test: JEventProcessor_ExpertMode_ProcessCount" << LOG_END;
 
     auto sut = new MyEventProcessor;
-
-    JApplication app;
-    app.SetParameterValue("log:global", "off");
-    app.SetParameterValue("log:info", "MyEventProcessor");
-    app.SetParameterValue("jana:nevents", 5);
-
-    app.Add(new JEventSource);
-    app.Add(sut);
-    app.Run();
-
-    REQUIRE(sut->init_count == 1);
-    REQUIRE(sut->process_count == 5);
-    REQUIRE(sut->GetEventCount() == 5);
-    REQUIRE(sut->finish_count == 1);
-
+    int destroy_count = 0;
+    sut->destroy_count = &destroy_count;
+
+    {
+        JApplication app;
+        app.SetParameterValue("log:global", "off");
+        app.SetParameterValue("log:info", "MyEventProcessor");
+        app.SetParameterValue("jana:nevents", 5);
+
+        app.Add(new JEventSource);
+        app.Add(sut);
+        app.Run();
+
+        REQUIRE(sut->init_count == 1);
+        REQUIRE(sut->process_count == 5);
+        REQUIRE(sut->GetEventCount() == 5);
+        REQUIRE(sut->finish_count == 1);
+        REQUIRE(destroy_count == 0);
+
+        // App goes out of scope; sut should be destroyed
+    }
+    REQUIRE(destroy_count == 1);
 }