Refs #15841: Fix failure handle in Monitor creation

Signed-off-by: jparisu <[email protected]>

Author: jparisu

src/cpp/Monitor.hpp

@@ -22,24 +22,23 @@
 #include <map>
 #include <string>
 
+#include <fastdds/dds/domain/DomainParticipant.hpp>
+#include <fastdds/dds/domain/DomainParticipantFactory.hpp>
+#include <fastdds/dds/domain/DomainParticipantListener.hpp>
+#include <fastdds/dds/domain/qos/DomainParticipantQos.hpp>
+#include <fastdds/dds/subscriber/DataReader.hpp>
+#include <fastdds/dds/subscriber/DataReaderListener.hpp>
+#include <fastdds/dds/subscriber/qos/DataReaderQos.hpp>
+#include <fastdds/dds/subscriber/qos/SubscriberQos.hpp>
+#include <fastdds/dds/subscriber/Subscriber.hpp>
+#include <fastdds/dds/topic/qos/TopicQos.hpp>
+#include <fastdds/dds/topic/Topic.hpp>
+
 #include <fastdds_statistics_backend/listener/DomainListener.hpp>
 #include <fastdds_statistics_backend/listener/CallbackMask.hpp>
 #include <fastdds_statistics_backend/types/EntityId.hpp>
 
 namespace eprosima {
-namespace fastdds {
-namespace dds {
-
-class DomainParticipant;
-class DomainParticipantListener;
-class Subscriber;
-class Topic;
-class DataReader;
-class DataReaderListener;
-
-} // namespace dds
-} // namespace fastdds
-
 namespace statistics_backend {
 namespace details {
 
@@ -50,51 +49,95 @@ namespace details {
  */
 struct Monitor
 {
+    /**
+     * @brief Destroy the Monitor object
+     *
+     * Destroy every pointer that has been set.
+     * This method works even if the monitor creation has failed
+     *
+     * @warning this may not be the best way to implement the destruction of subentities, as they are not created
+     * under this class. But it is very convenience so it is reused during Monitor creation in case an error occurs
+     * and also it is used to normally destroy the Monitor.
+     */
+    ~Monitor()
+    {
+        // These values are not always set, as could come from an error creating Monitor, or for test sake.
+        if (participant)
+        {
+            if (subscriber)
+            {
+                for (auto& reader : readers)
+                {
+                    subscriber->delete_datareader(reader.second);
+                }
+
+                participant->delete_subscriber(subscriber);
+            }
+
+            for (auto& topic : topics)
+            {
+                participant->delete_topic(topic.second);
+            }
+
+            fastdds::dds::DomainParticipantFactory::get_instance()->delete_participant(participant);
+        }
+
+        if (reader_listener)
+        {
+            delete reader_listener;
+        }
+
+        if (participant_listener)
+        {
+            delete participant_listener;
+        }
+    }
+
     //! The EntityId of the monitored domain
-    EntityId id;
+    EntityId id{};
 
     //! The user listener for this monitor
-    DomainListener* domain_listener;
+    DomainListener* domain_listener = nullptr;
 
     //! The callback mask applied to the \c domain_listener
-    CallbackMask domain_callback_mask;
+    CallbackMask domain_callback_mask{};
 
     //! The data mask applied to the \c domain_listener->on_data_available
-    DataKindMask data_mask;
+    DataKindMask data_mask{};
 
     //! The participant created to communicate with the statistics reporting endpoints in this monitor
-    fastdds::dds::DomainParticipant* participant;
+    fastdds::dds::DomainParticipant* participant = nullptr;
 
     //! The listener linked to the \c participant
     //! It will process the entity discoveries
-    fastdds::dds::DomainParticipantListener* participant_listener;
+    fastdds::dds::DomainParticipantListener* participant_listener = nullptr;
 
 
     //! The participant created to communicate with the statistics reporting publishers in this monitor
-    fastdds::dds::Subscriber* subscriber;
+    fastdds::dds::Subscriber* subscriber = nullptr;
 
     //! Holds the topic object created for each of the statistics topics
-    std::map<std::string, fastdds::dds::Topic*> topics;
+    std::map<std::string, fastdds::dds::Topic*> topics{};
 
     //! Holds the datareader object created for each of the statistics topics
-    std::map<std::string, fastdds::dds::DataReader*> readers;
+    std::map<std::string, fastdds::dds::DataReader*> readers{};
 
     //! The listener linked to the \c readers
     //! All readers will use the same listener
     //! The listener will decide how to process the data according to the topic of the reader
-    fastdds::dds::DataReaderListener* reader_listener;
+    fastdds::dds::DataReaderListener* reader_listener = nullptr;
 
     //! Participant discovery status. Used in the participant discovery user callback
-    DomainListener::Status participant_status_;
+    DomainListener::Status participant_status_{};
 
     //! Topic discovery status. Used in the topic discovery user callback
-    DomainListener::Status topic_status_;
+    DomainListener::Status topic_status_{};
 
     //! Datareader discovery status. Used in the datareader discovery user callback
-    DomainListener::Status datareader_status_;
+    DomainListener::Status datareader_status_{};
 
     //! DataWriter discovery status. Used in the datawriter discovery user callback
-    DomainListener::Status datawriter_status_;
+    DomainListener::Status datawriter_status_{};
 };
 
 } // namespace details

src/cpp/StatisticsBackend.cpp

@@ -184,24 +184,14 @@ EntityId create_and_register_monitor(
 {
     details::StatisticsBackendData::get_instance()->lock();
 
-    /* Create monitor instance and register it in the database */
+    // Create monitor instance.
+    // NOTE: register in database at the end, in case any creation fails
     std::shared_ptr<details::Monitor> monitor = std::make_shared<details::Monitor>();
     std::shared_ptr<database::Domain> domain = std::make_shared<database::Domain>(domain_name);
-    try
-    {
-        domain->id = details::StatisticsBackendData::get_instance()->database_->insert(domain);
-    }
-    catch (const std::exception&)
-    {
-        details::StatisticsBackendData::get_instance()->unlock();
-        throw;
-    }
 
-    monitor->id = domain->id;
     monitor->domain_listener = domain_listener;
     monitor->domain_callback_mask = callback_mask;
     monitor->data_mask = data_mask;
-    details::StatisticsBackendData::get_instance()->monitors_by_entity_[domain->id] = monitor;
 
     monitor->participant_listener = new subscriber::StatisticsParticipantListener(
         domain->id,
@@ -263,6 +253,22 @@ EntityId create_and_register_monitor(
         }
     }
 
+    // Insert domain entity in database
+    try
+    {
+        domain->id = details::StatisticsBackendData::get_instance()->database_->insert(domain);
+    }
+    catch (const std::exception&)
+    {
+        details::StatisticsBackendData::get_instance()->unlock();
+        throw;
+    }
+
+    // Insert monitor as a new monitor entity.
+    // NOTE: Monitor Id is only set after insert domain in database
+    monitor->id = domain->id;
+    details::StatisticsBackendData::get_instance()->monitors_by_entity_[domain->id] = monitor;
+
     details::StatisticsBackendData::get_instance()->unlock();
     return domain->id;
 }

src/cpp/StatisticsBackendData.cpp

@@ -363,41 +363,7 @@ void StatisticsBackendData::stop_monitor(
     monitors_by_entity_.erase(it);
 
     // Delete everything created during monitor initialization
-    for (const auto& reader : monitor->readers)
-    {
-        monitor->subscriber->delete_datareader(reader.second);
-    }
-    monitor->readers.clear();
-
-    for (const auto& topic : monitor->topics)
-    {
-        monitor->participant->delete_topic(topic.second);
-    }
-    monitor->topics.clear();
-
-    // NOTE: for test sake, this is not always set
-    if (monitor->subscriber)
-    {
-        monitor->participant->delete_subscriber(monitor->subscriber);
-    }
-
-    // NOTE: for test sake, this is not always set
-    if (monitor->participant)
-    {
-        eprosima::fastdds::dds::DomainParticipantFactory::get_instance()->delete_participant(monitor->participant);
-    }
-
-    // NOTE: for test sake, this is not always set
-    if (monitor->reader_listener)
-    {
-        delete monitor->reader_listener;
-    }
-
-    // NOTE: for test sake, this is not always set
-    if (monitor->participant_listener)
-    {
-        delete monitor->participant_listener;
-    }
+    monitor.reset();
 
     // The monitor is inactive
     // NOTE: for test sake, this is not always set