Make it possible to dispose of unconsumed collection buffers (#502)

* Add unittest to cover parts the CollectionBufferFactory

* Make sure to cleanup buffers in CollectionData constructor

* Make the ROOTFrameData properly cleanup after itself

include/podio/CollectionBuffers.h

@@ -53,15 +53,19 @@ struct CollectionReadBuffers {
   using CreateFuncT = std::function<std::unique_ptr<podio::CollectionBase>(podio::CollectionReadBuffers, bool)>;
   using RecastFuncT = std::function<void(CollectionReadBuffers&)>;
 
+  using DeleteFuncT = std::function<void(CollectionReadBuffers&)>;
+
   CollectionReadBuffers(void* d, CollRefCollection* ref, VectorMembersInfo* vec, SchemaVersionT version,
-                        std::string_view typ, CreateFuncT&& createFunc, RecastFuncT&& recastFunc) :
+                        std::string_view typ, CreateFuncT&& createFunc, RecastFuncT&& recastFunc,
+                        DeleteFuncT&& deleteFunc) :
       data(d),
       references(ref),
       vectorMembers(vec),
       schemaVersion(version),
       type(typ),
       createCollection(std::move(createFunc)),
-      recast(std::move(recastFunc)) {
+      recast(std::move(recastFunc)),
+      deleteBuffers(std::move(deleteFunc)) {
   }
 
   CollectionReadBuffers() = default;
@@ -105,6 +109,11 @@ struct CollectionReadBuffers {
   // generation) to do the second cast and assign the result of that to the data
   // field again.
   RecastFuncT recast{};
+
+  // Workaround for https://github.com/AIDASoft/podio#500
+  // We need a function that explicitly deletes the buffers, but for this we
+  // need type information, so we attach a delete function at generation time
+  DeleteFuncT deleteBuffers{};
 };
 
 } // namespace podio

include/podio/ROOTFrameData.h

@@ -19,7 +19,7 @@ class ROOTFrameData {
   using BufferMap = std::unordered_map<std::string, podio::CollectionReadBuffers>;
 
   ROOTFrameData() = delete;
-  ~ROOTFrameData() = default;
+  ~ROOTFrameData();
   ROOTFrameData(ROOTFrameData&&) = default;
   ROOTFrameData& operator=(ROOTFrameData&&) = default;
   ROOTFrameData(const ROOTFrameData&) = delete;
@@ -65,6 +65,14 @@ class ROOTFrameData {
   CollIDPtr m_idTable{nullptr};
   podio::GenericParameters m_parameters{};
 };
+
+// Interim workaround for https://github.com/AIDASoft/podio#500
+inline ROOTFrameData::~ROOTFrameData() {
+  for (auto& [_, buffer] : m_buffers) {
+    buffer.deleteBuffers(buffer);
+  }
+}
+
 } // namespace podio
 
 #endif // PODIO_ROOTFRAMEDATA_H

python/templates/CollectionData.cc.jinja2

@@ -43,6 +43,10 @@
   m_vec_{{ member.name }}.reset(podio::CollectionReadBuffers::asVector<{{ member.full_type }}>(m_vecmem_info[{{ loop.index0 }}].second));
 {% endfor %}
   }
+
+  // Cleanup these to avoid leaking them
+  delete buffers.references;
+  delete buffers.vectorMembers;
 }
 
 void {{ class_type }}::clear(bool isSubsetColl) {

python/templates/macros/collections.jinja2

@@ -203,6 +203,21 @@ podio::CollectionReadBuffers createBuffersV{{ schemaVersion }}(bool isSubset) {
     }
   };
 
+  readBuffers.deleteBuffers = [](podio::CollectionReadBuffers& buffers) {
+    if (buffers.data) {
+      // If we have data then we are not a subset collection and we have to
+      // clean up all type erased buffers by casting them back to something that
+      // we can delete
+      delete static_cast<{{ class.full_type }}DataContainer*>(buffers.data);
+{% for member in VectorMembers %}
+      delete static_cast<std::vector<{{ member.full_type }}>*>((*buffers.vectorMembers)[{{ loop.index0 }}].second);
+{% endfor %}
+
+    }
+    delete buffers.references;
+    delete buffers.vectorMembers;
+  };
+
   return readBuffers;
 }
 

src/UserDataCollection.cc

