diff --git a/include/podio/CollectionBuffers.h b/include/podio/CollectionBuffers.h index b51161c2d..834c465c9 100644 --- a/include/podio/CollectionBuffers.h +++ b/include/podio/CollectionBuffers.h @@ -53,15 +53,19 @@ struct CollectionReadBuffers { using CreateFuncT = std::function(podio::CollectionReadBuffers, bool)>; using RecastFuncT = std::function; + using DeleteFuncT = std::function; + 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 diff --git a/include/podio/ROOTFrameData.h b/include/podio/ROOTFrameData.h index 157e3fbea..3cc63d04a 100644 --- a/include/podio/ROOTFrameData.h +++ b/include/podio/ROOTFrameData.h @@ -19,7 +19,7 @@ class ROOTFrameData { using BufferMap = std::unordered_map; 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 diff --git a/python/templates/CollectionData.cc.jinja2 b/python/templates/CollectionData.cc.jinja2 index cfa143060..988f4876d 100644 --- a/python/templates/CollectionData.cc.jinja2 +++ b/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) { diff --git a/python/templates/macros/collections.jinja2 b/python/templates/macros/collections.jinja2 index efc7c24cb..cda1cf948 100644 --- a/python/templates/macros/collections.jinja2 +++ b/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*>((*buffers.vectorMembers)[{{ loop.index0 }}].second); +{% endfor %} + + } + delete buffers.references; + delete buffers.vectorMembers; + }; + return readBuffers; } diff --git a/src/UserDataCollection.cc b/src/UserDataCollection.cc index c965f5b9c..673e7969c 100644 --- a/src/UserDataCollection.cc +++ b/src/UserDataCollection.cc @@ -20,18 +20,19 @@ namespace { // Register with schema version 1 to allow for potential changes CollectionBufferFactory::mutInstance().registerCreationFunc( userDataCollTypeName(), UserDataCollection::schemaVersion, [](bool) { - return podio::CollectionReadBuffers{new std::vector(), - nullptr, - nullptr, - podio::UserDataCollection::schemaVersion, - podio::userDataCollTypeName(), - [](podio::CollectionReadBuffers buffers, bool) { - return std::make_unique>( - std::move(*buffers.dataAsVector())); - }, - [](podio::CollectionReadBuffers& buffers) { - buffers.data = podio::CollectionWriteBuffers::asVector(buffers.data); - }}; + return podio::CollectionReadBuffers{ + new std::vector(), + nullptr, + nullptr, + podio::UserDataCollection::schemaVersion, + podio::userDataCollTypeName(), + [](podio::CollectionReadBuffers buffers, bool) { + return std::make_unique>(std::move(*buffers.dataAsVector())); + }, + [](podio::CollectionReadBuffers& buffers) { + buffers.data = podio::CollectionWriteBuffers::asVector(buffers.data); + }, + [](podio::CollectionReadBuffers& buffers) { delete static_cast*>(buffers.data); }}; }); // For now passing the same schema version for from and current versions diff --git a/tests/unittests/CMakeLists.txt b/tests/unittests/CMakeLists.txt index 9cbb99224..4988ab36d 100644 --- a/tests/unittests/CMakeLists.txt +++ b/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) diff --git a/tests/unittests/buffer_factory.cpp b/tests/unittests/buffer_factory.cpp new file mode 100644 index 000000000..382451644 --- /dev/null +++ b/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(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(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(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*>((*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(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(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*>((*buffers.vectorMembers)[0].second); + vecBuffer->emplace_back(42); + + auto collData = ExampleWithVectorMemberCollectionData(std::move(buffers), false); + } +}