diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 3ea04225a..d1632427f 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -250,14 +250,12 @@ jobs: - name: Install apt packages uses: ./src/Fast-DDS-statistics-backend/.github/actions/install-apt-packages - - name: Install GTest - uses: ./src/Fast-DDS-statistics-backend/.github/actions/install-gtest - - name: Install Python packages uses: ./src/Fast-DDS-statistics-backend/.github/actions/install-python-packages - name: Fetch eProsima dependencies - uses: ./src/Fast-DDS-statistics-backend/.github/actions/fetch-fastdds-repos + run: | + vcs import src < ./src/Fast-DDS-statistics-backend/.github/workflows/ci.repos - name: Update colcon mixin run: | diff --git a/src/cpp/StatisticsBackend.cpp b/src/cpp/StatisticsBackend.cpp index 2d24f61fa..406ed7a05 100644 --- a/src/cpp/StatisticsBackend.cpp +++ b/src/cpp/StatisticsBackend.cpp @@ -54,6 +54,7 @@ #include "StatisticsBackendData.hpp" #include "detail/data_getters.hpp" #include "detail/data_aggregation.hpp" +#include "detail/ScopeExit.hpp" using namespace eprosima::fastdds::dds; using namespace eprosima::fastdds::rtps; @@ -187,20 +188,15 @@ EntityId create_and_register_monitor( // What should happen is that all this logic is moved to StatisticsBackendData. You know, some day... details::StatisticsBackendData::get_instance()->lock(); + MAKE_UNNAMED_SCOPE_EXIT(details::StatisticsBackendData::get_instance()->unlock()); // Create monitor instance. std::shared_ptr monitor = std::make_shared(); std::shared_ptr domain = std::make_shared(domain_name); - try - { - domain->id = details::StatisticsBackendData::get_instance()->database_->insert(domain); - } - catch (const std::exception&) - { - details::StatisticsBackendData::get_instance()->unlock(); - throw; - } + // Throw exception in fail case + domain->id = details::StatisticsBackendData::get_instance()->database_->insert(domain); + // TODO: in case this function fails afterwards, the domain will be kept in the database without associated // Participant. There must exist a way in database to delete a domain, or to make a rollback. @@ -209,14 +205,19 @@ EntityId create_and_register_monitor( monitor->domain_callback_mask = callback_mask; monitor->data_mask = data_mask; details::StatisticsBackendData::get_instance()->monitors_by_entity_[domain->id] = monitor; + auto se_erase_monitor_database_ = + MAKE_SCOPE_EXIT(details::StatisticsBackendData::get_instance()->monitors_by_entity_.erase(domain->id)); monitor->participant_listener = new subscriber::StatisticsParticipantListener( domain->id, details::StatisticsBackendData::get_instance()->database_.get(), details::StatisticsBackendData::get_instance()->entity_queue_, details::StatisticsBackendData::get_instance()->data_queue_); + auto se_participant_listener_ = MAKE_SCOPE_EXIT(delete monitor->participant_listener); + monitor->reader_listener = new subscriber::StatisticsReaderListener( details::StatisticsBackendData::get_instance()->data_queue_); + auto se_reader_listener_ = MAKE_SCOPE_EXIT(delete monitor->reader_listener); /* Create DomainParticipant */ StatusMask participant_mask = StatusMask::all(); @@ -229,14 +230,9 @@ EntityId create_and_register_monitor( if (monitor->participant == nullptr) { - // Remove those elements that have been set - delete monitor->reader_listener; - delete monitor->participant_listener; - details::StatisticsBackendData::get_instance()->monitors_by_entity_.erase(domain->id); - - details::StatisticsBackendData::get_instance()->unlock(); throw Error("Error initializing monitor. Could not create participant"); } + auto se_participant_ = MAKE_SCOPE_EXIT(DomainParticipantFactory::get_instance()->delete_participant(monitor->participant)); /* Create Subscriber */ monitor->subscriber = monitor->participant->create_subscriber( @@ -246,15 +242,29 @@ EntityId create_and_register_monitor( if (monitor->subscriber == nullptr) { - // Remove those elements that have been set - DomainParticipantFactory::get_instance()->delete_participant(monitor->participant); - delete monitor->reader_listener; - delete monitor->participant_listener; - details::StatisticsBackendData::get_instance()->monitors_by_entity_.erase(domain->id); - - details::StatisticsBackendData::get_instance()->unlock(); throw Error("Error initializing monitor. Could not create subscriber"); } + auto se_subscriber_ = MAKE_SCOPE_EXIT(monitor->participant->delete_subscriber(monitor->subscriber)); + + auto se_topics_datareaders_ = + MAKE_SCOPE_EXIT( + { + for (auto& it : monitor->readers) + { + if (nullptr != it.second) + { + monitor->subscriber->delete_datareader(it.second); + } + } + for (auto& it : monitor->topics) + { + if (nullptr != it.second) + { + monitor->participant->delete_topic(it.second); + } + } + } + ); for (const auto& topic : topics) { @@ -265,28 +275,6 @@ EntityId create_and_register_monitor( } catch (const std::exception& e) { - // Remove those elements that have been set - for (auto& it : monitor->readers) - { - if (nullptr != it.second) - { - monitor->subscriber->delete_datareader(it.second); - } - } - for (auto& it : monitor->topics) - { - if (nullptr != it.second) - { - monitor->participant->delete_topic(it.second); - } - } - monitor->participant->delete_subscriber(monitor->subscriber); - DomainParticipantFactory::get_instance()->delete_participant(monitor->participant); - delete monitor->reader_listener; - delete monitor->participant_listener; - details::StatisticsBackendData::get_instance()->monitors_by_entity_.erase(domain->id); - - details::StatisticsBackendData::get_instance()->unlock(); throw Error("Error registering topic " + std::string(topic) + " : " + e.what()); } @@ -299,33 +287,17 @@ EntityId create_and_register_monitor( if (monitor->readers[topic] == nullptr) { - // Remove those elements that have been set - for (auto& it : monitor->readers) - { - if (nullptr != it.second) - { - monitor->subscriber->delete_datareader(it.second); - } - } - for (auto& it : monitor->topics) - { - if (nullptr != it.second) - { - monitor->participant->delete_topic(it.second); - } - } - monitor->participant->delete_subscriber(monitor->subscriber); - DomainParticipantFactory::get_instance()->delete_participant(monitor->participant); - delete monitor->reader_listener; - delete monitor->participant_listener; - details::StatisticsBackendData::get_instance()->monitors_by_entity_.erase(domain->id); - - details::StatisticsBackendData::get_instance()->unlock(); throw Error("Error initializing monitor. Could not create reader for topic " + std::string(topic)); } } - details::StatisticsBackendData::get_instance()->unlock(); + se_erase_monitor_database_.cancel(); + se_participant_listener_.cancel(); + se_reader_listener_.cancel(); + se_participant_.cancel(); + se_subscriber_.cancel(); + se_topics_datareaders_.cancel(); + return domain->id; } diff --git a/src/cpp/StatisticsBackendData.cpp b/src/cpp/StatisticsBackendData.cpp index 316a64ed1..3e245fe2a 100644 --- a/src/cpp/StatisticsBackendData.cpp +++ b/src/cpp/StatisticsBackendData.cpp @@ -16,6 +16,7 @@ * @file StatisticsBackendData.cpp */ +#include "StatisticsBackendData.hpp" #include #include @@ -31,8 +32,6 @@ #include #include -#include "StatisticsBackendData.hpp" - #include "Monitor.hpp" #include #include @@ -52,10 +51,7 @@ StatisticsBackendData::StatisticsBackendData() , lock_(mutex_, std::defer_lock) , participant_factory_instance_(eprosima::fastdds::dds::DomainParticipantFactory::get_shared_instance()) { - // Set in DomainParticipantFactory that entities are created disabled - eprosima::fastdds::dds::DomainParticipantFactoryQos qos; - participant_factory_instance_->get_qos(qos); - qos.entity_factory().autoenable_created_entities = false; + // Do nothing } StatisticsBackendData::~StatisticsBackendData() diff --git a/src/cpp/detail/ScopeExit.hpp b/src/cpp/detail/ScopeExit.hpp new file mode 100644 index 000000000..b16676ffe --- /dev/null +++ b/src/cpp/detail/ScopeExit.hpp @@ -0,0 +1,88 @@ +// Copyright 2015-2020 Open Source Robotics Foundation, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Copyright 2022 Proyectos y Sistemas de Mantenimiento SL (eProsima). +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef _EPROSIMA_FASTDDS_STATISTICS_BACKEND_DETAIL_SCOPE_EXIT_HPP_ +#define _EPROSIMA_FASTDDS_STATISTICS_BACKEND_DETAIL_SCOPE_EXIT_HPP_ + +#include + +namespace eprosima { +namespace statistics_backend { +namespace details { + +template +class ScopeExit final +{ +public: + explicit ScopeExit(CallableT && callable) + : callable_(std::forward(callable)) + { + } + + ScopeExit(const ScopeExit &) = delete; + ScopeExit(ScopeExit &&) = default; + + ScopeExit & operator=(const ScopeExit &) = delete; + ScopeExit & operator=(ScopeExit &&) = default; + + ~ScopeExit() + { + if (!cancelled_) { + callable_(); + } + } + + void cancel() + { + cancelled_ = true; + } + +private: + CallableT callable_; + bool cancelled_{false}; +}; + +template +ScopeExit +make_scope_exit(CallableT && callable) +{ + return ScopeExit(std::forward(callable)); +} + +} // namespace details +} // namespace statistics_backend +} // namespace eprosima + +#define MAKE_SCOPE_EXIT(code) \ + eprosima::statistics_backend::details::make_scope_exit([&]() {code;}) + +#define JOIN_IMPL(arg1, arg2) arg1 ## arg2 +#define MAKE_UNNAMED_SCOPE_EXIT(code) \ + auto JOIN_IMPL(scope_exit_, __LINE__) = eprosima::statistics_backend::details::make_scope_exit([&]() {code;}) + +#endif // _EPROSIMA_FASTDDS_STATISTICS_BACKEND_DETAIL_SCOPE_EXIT_HPP_ diff --git a/test/mock/dds/DomainParticipantFactory/fastdds/dds/domain/DomainParticipantFactory_mock.hpp b/test/mock/dds/DomainParticipantFactory/fastdds/dds/domain/DomainParticipantFactory_mock.hpp deleted file mode 100644 index 818944463..000000000 --- a/test/mock/dds/DomainParticipantFactory/fastdds/dds/domain/DomainParticipantFactory_mock.hpp +++ /dev/null @@ -1,93 +0,0 @@ -// Copyright 2021 Proyectos y Sistemas de Mantenimiento SL (eProsima). -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -/** - * @file DomainParticipantFactory.hpp - * - */ - -// #ifndef _FASTDDS_DOMAINPARTICIPANTFACTORY_HPP_ -// #define _FASTDDS_DOMAINPARTICIPANTFACTORY_HPP_ - -// #include -// #include -// #include - -// #include -// #include -// #include - -// namespace eprosima { -// namespace fastdds { -// namespace dds { - -// class DomainParticipantListener; -// class DomainParticipant; - -// class DomainParticipantFactory -// { - -// public: - -// static DomainParticipantFactory* get_instance() -// { -// return get_shared_instance().get(); -// } - -// static std::shared_ptr get_shared_instance() -// { -// static std::shared_ptr instance(new DomainParticipantFactory()); -// return instance; -// } - -// ~DomainParticipantFactory() -// { -// // Do nothing -// } - -// MOCK_METHOD0( -// load_profiles, -// fastrtps::types::ReturnCode_t -// () -// ); - -// MOCK_METHOD4( -// create_participant, -// DomainParticipant * -// ( -// DomainId_t domain_id, -// const DomainParticipantQos& qos, -// DomainParticipantListener * listener, -// const StatusMask& mask -// )); - -// MOCK_METHOD1( -// delete_participant, -// void -// ( -// DomainParticipant * participant -// )); - -// MOCK_CONST_METHOD0( -// get_default_participant_qos, -// DomainParticipantQos & ()); - -// }; - - -// } /* namespace dds */ -// } /* namespace fastdds */ -// } /* namespace eprosima */ - -// #endif /* _FASTDDS_DOMAINPARTICIPANTFACTORY_HPP_*/ diff --git a/test/unittest/Database/DatabaseStatusTests.cpp b/test/unittest/Database/DatabaseStatusTests.cpp index 87319ab77..73a3303b2 100644 --- a/test/unittest/Database/DatabaseStatusTests.cpp +++ b/test/unittest/Database/DatabaseStatusTests.cpp @@ -40,8 +40,8 @@ class database_status_tests : public ::testing::Test datareader = db.get_dds_endpoints().begin()->second.begin()->second; locator = db.locators().begin()->second; - // Simulate that the backend is monitorizing the domain - // NOTE: This is so F*** dangerous, please do not do it again (1) + // Simulate that the backend is motorizing the domain + // NOTE: This is dangerous, please do not do it again std::shared_ptr monitor = std::make_shared(); monitor->id = domain->id; details::StatisticsBackendData::get_instance()->monitors_by_entity_[domain->id] = monitor; @@ -81,13 +81,6 @@ class database_status_tests : public ::testing::Test details::StatisticsBackendData::DiscoveryStatus::DISCOVERY); } - - // void TearDown() - // { - // // NOTE: This is thanks to (1) brilliant idea - // details::StatisticsBackendData::get_instance()->monitors_by_entity_.erase(domain->id); - // } - std::shared_ptr host; std::shared_ptr user; std::shared_ptr process; diff --git a/test/unittest/StatisticsBackend/InitMonitorFactoryFailsTests.cpp b/test/unittest/StatisticsBackend/InitMonitorFactoryFailsTests.cpp index cc066dcf5..4c881a9c9 100644 --- a/test/unittest/StatisticsBackend/InitMonitorFactoryFailsTests.cpp +++ b/test/unittest/StatisticsBackend/InitMonitorFactoryFailsTests.cpp @@ -149,12 +149,8 @@ class init_monitor_factory_fails_tests : public ::testing::Test domain_participant_factory_ = DomainParticipantFactory::get_instance(); // Mock method create_participant to return this participant domain_participant_factory_->domain_participant = &domain_participant_; - - // ON_CALL(*domain_participant_factory_, get_default_participant_qos()) - // .WillByDefault(ReturnRef(domain_participant_qos_)); - - // ON_CALL(*domain_participant_factory_, create_participant(_, _, _, _)) - // .WillByDefault(Return(&domain_participant_)); + // NOTE: as DomainParticipantFactory is a "Fake" class and not a Mock, there is no need for + // ON_CALL or EXPECT_CALL for its methods. ON_CALL(domain_participant_, get_default_subscriber_qos()) .WillByDefault(ReturnRef(subscriber_qos_)); @@ -189,18 +185,11 @@ class init_monitor_factory_fails_tests : public ::testing::Test .WillByDefault(Return(&data_readers_[topic_type.first])); } - // We usually do not care about these calls - // This can be overriden on specific tests if necessary - // EXPECT_CALL(*domain_participant_factory_, get_default_participant_qos()).Times(AnyNumber()); - // EXPECT_CALL(*domain_participant_factory_, load_profiles()).Times(AnyNumber()); - // EXPECT_CALL(*domain_participant_factory_, delete_participant(_)).Times(AnyNumber()); - EXPECT_CALL(domain_participant_, get_default_subscriber_qos()).Times(AnyNumber()); EXPECT_CALL(domain_participant_, get_default_topic_qos()).Times(AnyNumber()); EXPECT_CALL(subscriber_, get_default_datareader_qos()).Times(AnyNumber()); // The default expectations - // EXPECT_CALL(*domain_participant_factory_, create_participant(_, _, _, _)).Times(AnyNumber()); EXPECT_CALL(domain_participant_, create_subscriber(_, _, _)).Times(AnyNumber()); EXPECT_CALL(domain_participant_, create_topic(_, _, _, _, _)).Times(AnyNumber()); EXPECT_CALL(domain_participant_, create_topic(_, _, _, _)).Times(AnyNumber());