@@ -20,18 +20,19 @@ namespace {
     // Register with schema version 1 to allow for potential changes
     CollectionBufferFactory::mutInstance().registerCreationFunc(
         userDataCollTypeName<T>(), UserDataCollection<T>::schemaVersion, [](bool) {
-          return podio::CollectionReadBuffers{new std::vector<T>(),
-                                              nullptr,
-                                              nullptr,
-                                              podio::UserDataCollection<T>::schemaVersion,
-                                              podio::userDataCollTypeName<T>(),
-                                              [](podio::CollectionReadBuffers buffers, bool) {
-                                                return std::make_unique<UserDataCollection<T>>(
-                                                    std::move(*buffers.dataAsVector<T>()));
-                                              },
-                                              [](podio::CollectionReadBuffers& buffers) {
-                                                buffers.data = podio::CollectionWriteBuffers::asVector<T>(buffers.data);
-                                              }};
+          return podio::CollectionReadBuffers{
+              new std::vector<T>(),
+              nullptr,
+              nullptr,
+              podio::UserDataCollection<T>::schemaVersion,
+              podio::userDataCollTypeName<T>(),
+              [](podio::CollectionReadBuffers buffers, bool) {
+                return std::make_unique<UserDataCollection<T>>(std::move(*buffers.dataAsVector<T>()));
+              },
+              [](podio::CollectionReadBuffers& buffers) {
+                buffers.data = podio::CollectionWriteBuffers::asVector<T>(buffers.data);
+              },
+              [](podio::CollectionReadBuffers& buffers) { delete static_cast<std::vector<T>*>(buffers.data); }};
         });
 
     // For now passing the same schema version for from and current versions

tests/unittests/CMakeLists.txt

@@ -40,7 +40,7 @@ if(NOT Catch2_FOUND)
 endif()
 
 find_package(Threads REQUIRED)
-add_executable(unittest_podio unittest.cpp frame.cpp)
+add_executable(unittest_podio unittest.cpp frame.cpp buffer_factory.cpp)
 target_link_libraries(unittest_podio PUBLIC TestDataModel PRIVATE Catch2::Catch2WithMain Threads::Threads podio::podioRootIO)
 if (ENABLE_SIO)
   target_link_libraries(unittest_podio PRIVATE podio::podioSioIO)

tests/unittests/buffer_factory.cpp

@@ -0,0 +1,152 @@
+#include "datamodel/ExampleHitCollectionData.h"
+#include "podio/CollectionBufferFactory.h"
+
+#include "datamodel/DatamodelDefinition.h"
+#include "datamodel/ExampleClusterCollection.h"
+#include "datamodel/ExampleHitCollection.h"
+#include "datamodel/ExampleWithVectorMemberCollection.h"
+
+#include "catch2/catch_test_macros.hpp"
+
+TEST_CASE("createBuffers", "[internals][memory-management]") {
+  const auto& factory = podio::CollectionBufferFactory::instance();
+
+  SECTION("Simple type") {
+    auto maybeBuffers = factory.createBuffers("ExampleHitCollection", datamodel::meta::schemaVersion, false);
+
+    REQUIRE(maybeBuffers.has_value());
+    auto buffers = maybeBuffers.value();
+
+    // All pointers should be initialized
+    REQUIRE(buffers.data);
+    REQUIRE(buffers.references);
+    REQUIRE(buffers.vectorMembers);
+
+    // Cast this to something useful again
+    auto dataBuffers = static_cast<ExampleHitDataContainer*>(buffers.data);
+    REQUIRE(dataBuffers->empty());
+    REQUIRE(buffers.references->empty());
+    REQUIRE(buffers.vectorMembers->empty());
+
+    // Do the necessary cleanup
+    delete dataBuffers;
+    delete buffers.references;
+    delete buffers.vectorMembers;
+  }
+
+  SECTION("Type with relations") {
+    auto maybeBuffers = factory.createBuffers("ExampleClusterCollection", datamodel::meta::schemaVersion, false);
+
+    REQUIRE(maybeBuffers.has_value());
+    auto buffers = maybeBuffers.value();
+
+    // All pointers should be initialized
+    REQUIRE(buffers.data);
+    REQUIRE(buffers.references);
+    REQUIRE(buffers.vectorMembers);
+
+    // Cast this to something useful again
+    auto dataBuffers = static_cast<ExampleClusterDataContainer*>(buffers.data);
+    REQUIRE(dataBuffers->empty());
+    REQUIRE(buffers.references->size() == 2);
+    REQUIRE(buffers.vectorMembers->empty());
+
+    // Do the necessary cleanup
+    delete dataBuffers;
+    delete buffers.references;
+    delete buffers.vectorMembers;
+  }
+
+  SECTION("Type with vector members") {
+    auto maybeBuffers =
+        factory.createBuffers("ExampleWithVectorMemberCollection", datamodel::meta::schemaVersion, false);
+
+    REQUIRE(maybeBuffers.has_value());
+    auto buffers = maybeBuffers.value();
+
+    // All pointers should be initialized
+    REQUIRE(buffers.data);
+    REQUIRE(buffers.references);
+    REQUIRE(buffers.vectorMembers);
+
+    // Cast this to something useful again
+    auto dataBuffers = static_cast<ExampleWithVectorMemberDataContainer*>(buffers.data);
+    REQUIRE(dataBuffers->empty());
+    REQUIRE(buffers.references->empty());
+    REQUIRE(buffers.vectorMembers->size() == 1);
+
+    // Do the necessary cleanup
+    delete dataBuffers;
+    delete buffers.references;
+    auto vecBuffer = static_cast<std::vector<int>*>((*buffers.vectorMembers)[0].second);
+    delete vecBuffer;
+    delete buffers.vectorMembers;
+  }
+}
+
+TEST_CASE("construct CollectionData empty buffers", "[internals][memory-management]") {
+  const auto& factory = podio::CollectionBufferFactory::instance();
+
+  SECTION("Simple type") {
+    auto buffers = factory.createBuffers("ExampleHitCollection", datamodel::meta::schemaVersion, false).value();
+    auto collData = ExampleHitCollectionData(std::move(buffers), false);
+
+    // These tests either get flagged by sanitizers or they work
+    REQUIRE(true);
+  }
+
+  SECTION("Type with relation") {
+    auto buffers = factory.createBuffers("ExampleClusterCollection", datamodel::meta::schemaVersion, false).value();
+    auto collData = ExampleClusterCollectionData(std::move(buffers), false);
+
+    // These tests either get flagged by sanitizers or they work
+    REQUIRE(true);
+  }
+
+  SECTION("Type with vector members") {
+    auto buffers =
+        factory.createBuffers("ExampleWithVectorMemberCollection", datamodel::meta::schemaVersion, false).value();
+
+    auto collData = ExampleWithVectorMemberCollectionData(std::move(buffers), false);
+
+    // These tests either get flagged by sanitizers or they work
+    REQUIRE(true);
+  }
+}
+
+TEST_CASE("construct CollectionData non-empty buffers", "[internals][memory-management]") {
+  const auto& factory = podio::CollectionBufferFactory::instance();
+
+  SECTION("Simple type") {
+    auto buffers = factory.createBuffers("ExampleHitCollection", datamodel::meta::schemaVersion, false).value();
+
+    // Cast this to something useful again to add one data element
+    auto dataBuffers = static_cast<ExampleHitDataContainer*>(buffers.data);
+    dataBuffers->emplace_back(ExampleHitData{0xcaffee, 1.0, 2.0, 3.0, 125.0});
+
+    auto collData = ExampleHitCollectionData(std::move(buffers), false);
+  }
+
+  SECTION("Type with relations") {
+    auto buffers = factory.createBuffers("ExampleClusterCollection", datamodel::meta::schemaVersion, false).value();
+
+    // Cast this to something useful again to add one data element
+    auto dataBuffers = static_cast<ExampleClusterDataContainer*>(buffers.data);
+    dataBuffers->emplace_back(ExampleClusterData{125.0});
+
+    // Add an ObjectID (we don't need to resolve it in any way)
+    (*buffers.references)[0]->emplace_back(podio::ObjectID{42, 42});
+
+    auto collData = ExampleClusterCollectionData(std::move(buffers), false);
+  }
+
+  SECTION("Type with vector members") {
+    auto buffers =
+        factory.createBuffers("ExampleWithVectorMemberCollection", datamodel::meta::schemaVersion, false).value();
+
+    auto vecBuffer = static_cast<std::vector<int>*>((*buffers.vectorMembers)[0].second);
+    vecBuffer->emplace_back(42);
+
+    auto collData = ExampleWithVectorMemberCollectionData(std::move(buffers), false);
+  }
+}