From a51eeff01accabee29ecce30595b24303fe4b92a Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Wed, 30 Jun 2021 15:41:06 +0200 Subject: [PATCH 01/32] iox-#415 Draft two attempts to access service registry database efficently (WIP) Signed-off-by: Simon Hoinkis --- .../internal/roudi/service_registry.hpp | 32 +-- .../iceoryx_posh/runtime/posh_runtime.hpp | 4 + iceoryx_posh/source/roudi/port_manager.cpp | 64 +++--- .../source/roudi/service_registry.cpp | 216 ++++++++++++++++-- .../test_roudi_service_registry.cpp | 29 +-- 5 files changed, 271 insertions(+), 74 deletions(-) diff --git a/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp b/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp index f457629d3d..6908ddd528 100644 --- a/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp +++ b/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp @@ -23,6 +23,7 @@ #include #include +#include namespace iox { @@ -32,23 +33,28 @@ static const capro::IdString_t Wildcard{"*"}; class ServiceRegistry { public: - static constexpr uint32_t MAX_INSTANCES_PER_SERVICE = 100u; - using InstanceSet_t = cxx::vector; - struct instance_t - { - InstanceSet_t instanceSet; - }; - using serviceMap_t = std::map; - - void add(const capro::IdString_t& service, const capro::IdString_t& instance); - void remove(const capro::IdString_t& service, const capro::IdString_t& instance); - void find(InstanceSet_t& instances, - const capro::IdString_t& service, + static constexpr uint32_t MAX_SERVICE_DESCRIPTIONS = 100U; + using ServiceDescriptionVector_t = cxx::vector; + + /// @todo Switch to a dictionary approach + using serviceMap_t = std::multimap, capro::IdString_t>; + + void add(const capro::ServiceDescription& serviceDescription); + void remove(const capro::ServiceDescription& serviceDescription); + void find(ServiceDescriptionVector_t& searchResult, + const capro::IdString_t& service = Wildcard, const capro::IdString_t& instance = Wildcard) const; + /// @todo remove handing out a ref to map, provide a copy of the service registry object instead const serviceMap_t& getServiceMap() const; private: - mutable serviceMap_t m_serviceMap; + // Attempt A + mutable serviceMap_t m_serviceMapOld; + + // Attempt B + std::multimap m_serviceMap; + std::multimap m_instanceMap; + cxx::vector m_realServiceDescriptionVector; }; } // namespace roudi } // namespace iox diff --git a/iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp b/iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp index 698717261c..2a97d21edc 100644 --- a/iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp +++ b/iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp @@ -96,11 +96,15 @@ class PoshRuntime findService(const cxx::variant service, const cxx::variant instance) noexcept = 0; + /// @todo this shall offer all services with the provided services name + /// param should just be a string /// @brief offer the provided service, sends the offer from application to RouDi daemon /// @param[in] service valid ServiceDescription to offer /// @return bool, if service is offered returns true else false virtual bool offerService(const capro::ServiceDescription& serviceDescription) noexcept = 0; + /// @todo this shall stopOffer all services with the provided services name + /// param should just be a string /// @brief stop offering the provided service /// @param[in] service valid ServiceDescription that shall be no more offered /// @return bool, if service is not offered anymore returns true else false diff --git a/iceoryx_posh/source/roudi/port_manager.cpp b/iceoryx_posh/source/roudi/port_manager.cpp index eb8aecc541..5d3b85960d 100644 --- a/iceoryx_posh/source/roudi/port_manager.cpp +++ b/iceoryx_posh/source/roudi/port_manager.cpp @@ -155,11 +155,13 @@ void PortManager::doDiscoveryForPublisherPort(PublisherPortRouDiType& publisherP m_portIntrospection.reportMessage(caproMessage); if (capro::CaproMessageType::OFFER == caproMessage.m_type) { + /// @todo add here the full capro message this->addEntryToServiceRegistry(caproMessage.m_serviceDescription.getServiceIDString(), caproMessage.m_serviceDescription.getInstanceIDString()); } else if (capro::CaproMessageType::STOP_OFFER == caproMessage.m_type) { + /// @todo add here the full capro message this->removeEntryFromServiceRegistry(caproMessage.m_serviceDescription.getServiceIDString(), caproMessage.m_serviceDescription.getInstanceIDString()); } @@ -268,23 +270,25 @@ void PortManager::handleInterfaces() noexcept } } // also forward services from service registry - auto serviceMap = m_serviceRegistry.getServiceMap(); - - caproMessage.m_subType = capro::CaproMessageSubType::SERVICE; - - for (auto const& x : serviceMap) - { - for (auto& instance : x.second.instanceSet) - { - caproMessage.m_serviceDescription = capro::ServiceDescription(x.first, instance, roudi::Wildcard); - - for (auto& interfacePortData : interfacePortsForInitialForwarding) - { - auto interfacePort = popo::InterfacePort(interfacePortData); - interfacePort.dispatchCaProMessage(caproMessage); - } - } - } + /// @todo do we still need this? yes but return a copy here to be stored in shared memory + // auto serviceMap = m_serviceRegistry.getServiceMap(); + + // caproMessage.m_subType = capro::CaproMessageSubType::SERVICE; + + // for (auto const& x : serviceMap) + // { + // for (auto& instance : x.second.instanceSet) + // { + // caproMessage.m_serviceDescription = capro::ServiceDescription(x.first, instance, + // roudi::Wildcard); + + // for (auto& interfacePortData : interfacePortsForInitialForwarding) + // { + // auto interfacePort = popo::InterfacePort(interfacePortData); + // interfacePort.dispatchCaProMessage(caproMessage); + // } + // } + // } } } @@ -600,17 +604,17 @@ runtime::IpcMessage PortManager::findService(const capro::IdString_t& service, interfacePort.dispatchCaProMessage(caproMessage); } - // add all found instances to instanceString - runtime::IpcMessage instanceMessage; + // add all found events to eventString + runtime::IpcMessage eventMessage; - ServiceRegistry::InstanceSet_t instances; - m_serviceRegistry.find(instances, service, instance); - for (auto& instance : instances) + ServiceRegistry::ServiceDescriptionVector_t services; + m_serviceRegistry.find(services, service, instance); + for (auto& service : services) { - instanceMessage << instance; + eventMessage << service.getEventIDString(); } - return instanceMessage; + return eventMessage; } const std::atomic* PortManager::serviceRegistryChangeCounter() noexcept @@ -721,17 +725,17 @@ popo::ApplicationPortData* PortManager::acquireApplicationPortData(const Runtime } } -void PortManager::addEntryToServiceRegistry(const capro::IdString_t& service, - const capro::IdString_t& instance) noexcept +void PortManager::addEntryToServiceRegistry(const capro::IdString_t& service IOX_MAYBE_UNUSED, + const capro::IdString_t& instance IOX_MAYBE_UNUSED) noexcept { - m_serviceRegistry.add(service, instance); + // m_serviceRegistry.add(service, instance); m_portPool->serviceRegistryChangeCounter()->fetch_add(1, std::memory_order_relaxed); } -void PortManager::removeEntryFromServiceRegistry(const capro::IdString_t& service, - const capro::IdString_t& instance) noexcept +void PortManager::removeEntryFromServiceRegistry(const capro::IdString_t& service IOX_MAYBE_UNUSED, + const capro::IdString_t& instance IOX_MAYBE_UNUSED) noexcept { - m_serviceRegistry.remove(service, instance); + // m_serviceRegistry.remove(service, instance); m_portPool->serviceRegistryChangeCounter()->fetch_add(1, std::memory_order_relaxed); } diff --git a/iceoryx_posh/source/roudi/service_registry.cpp b/iceoryx_posh/source/roudi/service_registry.cpp index dc79b4c723..74dfd16383 100644 --- a/iceoryx_posh/source/roudi/service_registry.cpp +++ b/iceoryx_posh/source/roudi/service_registry.cpp @@ -17,45 +17,225 @@ #include "iceoryx_posh/internal/roudi/service_registry.hpp" +#include + +/// cxx::multimap,V> +/// cxx::vector + + +/// cxx::multimap +/// cxx::multimap + + +/// cxx::vector + + +//, Object +//, Object +//, Speed +//, Speed + + +// if (instance == capro::IdString_t(capro::AnyInstanceString)) +// { +// for (auto& instance : m_serviceMap[make_pair(service,instance)]) +// { +// instances.push_back(); +// } +// } + namespace iox { namespace roudi { -void ServiceRegistry::add(const capro::IdString_t& service, const capro::IdString_t& instance) +void ServiceRegistry::add(const capro::ServiceDescription& serviceDescription) { - cxx::set::add(m_serviceMap[service].instanceSet, instance); + m_serviceMapOld.insert( + {std::make_pair(serviceDescription.getServiceIDString(), serviceDescription.getInstanceIDString()), + serviceDescription.getEventIDString()}); + // cxx::set::add(m_serviceMap[service].instanceSet, instance); } -void ServiceRegistry::remove(const capro::IdString_t& service, const capro::IdString_t& instance) +void ServiceRegistry::remove(const capro::ServiceDescription& serviceDescription) { - cxx::set::remove(m_serviceMap[service].instanceSet, instance); + cxx::set::remove(m_serviceMapOld, + {std::make_pair(serviceDescription.getServiceIDString(), serviceDescription.getInstanceIDString()), + serviceDescription.getEventIDString()}); } -void ServiceRegistry::find(InstanceSet_t& instances, +void ServiceRegistry::find(ServiceDescriptionVector_t& searchResult, const capro::IdString_t& service, const capro::IdString_t& instance) const { - if (instance == Wildcard) + // Attempt A + if (instance != capro::IdString_t(Wildcard) + && service != capro::IdString_t(Wildcard)) { - for (auto& instance : m_serviceMap[service].instanceSet) + // O(log n) + auto range = m_serviceMapOld.equal_range(std::make_pair(service, instance)); + // O(#result) + for (auto entry = range.first; entry != range.second; ++entry) { - instances.push_back(instance); + searchResult.push_back(capro::ServiceDescription(service, instance, entry->second)); } } else { - auto& instanceSet = m_serviceMap[service].instanceSet; - auto iter = std::find(instanceSet.begin(), instanceSet.end(), instance); - if (iter != instanceSet.end()) + cxx::vector availableServices; + cxx::vector availableInstances; + + // Grep for all available service strings + if (service == capro::IdString_t(Wildcard)) + { + for (auto& entry : m_serviceMapOld) + { + availableServices.push_back(entry.first.first); + } + } + else + { + availableServices.push_back(service); + } + + // Grep for all available instance strings + if (instance == capro::IdString_t(Wildcard)) + { + for (auto& entry : m_serviceMapOld) + { + availableInstances.push_back(entry.first.second); + } + } + else { - instances.push_back(*iter); + availableInstances.push_back(instance); } + + // O(n) + for (auto& service : availableServices) + { + // O(n) + for (auto& instance : availableInstances) + { + // O(log n) + auto range = m_serviceMapOld.equal_range(std::make_pair(service, instance)); + for (auto entry = range.first; entry != range.second; ++entry) + { + searchResult.push_back(capro::ServiceDescription(service, instance, entry->second)); + } + } + } + + ////////////////////////// + + // Find (K1, K2) + // O(log n) + + // Find (K1, *) <- aktuelle Implementierung ist für diesen Fall optimiert + // m = available K2 + // O(n + n * m * log n) + + // Find (*, *) + // p = available K1 + // m = available K2 + // O(n + n + (p * m * log n)) + + // Questions: + // * Can we just stored IDs aka integers instead of strings? + // * How would this affect the choice for the underlying data structure? + // * Can we force users to always use IDs? + // * Can we remove the integer values? What are they used for? + // * Where is the ClassHash function? + // * What happens if we create a ServiceDescription(AnyServiceString, AnyInstanceString, "foo")? + // * Forbid this pattern? + + // Temporary data structure + // map k1; + // map k2; + // vector> serviceDescriptionVector; + + ////////////////////////// + + + // Attempt B + cxx::vector intersection; + + // Find (K1, K2) + if (instance != capro::IdString_t(Wildcard) + && service != capro::IdString_t(Wildcard)) + { + // O(log n) + cxx::vector possibleServices; + auto range = m_serviceMap.equal_range(service); + for (auto entry = range.first; entry != range.second; ++entry) + { + possibleServices.push_back(entry.second); + } + + // O(log n) + cxx::vector possibleInstances; + auto range = m_instanceMap.equal_range(instance); + for (auto entry = range.first; entry != range.second; ++entry) + { + possibleInstances.push_back(entry.second); + } + + // O(max(#possibleServices,#possiblesInstances)) = O(n) + std::set_intersection(possibleServices.begin(), + possibleServices.end(), + possibleInstances.begin(), + possibleInstances.end(), + std::back_inserter(intersection)); + + for (auto& value : intersection) + { + searchResult.push_back(capro::ServiceDescription(service, instance, m_serviceDescriptionVector[value])); + } + } + else + { + // Find (*, K2) + if (service == capro::IdString_t(capro::AnyServiceString) + && instance != capro::IdString_t(capro::AnyInstanceString)) + { + // O(log n) + auto range = m_instanceMap.equal_range(instance); + // O(#result) + for (auto entry = range.first; entry != range.second; ++entry) + { + searchResult.push_back( + capro::ServiceDescription(FOOBAR, instance, m_serviceDescriptionVector[entry->second])); + } + } + // Find (K1, *) + else if (instance == capro::IdString_t(capro::AnyInstanceString) + && service != capro::IdString_t(capro::AnyServiceString)) + { + // O(log n) + auto range = m_serviceMap.equal_range(service); + // O(#result) + for (auto entry = range.first; entry != range.second; ++entry) + { + possibleServices. + capro::ServiceDescription(FOOBAR, instance, m_serviceDescriptionVector[entry->second])); + } + } + // Find (*, *) + searchResult = m_serviceDescriptionVector; + } + + // Find (K1, K2) + // O(log n + log n + #maxPossibleServices + #intersection) + + // Find (K1, *) <- bisheriger Implementierung + // O(log n) + + // Find (*, *) + // O(n) } -} -const ServiceRegistry::serviceMap_t& ServiceRegistry::getServiceMap() const -{ - return m_serviceMap; -} + const ServiceRegistry::serviceMap_t& ServiceRegistry::getServiceMap() const + { + return m_serviceMap; + } +} // namespace roudi } // namespace roudi -} // namespace iox diff --git a/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp b/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp index 56c9ddf03d..59fd9ae629 100644 --- a/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp +++ b/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp @@ -43,7 +43,8 @@ class ServiceRegistry_test : public Test } } iox::roudi::ServiceRegistry registry; - iox::roudi::ServiceRegistry::InstanceSet_t searchResults; + + iox::roudi::ServiceRegistry::ServiceDescriptionVector_t searchResults; }; TEST_F(ServiceRegistry_test, SingleAdd) @@ -52,7 +53,7 @@ TEST_F(ServiceRegistry_test, SingleAdd) registry.find(searchResults, "a", Wildcard); EXPECT_THAT(searchResults.size(), Eq(1)); - EXPECT_THAT(searchResults[0], Eq(iox::cxx::string<100>("b"))); + EXPECT_THAT(searchResults[0], Eq(capro::IdString_t("b"))); } TEST_F(ServiceRegistry_test, SingleMultiAdd) @@ -70,11 +71,11 @@ TEST_F(ServiceRegistry_test, SingleMultiAdd) for (auto& e : searchResults) { - if (e == iox::cxx::string<100>("b")) + if (e == capro::IdString_t("b")) hasFoundB = true; - if (e == iox::cxx::string<100>("c")) + if (e == capro::IdString_t("c")) hasFoundC = true; - if (e == iox::cxx::string<100>("d")) + if (e == capro::IdString_t("d")) hasFoundD = true; } @@ -88,12 +89,12 @@ TEST_F(ServiceRegistry_test, SingleAddMultiService) registry.find(searchResults, "a", Wildcard); EXPECT_THAT(searchResults.size(), Eq(1)); - EXPECT_THAT(searchResults[0], Eq(iox::cxx::string<100>("b"))); + EXPECT_THAT(searchResults[0], Eq(capro::IdString_t("b"))); searchResults.clear(); registry.find(searchResults, "c", Wildcard); EXPECT_THAT(searchResults.size(), Eq(1)); - EXPECT_THAT(searchResults[0], Eq(iox::cxx::string<100>("d"))); + EXPECT_THAT(searchResults[0], Eq(capro::IdString_t("d"))); } TEST_F(ServiceRegistry_test, FindSpecificInstance) @@ -104,7 +105,7 @@ TEST_F(ServiceRegistry_test, FindSpecificInstance) registry.find(searchResults, "a", "c"); EXPECT_THAT(searchResults.size(), Eq(1)); - EXPECT_THAT(searchResults[0], Eq(iox::cxx::string<100>("c"))); + EXPECT_THAT(searchResults[0], Eq(capro::IdString_t("c"))); } TEST_F(ServiceRegistry_test, FindSpecificNonExistingInstance) @@ -173,20 +174,22 @@ TEST_F(ServiceRegistry_test, GetServiceMap) for (auto const& x : serviceMap) { - if (x.first == iox::cxx::string<100>("a")) + if (x.first == capro::IdString_t("a")) { ASSERT_THAT(x.second.instanceSet.size(), Eq(3)); mapA = true; - EXPECT_THAT(x.second.instanceSet[0], Eq(iox::cxx::string<100>("b"))); - EXPECT_THAT(x.second.instanceSet[1], Eq(iox::cxx::string<100>("c"))); - EXPECT_THAT(x.second.instanceSet[2], Eq(iox::cxx::string<100>("d"))); + EXPECT_THAT(x.second.instanceSet[0], Eq(capro::IdString_t("b"))); + EXPECT_THAT(x.second.instanceSet[1], Eq(capro::IdString_t("c"))); + EXPECT_THAT(x.second.instanceSet[2], Eq(capro::IdString_t("d"))); } - if (x.first == iox::cxx::string<100>("e")) + if (x.first == capro::IdString_t("e")) mapE = true; } EXPECT_THAT(mapA && mapE, Eq(true)); } +/// @todo implement missing tests + } // namespace From 88fea8f14a02b3652e177387b7c1ca1b6671a7b5 Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Wed, 30 Jun 2021 15:47:16 +0200 Subject: [PATCH 02/32] iox-#415 Implement first version of new ServiceRegistry Signed-off-by: Simon Hoinkis --- .../internal/roudi/service_registry.hpp | 29 +- .../source/roudi/service_registry.cpp | 278 +++++++----------- .../test_roudi_service_registry.cpp | 166 +++++++---- 3 files changed, 220 insertions(+), 253 deletions(-) diff --git a/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp b/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp index 6908ddd528..421acea3c7 100644 --- a/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp +++ b/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp @@ -17,6 +17,7 @@ #ifndef IOX_POSH_ROUDI_SERVICE_REGISTRY_HPP #define IOX_POSH_ROUDI_SERVICE_REGISTRY_HPP +#include "iceoryx_hoofs/cxx/expected.hpp" #include "iceoryx_hoofs/cxx/vector.hpp" #include "iceoryx_hoofs/internal/cxx/set.hpp" #include "iceoryx_posh/capro/service_description.hpp" @@ -33,28 +34,26 @@ static const capro::IdString_t Wildcard{"*"}; class ServiceRegistry { public: + enum class ServiceRegistryError + { + INVALID_STATE, + SERVICE_DESCRIPTION_ALREADY_ADDED, + SERVICE_REGISTRY_FULL, + }; static constexpr uint32_t MAX_SERVICE_DESCRIPTIONS = 100U; using ServiceDescriptionVector_t = cxx::vector; - /// @todo Switch to a dictionary approach - using serviceMap_t = std::multimap, capro::IdString_t>; - - void add(const capro::ServiceDescription& serviceDescription); - void remove(const capro::ServiceDescription& serviceDescription); + cxx::expected add(const capro::ServiceDescription& serviceDescription); + bool remove(const capro::ServiceDescription& serviceDescription); void find(ServiceDescriptionVector_t& searchResult, const capro::IdString_t& service = Wildcard, - const capro::IdString_t& instance = Wildcard) const; - /// @todo remove handing out a ref to map, provide a copy of the service registry object instead - const serviceMap_t& getServiceMap() const; + const capro::IdString_t& instinstanceance = Wildcard) const; + const ServiceDescriptionVector_t getAllServices() const; private: - // Attempt A - mutable serviceMap_t m_serviceMapOld; - - // Attempt B - std::multimap m_serviceMap; - std::multimap m_instanceMap; - cxx::vector m_realServiceDescriptionVector; + ::std::multimap m_serviceMap; + ::std::multimap m_instanceMap; + ServiceDescriptionVector_t m_serviceDescriptionVector; }; } // namespace roudi } // namespace iox diff --git a/iceoryx_posh/source/roudi/service_registry.cpp b/iceoryx_posh/source/roudi/service_registry.cpp index 74dfd16383..2e53f70a2c 100644 --- a/iceoryx_posh/source/roudi/service_registry.cpp +++ b/iceoryx_posh/source/roudi/service_registry.cpp @@ -19,223 +19,149 @@ #include -/// cxx::multimap,V> -/// cxx::vector - - -/// cxx::multimap -/// cxx::multimap - - -/// cxx::vector - - -//, Object -//, Object -//, Speed -//, Speed - - -// if (instance == capro::IdString_t(capro::AnyInstanceString)) -// { -// for (auto& instance : m_serviceMap[make_pair(service,instance)]) -// { -// instances.push_back(); -// } -// } - namespace iox { namespace roudi { -void ServiceRegistry::add(const capro::ServiceDescription& serviceDescription) -{ - m_serviceMapOld.insert( - {std::make_pair(serviceDescription.getServiceIDString(), serviceDescription.getInstanceIDString()), - serviceDescription.getEventIDString()}); - // cxx::set::add(m_serviceMap[service].instanceSet, instance); -} - -void ServiceRegistry::remove(const capro::ServiceDescription& serviceDescription) +cxx::expected +ServiceRegistry::add(const capro::ServiceDescription& serviceDescription) { - cxx::set::remove(m_serviceMapOld, - {std::make_pair(serviceDescription.getServiceIDString(), serviceDescription.getInstanceIDString()), - serviceDescription.getEventIDString()}); -} - -void ServiceRegistry::find(ServiceDescriptionVector_t& searchResult, - const capro::IdString_t& service, - const capro::IdString_t& instance) const -{ - // Attempt A - if (instance != capro::IdString_t(Wildcard) - && service != capro::IdString_t(Wildcard)) + // Forbid duplicate service descriptions entries + for (auto& element : m_serviceDescriptionVector) { - // O(log n) - auto range = m_serviceMapOld.equal_range(std::make_pair(service, instance)); - // O(#result) - for (auto entry = range.first; entry != range.second; ++entry) + if (element == serviceDescription) { - searchResult.push_back(capro::ServiceDescription(service, instance, entry->second)); + return cxx::error(ServiceRegistryError::SERVICE_DESCRIPTION_ALREADY_ADDED); } } - else + + if (!m_serviceDescriptionVector.push_back(serviceDescription)) { - cxx::vector availableServices; - cxx::vector availableInstances; + return cxx::error(ServiceRegistryError::SERVICE_REGISTRY_FULL); + } + m_serviceMap.insert({serviceDescription.getServiceIDString(), m_serviceDescriptionVector.size() - 1}); + m_instanceMap.insert({serviceDescription.getInstanceIDString(), m_serviceDescriptionVector.size() - 1}); + return cxx::success<>(); +} - // Grep for all available service strings - if (service == capro::IdString_t(Wildcard)) - { - for (auto& entry : m_serviceMapOld) - { - availableServices.push_back(entry.first.first); - } - } - else - { - availableServices.push_back(service); - } +bool ServiceRegistry::remove(const capro::ServiceDescription& serviceDescription) +{ + bool removedElement{false}; - // Grep for all available instance strings - if (instance == capro::IdString_t(Wildcard)) + uint64_t index = 0U; + for (auto iterator = m_serviceDescriptionVector.begin(); iterator != m_serviceDescriptionVector.end();) + { + if (m_serviceDescriptionVector[index] == serviceDescription) { - for (auto& entry : m_serviceMapOld) + for (auto it = m_serviceMap.begin(); it != m_serviceMap.end();) { - availableInstances.push_back(entry.first.second); + if (it->second == index) + { + m_serviceMap.erase(it); + } + else + { + // update index due to removed element + it->second--; + } + it++; } - } - else - { - availableInstances.push_back(instance); - } - // O(n) - for (auto& service : availableServices) - { - // O(n) - for (auto& instance : availableInstances) + for (auto it = m_instanceMap.begin(); it != m_instanceMap.end();) { - // O(log n) - auto range = m_serviceMapOld.equal_range(std::make_pair(service, instance)); - for (auto entry = range.first; entry != range.second; ++entry) + if (it->second == index) { - searchResult.push_back(capro::ServiceDescription(service, instance, entry->second)); + m_instanceMap.erase(it); } + else + { + // update index due to removed element + it->second--; + } + it++; } + m_serviceDescriptionVector.erase(iterator); + removedElement = true; + // There can be not more than one element + break; } + index++; + iterator++; + } - ////////////////////////// - - // Find (K1, K2) - // O(log n) - - // Find (K1, *) <- aktuelle Implementierung ist für diesen Fall optimiert - // m = available K2 - // O(n + n * m * log n) - - // Find (*, *) - // p = available K1 - // m = available K2 - // O(n + n + (p * m * log n)) + return removedElement; +} - // Questions: - // * Can we just stored IDs aka integers instead of strings? - // * How would this affect the choice for the underlying data structure? - // * Can we force users to always use IDs? - // * Can we remove the integer values? What are they used for? - // * Where is the ClassHash function? - // * What happens if we create a ServiceDescription(AnyServiceString, AnyInstanceString, "foo")? - // * Forbid this pattern? +void ServiceRegistry::find(ServiceDescriptionVector_t& searchResult, + const capro::IdString_t& service, + const capro::IdString_t& instance) const +{ + cxx::vector intersection; - // Temporary data structure - // map k1; - // map k2; - // vector> serviceDescriptionVector; + // Find (K1, K2) + // O(log n + log n + max(#PossibleServices + #possiblesInstances) + #intersection) + if (instance != Wildcard && service != Wildcard) + { + cxx::vector possibleServices; + cxx::vector possibleInstances; - ////////////////////////// + auto rangeServiceMap = m_serviceMap.equal_range(service); + for (auto entry = rangeServiceMap.first; entry != rangeServiceMap.second; ++entry) + { + possibleServices.push_back(entry->second); + } + auto rangeInstanceMap = m_instanceMap.equal_range(instance); + for (auto entry = rangeInstanceMap.first; entry != rangeInstanceMap.second; ++entry) + { + possibleInstances.push_back(entry->second); + } - // Attempt B - cxx::vector intersection; + ::std::set_intersection(possibleServices.begin(), + possibleServices.end(), + possibleInstances.begin(), + possibleInstances.end(), + ::std::back_inserter(intersection)); - // Find (K1, K2) - if (instance != capro::IdString_t(Wildcard) - && service != capro::IdString_t(Wildcard)) + for (auto& value : intersection) + { + searchResult.push_back(m_serviceDescriptionVector[value]); + } + } + else + { + // Find (*, K2) + // O(log n + #result) + if (service == Wildcard && instance != Wildcard) { - // O(log n) - cxx::vector possibleServices; - auto range = m_serviceMap.equal_range(service); - for (auto entry = range.first; entry != range.second; ++entry) - { - possibleServices.push_back(entry.second); - } - - // O(log n) - cxx::vector possibleInstances; auto range = m_instanceMap.equal_range(instance); for (auto entry = range.first; entry != range.second; ++entry) { - possibleInstances.push_back(entry.second); + searchResult.push_back(m_serviceDescriptionVector[entry->second]); } - - // O(max(#possibleServices,#possiblesInstances)) = O(n) - std::set_intersection(possibleServices.begin(), - possibleServices.end(), - possibleInstances.begin(), - possibleInstances.end(), - std::back_inserter(intersection)); - - for (auto& value : intersection) + } + // Find (K1, *) + // O(log n + #result) + else if (instance == Wildcard && service != Wildcard) + { + auto range = m_serviceMap.equal_range(service); + for (auto entry = range.first; entry != range.second; ++entry) { - searchResult.push_back(capro::ServiceDescription(service, instance, m_serviceDescriptionVector[value])); + searchResult.push_back(m_serviceDescriptionVector[entry->second]); } } else { - // Find (*, K2) - if (service == capro::IdString_t(capro::AnyServiceString) - && instance != capro::IdString_t(capro::AnyInstanceString)) - { - // O(log n) - auto range = m_instanceMap.equal_range(instance); - // O(#result) - for (auto entry = range.first; entry != range.second; ++entry) - { - searchResult.push_back( - capro::ServiceDescription(FOOBAR, instance, m_serviceDescriptionVector[entry->second])); - } - } - // Find (K1, *) - else if (instance == capro::IdString_t(capro::AnyInstanceString) - && service != capro::IdString_t(capro::AnyServiceString)) - { - // O(log n) - auto range = m_serviceMap.equal_range(service); - // O(#result) - for (auto entry = range.first; entry != range.second; ++entry) - { - possibleServices. - capro::ServiceDescription(FOOBAR, instance, m_serviceDescriptionVector[entry->second])); - } - } // Find (*, *) + // O(1) searchResult = m_serviceDescriptionVector; } - - // Find (K1, K2) - // O(log n + log n + #maxPossibleServices + #intersection) - - // Find (K1, *) <- bisheriger Implementierung - // O(log n) - - // Find (*, *) - // O(n) } +} - const ServiceRegistry::serviceMap_t& ServiceRegistry::getServiceMap() const - { - return m_serviceMap; - } -} // namespace roudi +const ServiceRegistry::ServiceDescriptionVector_t ServiceRegistry::getAllServices() const +{ + return m_serviceDescriptionVector; +} } // namespace roudi +} // namespace iox diff --git a/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp b/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp index 59fd9ae629..b27f26f25b 100644 --- a/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp +++ b/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp @@ -29,6 +29,7 @@ class ServiceRegistry_test : public Test { public: using ServiceDescription = iox::capro::ServiceDescription; + virtual void SetUp() { internal::CaptureStdout(); @@ -47,20 +48,27 @@ class ServiceRegistry_test : public Test iox::roudi::ServiceRegistry::ServiceDescriptionVector_t searchResults; }; -TEST_F(ServiceRegistry_test, SingleAdd) +/// @todo Add EXPECT_THAT(add(), Eq(ExpectedError::FOO)) + +TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFound) { - registry.add("a", "b"); + iox::capro::ServiceDescription service1("a", "b", "c"); + ASSERT_FALSE(registry.add(service1).has_error()); registry.find(searchResults, "a", Wildcard); EXPECT_THAT(searchResults.size(), Eq(1)); - EXPECT_THAT(searchResults[0], Eq(capro::IdString_t("b"))); + EXPECT_THAT(searchResults[0], Eq(service1)); } -TEST_F(ServiceRegistry_test, SingleMultiAdd) +TEST_F(ServiceRegistry_test, MultipleServiceDescriptionWithSameServiceNameCanAllBeFound) { - registry.add("a", "b"); - registry.add("a", "c"); - registry.add("a", "d"); + iox::capro::ServiceDescription service1("a", "b", "b"); + iox::capro::ServiceDescription service2("a", "c", "c"); + iox::capro::ServiceDescription service3("a", "d", "d"); + + ASSERT_FALSE(registry.add(service1).has_error()); + ASSERT_FALSE(registry.add(service2).has_error()); + ASSERT_FALSE(registry.add(service3).has_error()); registry.find(searchResults, "a", Wildcard); EXPECT_THAT(searchResults.size(), Eq(3)); @@ -71,123 +79,157 @@ TEST_F(ServiceRegistry_test, SingleMultiAdd) for (auto& e : searchResults) { - if (e == capro::IdString_t("b")) + if (e == service1) hasFoundB = true; - if (e == capro::IdString_t("c")) + if (e == service2) hasFoundC = true; - if (e == capro::IdString_t("d")) + if (e == service3) hasFoundD = true; } EXPECT_THAT(hasFoundB && hasFoundC && hasFoundD, Eq(true)); } -TEST_F(ServiceRegistry_test, SingleAddMultiService) +TEST_F(ServiceRegistry_test, MultipleServiceDescriptionWithDifferentServiceNameCanAllBeFound) { - registry.add("a", "b"); - registry.add("c", "d"); + iox::capro::ServiceDescription service1("a", "b", "b"); + iox::capro::ServiceDescription service2("c", "d", "d"); + + ASSERT_FALSE(registry.add(service1).has_error()); + ASSERT_FALSE(registry.add(service2).has_error()); registry.find(searchResults, "a", Wildcard); EXPECT_THAT(searchResults.size(), Eq(1)); - EXPECT_THAT(searchResults[0], Eq(capro::IdString_t("b"))); + EXPECT_THAT(searchResults[0], Eq(service1)); searchResults.clear(); registry.find(searchResults, "c", Wildcard); EXPECT_THAT(searchResults.size(), Eq(1)); - EXPECT_THAT(searchResults[0], Eq(capro::IdString_t("d"))); + EXPECT_THAT(searchResults[0], Eq(service2)); } -TEST_F(ServiceRegistry_test, FindSpecificInstance) +TEST_F(ServiceRegistry_test, MultipleServiceDescriptionWithSameServiceNameFindsSpecificInstance) { - registry.add("a", "b"); - registry.add("a", "c"); - registry.add("a", "d"); + iox::capro::ServiceDescription service1("a", "b", "b"); + iox::capro::ServiceDescription service2("a", "c", "c"); + iox::capro::ServiceDescription service3("a", "d", "d"); + + ASSERT_FALSE(registry.add(service1).has_error()); + ASSERT_FALSE(registry.add(service2).has_error()); + ASSERT_FALSE(registry.add(service3).has_error()); registry.find(searchResults, "a", "c"); EXPECT_THAT(searchResults.size(), Eq(1)); - EXPECT_THAT(searchResults[0], Eq(capro::IdString_t("c"))); + EXPECT_THAT(searchResults[0], Eq(service2)); } -TEST_F(ServiceRegistry_test, FindSpecificNonExistingInstance) +TEST_F(ServiceRegistry_test, FindSpecificNonExistingServiceDescription) { - registry.add("a", "b"); - registry.add("a", "c"); - registry.add("a", "d"); + iox::capro::ServiceDescription service1("a", "b", "b"); + iox::capro::ServiceDescription service2("a", "c", "c"); + iox::capro::ServiceDescription service3("a", "d", "d"); + + ASSERT_FALSE(registry.add(service1).has_error()); + ASSERT_FALSE(registry.add(service2).has_error()); + ASSERT_FALSE(registry.add(service3).has_error()); registry.find(searchResults, "a", "g"); EXPECT_THAT(searchResults.size(), Eq(0)); } -TEST_F(ServiceRegistry_test, RemoveSingle) +TEST_F(ServiceRegistry_test, AddingMultipleServiceDescriptionWithSameServicesAndRemovingSpecificDoesNotFindSpecific) { - registry.add("a", "b"); - registry.add("a", "c"); - registry.add("a", "d"); + iox::capro::ServiceDescription service1("a", "b", "b"); + iox::capro::ServiceDescription service2("a", "c", "c"); + iox::capro::ServiceDescription service3("a", "d", "d"); - registry.remove("a", "c"); + ASSERT_FALSE(registry.add(service1).has_error()); + ASSERT_FALSE(registry.add(service2).has_error()); + ASSERT_FALSE(registry.add(service3).has_error()); + + EXPECT_TRUE(registry.remove(service2)); registry.find(searchResults, "a", "c"); EXPECT_THAT(searchResults.size(), Eq(0)); } -TEST_F(ServiceRegistry_test, RemoveSingleFromMultipleServices) +TEST_F(ServiceRegistry_test, + AddingMultipleServiceDescriptionWithDifferentServicesAndRemovingSpecificDoesNotFindSpecific) { - registry.add("a", "b"); - registry.add("b", "c"); - registry.add("c", "d"); + iox::capro::ServiceDescription service1("a", "b", "b"); + iox::capro::ServiceDescription service2("b", "c", "c"); + iox::capro::ServiceDescription service3("c", "d", "d"); + + ASSERT_FALSE(registry.add(service1).has_error()); + ASSERT_FALSE(registry.add(service2).has_error()); + ASSERT_FALSE(registry.add(service3).has_error()); - registry.remove("b", "c"); + EXPECT_TRUE(registry.remove(service2)); registry.find(searchResults, "b", "c"); EXPECT_THAT(searchResults.size(), Eq(0)); } -TEST_F(ServiceRegistry_test, RemoveAll) +TEST_F(ServiceRegistry_test, AddingMultipleServiceDescriptionAndRemovingAllDoesNotFindAnything) { - registry.add("a", "b"); - registry.add("a", "c"); - registry.add("a", "d"); + iox::capro::ServiceDescription service1("a", "b", "b"); // 0 + iox::capro::ServiceDescription service2("a", "c", "c"); // 1 + iox::capro::ServiceDescription service3("a", "d", "d"); // 2 - registry.remove("a", "b"); - registry.remove("a", "c"); - registry.remove("a", "d"); + ASSERT_FALSE(registry.add(service1).has_error()); + ASSERT_FALSE(registry.add(service2).has_error()); + ASSERT_FALSE(registry.add(service3).has_error()); + + EXPECT_TRUE(registry.remove(service1)); + EXPECT_TRUE(registry.remove(service2)); + EXPECT_TRUE(registry.remove(service3)); registry.find(searchResults, "a", Wildcard); EXPECT_THAT(searchResults.size(), Eq(0)); } -TEST_F(ServiceRegistry_test, GetServiceMap) +TEST_F(ServiceRegistry_test, AddingVariousServiceDescriptionAndGetAllServicesDoesNotReturnDuplicate) { - iox::roudi::ServiceRegistry::serviceMap_t serviceMap; + iox::capro::ServiceDescription service1("a", "b", "b"); + iox::capro::ServiceDescription service2("a", "c", "c"); + iox::capro::ServiceDescription service3("a", "d", "d"); + iox::capro::ServiceDescription service4("e", "f", "f"); - registry.add("a", "b"); + ASSERT_FALSE(registry.add(service1).has_error()); // add same service a, instance c to check if in registry only one entry is created - registry.add("a", "c"); - registry.add("a", "c"); - registry.add("a", "d"); - registry.add("e", "f"); + ASSERT_FALSE(registry.add(service2).has_error()); + auto result = registry.add(service2); + ASSERT_TRUE(result.has_error()); + EXPECT_THAT(result.get_error(), + Eq(iox::roudi::ServiceRegistry::ServiceRegistryError::SERVICE_DESCRIPTION_ALREADY_ADDED)); + ASSERT_FALSE(registry.add(service3).has_error()); + ASSERT_FALSE(registry.add(service4).has_error()); - serviceMap = registry.getServiceMap(); + auto serviceDescriptionVector = registry.getAllServices(); - bool mapA = false; - bool mapE = false; + bool service1Found = false; + bool service2Found = false; + bool service4Found = false; - for (auto const& x : serviceMap) + for (auto const& element : serviceDescriptionVector) { - if (x.first == capro::IdString_t("a")) + if (element == service1) { - ASSERT_THAT(x.second.instanceSet.size(), Eq(3)); - mapA = true; - EXPECT_THAT(x.second.instanceSet[0], Eq(capro::IdString_t("b"))); - EXPECT_THAT(x.second.instanceSet[1], Eq(capro::IdString_t("c"))); - EXPECT_THAT(x.second.instanceSet[2], Eq(capro::IdString_t("d"))); + service1Found = true; } - if (x.first == capro::IdString_t("e")) - mapE = true; - } + if (element == service2) + { + service2Found = true; + } - EXPECT_THAT(mapA && mapE, Eq(true)); + if (element == service4) + { + service4Found = true; + } + } + EXPECT_THAT(serviceDescriptionVector.size(), Eq(4)); + EXPECT_THAT(service1Found && service2Found && service4Found, Eq(true)); } /// @todo implement missing tests From 02071d452e538a24bddfafc7291ea4639a291eb6 Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Fri, 18 Jun 2021 12:47:35 +0200 Subject: [PATCH 03/32] iox-#415 Remove obsolete comments Signed-off-by: Simon Hoinkis --- iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp b/iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp index 2a97d21edc..698717261c 100644 --- a/iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp +++ b/iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp @@ -96,15 +96,11 @@ class PoshRuntime findService(const cxx::variant service, const cxx::variant instance) noexcept = 0; - /// @todo this shall offer all services with the provided services name - /// param should just be a string /// @brief offer the provided service, sends the offer from application to RouDi daemon /// @param[in] service valid ServiceDescription to offer /// @return bool, if service is offered returns true else false virtual bool offerService(const capro::ServiceDescription& serviceDescription) noexcept = 0; - /// @todo this shall stopOffer all services with the provided services name - /// param should just be a string /// @brief stop offering the provided service /// @param[in] service valid ServiceDescription that shall be no more offered /// @return bool, if service is not offered anymore returns true else false From 973b1660cd77c73748fe45b9c1076b1eb61d9f85 Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Wed, 30 Jun 2021 16:23:36 +0200 Subject: [PATCH 04/32] iox-#415 Send real ServiceDescription over IPC channel and write down new test cases Signed-off-by: Simon Hoinkis --- .../iceoryx_posh/iceoryx_posh_types.hpp | 13 ++- .../internal/roudi/port_manager.hpp | 4 +- .../internal/runtime/posh_runtime_impl.hpp | 2 +- .../iceoryx_posh/runtime/posh_runtime.hpp | 2 +- iceoryx_posh/source/roudi/port_manager.cpp | 80 +++++++++---------- iceoryx_posh/source/roudi/process_manager.cpp | 7 +- .../source/roudi/service_registry.cpp | 6 +- .../source/runtime/ipc_interface_base.cpp | 4 +- .../source/runtime/posh_runtime_impl.cpp | 14 ++-- .../test_roudi_findservice.cpp | 66 ++++++++------- iceoryx_posh/test/mocks/posh_runtime_mock.hpp | 4 +- .../test_roudi_service_registry.cpp | 8 ++ 12 files changed, 112 insertions(+), 98 deletions(-) diff --git a/iceoryx_posh/include/iceoryx_posh/iceoryx_posh_types.hpp b/iceoryx_posh/include/iceoryx_posh/iceoryx_posh_types.hpp index 7b3a8e3458..a29204ae2b 100644 --- a/iceoryx_posh/include/iceoryx_posh/iceoryx_posh_types.hpp +++ b/iceoryx_posh/include/iceoryx_posh/iceoryx_posh_types.hpp @@ -42,6 +42,14 @@ class PublisherPortUser; class SubscriberPortRouDi; class SubscriberPortUser; } // namespace popo +namespace capro +{ +class ServiceDescription; +} +namespace posix +{ +class UnixDomainSocket; +} // namespace posix using PublisherPortRouDiType = iox::popo::PublisherPortRouDi; using PublisherPortUserType = iox::popo::PublisherPortUser; @@ -134,7 +142,8 @@ constexpr uint32_t MAX_PROCESS_NUMBER = 300U; /// Maximum number of instances of a given service, which can be found. /// This limitation is coming due to the fixed capacity of the cxx::vector (This doesn't limit the offered number of /// instances) -constexpr uint32_t MAX_NUMBER_OF_INSTANCES = 50U; +/// @todo #415 increase number to 50 once service registry is available via shared memory +constexpr uint32_t MAX_NUMBER_OF_INSTANCES = 10U; // Nodes constexpr uint32_t MAX_NODE_NUMBER = 1000U; @@ -232,7 +241,7 @@ using TimePointNs_t = std::chrono::time_point; namespace runtime { -using InstanceContainer = iox::cxx::vector; +using ServiceContainer = iox::cxx::vector; using namespace units::duration_literals; constexpr units::Duration PROCESS_WAITING_FOR_ROUDI_TIMEOUT = 60_s; constexpr units::Duration PROCESS_KEEP_ALIVE_INTERVAL = 3 * roudi::DISCOVERY_INTERVAL; // > DISCOVERY_INTERVAL diff --git a/iceoryx_posh/include/iceoryx_posh/internal/roudi/port_manager.hpp b/iceoryx_posh/include/iceoryx_posh/internal/roudi/port_manager.hpp index 145d74a920..7bf1035b5a 100644 --- a/iceoryx_posh/include/iceoryx_posh/internal/roudi/port_manager.hpp +++ b/iceoryx_posh/include/iceoryx_posh/internal/roudi/port_manager.hpp @@ -132,8 +132,8 @@ class PortManager void sendToAllMatchingInterfacePorts(const capro::CaproMessage& message) noexcept; - void addEntryToServiceRegistry(const capro::IdString_t& service, const capro::IdString_t& instance) noexcept; - void removeEntryFromServiceRegistry(const capro::IdString_t& service, const capro::IdString_t& instance) noexcept; + void addEntryToServiceRegistry(const capro::ServiceDescription& service) noexcept; + void removeEntryFromServiceRegistry(const capro::ServiceDescription& service) noexcept; template ::value>* = nullptr> cxx::optional doesViolateCommunicationPolicy(const capro::ServiceDescription& service) const diff --git a/iceoryx_posh/include/iceoryx_posh/internal/runtime/posh_runtime_impl.hpp b/iceoryx_posh/include/iceoryx_posh/internal/runtime/posh_runtime_impl.hpp index 8f8b55b793..854e97f033 100644 --- a/iceoryx_posh/include/iceoryx_posh/internal/runtime/posh_runtime_impl.hpp +++ b/iceoryx_posh/include/iceoryx_posh/internal/runtime/posh_runtime_impl.hpp @@ -44,7 +44,7 @@ class PoshRuntimeImpl : public PoshRuntime virtual ~PoshRuntimeImpl() noexcept; /// @copydoc PoshRuntime::findService - cxx::expected + cxx::expected findService(const cxx::variant service, const cxx::variant instance) noexcept override; diff --git a/iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp b/iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp index 698717261c..e1f6555abc 100644 --- a/iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp +++ b/iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp @@ -92,7 +92,7 @@ class PoshRuntime /// @return cxx::expected /// InstanceContainer: on success, container that is filled with all matching instances /// FindServiceError: if any, encountered during the operation - virtual cxx::expected + virtual cxx::expected findService(const cxx::variant service, const cxx::variant instance) noexcept = 0; diff --git a/iceoryx_posh/source/roudi/port_manager.cpp b/iceoryx_posh/source/roudi/port_manager.cpp index 5d3b85960d..95bf96ec0e 100644 --- a/iceoryx_posh/source/roudi/port_manager.cpp +++ b/iceoryx_posh/source/roudi/port_manager.cpp @@ -155,15 +155,11 @@ void PortManager::doDiscoveryForPublisherPort(PublisherPortRouDiType& publisherP m_portIntrospection.reportMessage(caproMessage); if (capro::CaproMessageType::OFFER == caproMessage.m_type) { - /// @todo add here the full capro message - this->addEntryToServiceRegistry(caproMessage.m_serviceDescription.getServiceIDString(), - caproMessage.m_serviceDescription.getInstanceIDString()); + this->addEntryToServiceRegistry(caproMessage.m_serviceDescription); } else if (capro::CaproMessageType::STOP_OFFER == caproMessage.m_type) { - /// @todo add here the full capro message - this->removeEntryFromServiceRegistry(caproMessage.m_serviceDescription.getServiceIDString(), - caproMessage.m_serviceDescription.getInstanceIDString()); + this->removeEntryFromServiceRegistry(caproMessage.m_serviceDescription); } else { @@ -270,25 +266,22 @@ void PortManager::handleInterfaces() noexcept } } // also forward services from service registry - /// @todo do we still need this? yes but return a copy here to be stored in shared memory - // auto serviceMap = m_serviceRegistry.getServiceMap(); - - // caproMessage.m_subType = capro::CaproMessageSubType::SERVICE; - - // for (auto const& x : serviceMap) - // { - // for (auto& instance : x.second.instanceSet) - // { - // caproMessage.m_serviceDescription = capro::ServiceDescription(x.first, instance, - // roudi::Wildcard); - - // for (auto& interfacePortData : interfacePortsForInitialForwarding) - // { - // auto interfacePort = popo::InterfacePort(interfacePortData); - // interfacePort.dispatchCaProMessage(caproMessage); - // } - // } - // } + /// @todo #415 do we still need this? yes but return a copy here to be stored in shared memory via new + /// StatusPort's + auto serviceVector = m_serviceRegistry.getAllServices(); + + caproMessage.m_subType = capro::CaproMessageSubType::SERVICE; + + for (auto const& element : serviceVector) + { + caproMessage.m_serviceDescription = element; + + for (auto& interfacePortData : interfacePortsForInitialForwarding) + { + auto interfacePort = popo::InterfacePort(interfacePortData); + interfacePort.dispatchCaProMessage(caproMessage); + } + } } } @@ -310,14 +303,15 @@ void PortManager::handleApplications() noexcept { case capro::CaproMessageType::OFFER: { - addEntryToServiceRegistry(serviceDescription.getServiceIDString(), - serviceDescription.getInstanceIDString()); + auto serviceDescription = caproMessage.m_serviceDescription; + addEntryToServiceRegistry(serviceDescription); break; } case capro::CaproMessageType::STOP_OFFER: { - removeEntryFromServiceRegistry(serviceDescription.getServiceIDString(), - serviceDescription.getInstanceIDString()); + auto serviceDescription = caproMessage.m_serviceDescription; + removeEntryFromServiceRegistry(serviceDescription); + break; } default: @@ -554,8 +548,7 @@ void PortManager::destroyPublisherPort(PublisherPortRouDiType::MemberType_t* con cxx::Ensures(caproMessage.m_type == capro::CaproMessageType::STOP_OFFER); m_portIntrospection.reportMessage(caproMessage); - this->removeEntryFromServiceRegistry(caproMessage.m_serviceDescription.getServiceIDString(), - caproMessage.m_serviceDescription.getInstanceIDString()); + this->removeEntryFromServiceRegistry(caproMessage.m_serviceDescription); this->sendToAllMatchingSubscriberPorts(caproMessage, publisherPortRoudi); this->sendToAllMatchingInterfacePorts(caproMessage); }); @@ -604,17 +597,16 @@ runtime::IpcMessage PortManager::findService(const capro::IdString_t& service, interfacePort.dispatchCaProMessage(caproMessage); } - // add all found events to eventString - runtime::IpcMessage eventMessage; + runtime::IpcMessage response; - ServiceRegistry::ServiceDescriptionVector_t services; - m_serviceRegistry.find(services, service, instance); - for (auto& service : services) + ServiceRegistry::ServiceDescriptionVector_t searchResult; + m_serviceRegistry.find(searchResult, service, instance); + for (auto& service : searchResult) { - eventMessage << service.getEventIDString(); + response << static_cast(service).toString(); } - return eventMessage; + return response; } const std::atomic* PortManager::serviceRegistryChangeCounter() noexcept @@ -725,17 +717,17 @@ popo::ApplicationPortData* PortManager::acquireApplicationPortData(const Runtime } } -void PortManager::addEntryToServiceRegistry(const capro::IdString_t& service IOX_MAYBE_UNUSED, - const capro::IdString_t& instance IOX_MAYBE_UNUSED) noexcept +void PortManager::addEntryToServiceRegistry(const capro::ServiceDescription& service) noexcept { - // m_serviceRegistry.add(service, instance); + m_serviceRegistry.add(service).or_else([](auto&) { + /// @todo #415 return something? + }); m_portPool->serviceRegistryChangeCounter()->fetch_add(1, std::memory_order_relaxed); } -void PortManager::removeEntryFromServiceRegistry(const capro::IdString_t& service IOX_MAYBE_UNUSED, - const capro::IdString_t& instance IOX_MAYBE_UNUSED) noexcept +void PortManager::removeEntryFromServiceRegistry(const capro::ServiceDescription& service) noexcept { - // m_serviceRegistry.remove(service, instance); + m_serviceRegistry.remove(service); m_portPool->serviceRegistryChangeCounter()->fetch_add(1, std::memory_order_relaxed); } diff --git a/iceoryx_posh/source/roudi/process_manager.cpp b/iceoryx_posh/source/roudi/process_manager.cpp index fef533562e..c61c1207bd 100644 --- a/iceoryx_posh/source/roudi/process_manager.cpp +++ b/iceoryx_posh/source/roudi/process_manager.cpp @@ -352,11 +352,10 @@ void ProcessManager::findServiceForProcess(const RuntimeName_t& name, searchForProcessAndThen( name, [&](Process& process) { - runtime::IpcMessage instanceString({m_portManager.findService(service, instance)}); - process.sendViaIpcChannel(instanceString); - LogDebug() << "Sent InstanceString to application " << name; + process.sendViaIpcChannel({m_portManager.findService(service, instance)}); + LogDebug() << "Sent all found services to application " << name; }, - [&]() { LogWarn() << "Unknown process " << name << " requested an InstanceString."; }); + [&]() { LogWarn() << "Unknown process " << name << " requested to find services."; }); } void ProcessManager::addInterfaceForProcess(const RuntimeName_t& name, diff --git a/iceoryx_posh/source/roudi/service_registry.cpp b/iceoryx_posh/source/roudi/service_registry.cpp index 2e53f70a2c..f48be774a5 100644 --- a/iceoryx_posh/source/roudi/service_registry.cpp +++ b/iceoryx_posh/source/roudi/service_registry.cpp @@ -57,7 +57,8 @@ bool ServiceRegistry::remove(const capro::ServiceDescription& serviceDescription { if (it->second == index) { - m_serviceMap.erase(it); + it = m_serviceMap.erase(it); + continue; } else { @@ -71,7 +72,8 @@ bool ServiceRegistry::remove(const capro::ServiceDescription& serviceDescription { if (it->second == index) { - m_instanceMap.erase(it); + it = m_instanceMap.erase(it); + continue; } else { diff --git a/iceoryx_posh/source/runtime/ipc_interface_base.cpp b/iceoryx_posh/source/runtime/ipc_interface_base.cpp index 9f7a326764..fd293d8eb0 100644 --- a/iceoryx_posh/source/runtime/ipc_interface_base.cpp +++ b/iceoryx_posh/source/runtime/ipc_interface_base.cpp @@ -120,7 +120,7 @@ bool IpcInterfaceBase::send(const IpcMessage& msg) const noexcept { const size_t messageSize = static_cast(msg.getMessage().size()) + platform::IoxIpcChannelType::NULL_TERMINATOR_SIZE; - LogError() << "msg size of " << messageSize << "bigger than configured max message size"; + LogError() << "msg size of " << messageSize << " bigger than configured max message size"; } }; return !m_ipcChannel.send(msg.getMessage()).or_else(logLengthError).has_error(); @@ -140,7 +140,7 @@ bool IpcInterfaceBase::timedSend(const IpcMessage& msg, units::Duration timeout) { const size_t messageSize = static_cast(msg.getMessage().size()) + platform::IoxIpcChannelType::NULL_TERMINATOR_SIZE; - LogError() << "msg size of " << messageSize << "bigger than configured max message size"; + LogError() << "msg size of " << messageSize << " bigger than configured max message size"; } }; return !m_ipcChannel.timedSend(msg.getMessage(), timeout).or_else(logLengthError).has_error(); diff --git a/iceoryx_posh/source/runtime/posh_runtime_impl.cpp b/iceoryx_posh/source/runtime/posh_runtime_impl.cpp index a8f446f546..b40582d6aa 100644 --- a/iceoryx_posh/source/runtime/posh_runtime_impl.cpp +++ b/iceoryx_posh/source/runtime/posh_runtime_impl.cpp @@ -372,7 +372,7 @@ NodeData* PoshRuntimeImpl::createNode(const NodeProperty& nodeProperty) noexcept return nullptr; } -cxx::expected +cxx::expected PoshRuntimeImpl::findService(const cxx::variant service, const cxx::variant instance) noexcept { @@ -410,16 +410,16 @@ PoshRuntimeImpl::findService(const cxx::variant servic return cxx::error(FindServiceError::UNABLE_TO_WRITE_TO_ROUDI_CHANNEL); } - InstanceContainer instanceContainer; + ServiceContainer serviceContainer; uint32_t numberOfElements = requestResponse.getNumberOfElements(); - uint32_t capacity = static_cast(instanceContainer.capacity()); + uint32_t capacity = static_cast(serviceContainer.capacity()); - // Limit the instances (max value is the capacity of instanceContainer) + // Limit the instances (max value is the capacity of serviceContainer) uint32_t numberOfInstances = ((numberOfElements > capacity) ? capacity : numberOfElements); for (uint32_t i = 0; i < numberOfInstances; ++i) { - capro::IdString_t instance(iox::cxx::TruncateToCapacity, requestResponse.getElementAtIndex(i).c_str()); - instanceContainer.push_back(instance); + capro::ServiceDescription service(cxx::Serialization(requestResponse.getElementAtIndex(i))); + serviceContainer.push_back(service); } if (numberOfElements > capacity) @@ -429,7 +429,7 @@ PoshRuntimeImpl::findService(const cxx::variant servic errorHandler(Error::kPOSH__SERVICE_DISCOVERY_INSTANCE_CONTAINER_OVERFLOW, nullptr, ErrorLevel::MODERATE); return cxx::error(FindServiceError::INSTANCE_CONTAINER_OVERFLOW); } - return {cxx::success(instanceContainer)}; + return {cxx::success(serviceContainer)}; } diff --git a/iceoryx_posh/test/integrationtests/test_roudi_findservice.cpp b/iceoryx_posh/test/integrationtests/test_roudi_findservice.cpp index 4cde978afb..b16384315a 100644 --- a/iceoryx_posh/test/integrationtests/test_roudi_findservice.cpp +++ b/iceoryx_posh/test/integrationtests/test_roudi_findservice.cpp @@ -22,7 +22,8 @@ namespace { using iox::capro::IdString_t; -using iox::runtime::InstanceContainer; +using iox::capro::ServiceDescription; +using iox::runtime::ServiceContainer; class RoudiFindService_test : public RouDi_GTest { @@ -47,7 +48,7 @@ TEST_F(RoudiFindService_test, OfferSingleMethodServiceSingleInstance) auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(IdString_t("instance1"))); + ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); ASSERT_EQ(true, isServiceOffered); } @@ -89,7 +90,7 @@ TEST_F(RoudiFindService_test, ReofferedServiceWithValidServiceDescriptionCanBeFo auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(IdString_t("instance1"))); + ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); } TEST_F(RoudiFindService_test, OfferExsistingServiceMultipleTimesIsRedundant) @@ -102,7 +103,7 @@ TEST_F(RoudiFindService_test, OfferExsistingServiceMultipleTimesIsRedundant) auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(IdString_t("instance1"))); + ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); } TEST_F(RoudiFindService_test, FindSameServiceMultipleTimesReturnsSingleInstance) @@ -112,11 +113,11 @@ TEST_F(RoudiFindService_test, FindSameServiceMultipleTimesReturnsSingleInstance) auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(IdString_t("instance1"))); + ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(IdString_t("instance1"))); + ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); } TEST_F(RoudiFindService_test, OfferMultiMethodServiceSingleInstance) @@ -128,15 +129,15 @@ TEST_F(RoudiFindService_test, OfferMultiMethodServiceSingleInstance) auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(IdString_t("instance1"))); + ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); instanceContainer = receiverRuntime->findService(IdString_t("service2"), IdString_t("instance1")); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(IdString_t("instance1"))); + ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service2", "instance1", "event1"})); instanceContainer = receiverRuntime->findService(IdString_t("service3"), IdString_t("instance1")); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(IdString_t("instance1"))); + ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service3", "instance1", "event1"})); } TEST_F(RoudiFindService_test, OfferMultiMethodServiceWithDistinctSingleInstance) @@ -147,14 +148,14 @@ TEST_F(RoudiFindService_test, OfferMultiMethodServiceWithDistinctSingleInstance) auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(IdString_t("instance1"))); + ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); instanceContainer = receiverRuntime->findService(IdString_t("service2"), IdString_t("instance1")); ASSERT_THAT(instanceContainer.value().size(), Eq(0u)); instanceContainer = receiverRuntime->findService(IdString_t("service2"), IdString_t("instance2")); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(IdString_t("instance2"))); + ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service2", "instance2", "event1"})); } TEST_F(RoudiFindService_test, SubscribeAnyInstance) @@ -163,10 +164,10 @@ TEST_F(RoudiFindService_test, SubscribeAnyInstance) EXPECT_TRUE(senderRuntime->offerService({"service1", "instance2", "event2"})); EXPECT_TRUE(senderRuntime->offerService({"service1", "instance3", "event3"})); this->InterOpWait(); - InstanceContainer instanceContainerExp; - instanceContainerExp.push_back("instance1"); - instanceContainerExp.push_back("instance2"); - instanceContainerExp.push_back("instance3"); + ServiceContainer instanceContainerExp; + instanceContainerExp.push_back({"service1", "instance1", "event1"}); + instanceContainerExp.push_back({"service1", "instance2", "event2"}); + instanceContainerExp.push_back({"service1", "instance3", "event3"}); auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), iox::runtime::Any_t()); @@ -184,15 +185,15 @@ TEST_F(RoudiFindService_test, OfferSingleMethodServiceMultiInstance) auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(IdString_t("instance1"))); + ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance2")); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(IdString_t("instance2"))); + ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance2", "event2"})); instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance3")); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(IdString_t("instance3"))); + ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance3", "event3"})); } TEST_F(RoudiFindService_test, OfferMultiMethodServiceMultiInstance) @@ -207,27 +208,27 @@ TEST_F(RoudiFindService_test, OfferMultiMethodServiceMultiInstance) auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(IdString_t("instance1"))); + ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance2")); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(IdString_t("instance2"))); + ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance2", "event2"})); instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance3")); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(IdString_t("instance3"))); + ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance3", "event3"})); instanceContainer = receiverRuntime->findService(IdString_t("service2"), IdString_t("instance1")); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(IdString_t("instance1"))); + ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service2", "instance1", "event1"})); instanceContainer = receiverRuntime->findService(IdString_t("service2"), IdString_t("instance2")); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(IdString_t("instance2"))); + ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service2", "instance2", "event2"})); instanceContainer = receiverRuntime->findService(IdString_t("service2"), IdString_t("instance3")); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(IdString_t("instance3"))); + ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service2", "instance3", "event3"})); } TEST_F(RoudiFindService_test, StopOfferWithInvalidServiceDescriptionFails) @@ -247,7 +248,7 @@ TEST_F(RoudiFindService_test, StopOfferSingleMethodServiceSingleInstance) ASSERT_THAT(instanceContainer.value().size(), Eq(0u)); } -TEST_F(RoudiFindService_test, StopOfferMultiMethodServiceSingleInstance) +TEST_F(RoudiFindService_test, StopOfferMultiMethodServiceSingleInstance) /// @todo #415 fix this { EXPECT_TRUE(senderRuntime->offerService({"service1", "instance1", "event1"})); EXPECT_TRUE(senderRuntime->offerService({"service2", "instance1", "event1"})); @@ -258,13 +259,16 @@ TEST_F(RoudiFindService_test, StopOfferMultiMethodServiceSingleInstance) this->InterOpWait(); auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); + ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(0u)); instanceContainer = receiverRuntime->findService(IdString_t("service2"), IdString_t("instance1")); + ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(IdString_t("instance1"))); + ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service2", "instance1", "event1"})); instanceContainer = receiverRuntime->findService(IdString_t("service3"), IdString_t("instance1")); + ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(0u)); } @@ -293,7 +297,7 @@ TEST_F(RoudiFindService_test, StopNonExistingService) auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); ASSERT_THAT(instanceContainer.value().size(), Eq(1)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(IdString_t("instance1"))); + ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); } TEST_F(RoudiFindService_test, FindNonExistingServices) @@ -341,14 +345,14 @@ TEST_F(RoudiFindService_test, InterfacePort) TEST_F(RoudiFindService_test, findServiceMaxInstances) { size_t noOfInstances = iox::MAX_NUMBER_OF_INSTANCES; - InstanceContainer instanceContainerExp; + ServiceContainer instanceContainerExp; for (size_t i = 0; i < noOfInstances; i++) { // Service & Instance string is kept short , to reduce the response size in find service request , // (message queue has a limit of 512) std::string instance = "i" + iox::cxx::convert::toString(i); EXPECT_TRUE(senderRuntime->offerService({"s", IdString_t(iox::cxx::TruncateToCapacity, instance), "foo"})); - instanceContainerExp.push_back(IdString_t(iox::cxx::TruncateToCapacity, instance)); + instanceContainerExp.push_back({"s", IdString_t(iox::cxx::TruncateToCapacity, instance), "foo"}); this->InterOpWait(); } @@ -362,12 +366,12 @@ TEST_F(RoudiFindService_test, findServiceMaxInstances) TEST_F(RoudiFindService_test, findServiceInstanceContainerOverflowError) { size_t noOfInstances = (iox::MAX_NUMBER_OF_INSTANCES + 1); - InstanceContainer instanceContainerExp; + ServiceContainer instanceContainerExp; for (size_t i = 0; i < noOfInstances; i++) { std::string instance = "i" + iox::cxx::convert::toString(i); EXPECT_TRUE(senderRuntime->offerService({"s", IdString_t(iox::cxx::TruncateToCapacity, instance), "foo"})); - instanceContainerExp.push_back(IdString_t(iox::cxx::TruncateToCapacity, instance)); + instanceContainerExp.push_back({"s", IdString_t(iox::cxx::TruncateToCapacity, instance), "foo"}); this->InterOpWait(); } diff --git a/iceoryx_posh/test/mocks/posh_runtime_mock.hpp b/iceoryx_posh/test/mocks/posh_runtime_mock.hpp index fef3e5bdcd..b26dc85fae 100644 --- a/iceoryx_posh/test/mocks/posh_runtime_mock.hpp +++ b/iceoryx_posh/test/mocks/posh_runtime_mock.hpp @@ -47,7 +47,7 @@ class PoshRuntimeMock : public iox::runtime::PoshRuntime /// @todo iox-#841 simplify this when we switch to gmock v1.10 MOCK_METHOD2(findServiceMock, - iox::cxx::expected( + iox::cxx::expected( const iox::cxx::variant, const iox::cxx::variant)); MOCK_METHOD1(offerServiceMock, bool(const iox::capro::ServiceDescription&)); @@ -89,7 +89,7 @@ class PoshRuntimeMock : public iox::runtime::PoshRuntime return runtime; } - iox::cxx::expected + iox::cxx::expected findService(const iox::cxx::variant service, const iox::cxx::variant instance) noexcept override { diff --git a/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp b/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp index b27f26f25b..7cad22af95 100644 --- a/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp +++ b/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp @@ -50,6 +50,14 @@ class ServiceRegistry_test : public Test /// @todo Add EXPECT_THAT(add(), Eq(ExpectedError::FOO)) +// TEST_F(ServiceRegistry_test, AddMaximumNumberOfServiceDescriptionsWorks) +// TEST_F(ServiceRegistry_test, AddMoreThanMaximumNumberOfServiceDescriptionsFails) +// TEST_F(ServiceRegistry_test, AddMoreThanMaximumNumberOfServiceDescriptionsFails) +// TEST_F(ServiceRegistry_test, AddServiceDescriptionsWhichWasAlreadyAddedDoesNotWork) +// TEST_F(ServiceRegistry_test, AddInvalidServiceDescriptionsFails) +// TEST_F(ServiceRegistry_test, RemovingServiceDescriptionsWhichWasntAddedFails) + + TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFound) { iox::capro::ServiceDescription service1("a", "b", "c"); From e20c118f4d682d9950136f27e7898169df611da5 Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Mon, 21 Jun 2021 10:01:03 +0200 Subject: [PATCH 05/32] iox-#415 Fix RoudiFindService_test.StopOfferMultiMethodServiceSingleInstance Signed-off-by: Simon Hoinkis --- iceoryx_posh/source/roudi/service_registry.cpp | 12 ++++++++++-- .../test/integrationtests/test_roudi_findservice.cpp | 2 +- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/iceoryx_posh/source/roudi/service_registry.cpp b/iceoryx_posh/source/roudi/service_registry.cpp index f48be774a5..cf0f8ab2d6 100644 --- a/iceoryx_posh/source/roudi/service_registry.cpp +++ b/iceoryx_posh/source/roudi/service_registry.cpp @@ -53,14 +53,18 @@ bool ServiceRegistry::remove(const capro::ServiceDescription& serviceDescription { if (m_serviceDescriptionVector[index] == serviceDescription) { + uint64_t removedValue{0U}; + bool removedEntry{false}; for (auto it = m_serviceMap.begin(); it != m_serviceMap.end();) { if (it->second == index) { + removedValue = it->second; it = m_serviceMap.erase(it); + removedEntry = true; continue; } - else + else if (removedEntry && it->second > removedValue) { // update index due to removed element it->second--; @@ -68,14 +72,18 @@ bool ServiceRegistry::remove(const capro::ServiceDescription& serviceDescription it++; } + removedValue = 0U; + removedEntry = false; for (auto it = m_instanceMap.begin(); it != m_instanceMap.end();) { if (it->second == index) { + removedValue = it->second; it = m_instanceMap.erase(it); + removedEntry = true; continue; } - else + else if (removedEntry && it->second > removedValue) { // update index due to removed element it->second--; diff --git a/iceoryx_posh/test/integrationtests/test_roudi_findservice.cpp b/iceoryx_posh/test/integrationtests/test_roudi_findservice.cpp index b16384315a..817c78af42 100644 --- a/iceoryx_posh/test/integrationtests/test_roudi_findservice.cpp +++ b/iceoryx_posh/test/integrationtests/test_roudi_findservice.cpp @@ -248,7 +248,7 @@ TEST_F(RoudiFindService_test, StopOfferSingleMethodServiceSingleInstance) ASSERT_THAT(instanceContainer.value().size(), Eq(0u)); } -TEST_F(RoudiFindService_test, StopOfferMultiMethodServiceSingleInstance) /// @todo #415 fix this +TEST_F(RoudiFindService_test, StopOfferMultiMethodServiceSingleInstance) { EXPECT_TRUE(senderRuntime->offerService({"service1", "instance1", "event1"})); EXPECT_TRUE(senderRuntime->offerService({"service2", "instance1", "event1"})); From 51052899089cbf94f360f6724878fc61088f4942 Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Wed, 30 Jun 2021 16:25:05 +0200 Subject: [PATCH 06/32] iox-#415 Fix RoudiFindService_test.InterfacePort Signed-off-by: Simon Hoinkis --- iceoryx_posh/test/integrationtests/test_roudi_findservice.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/iceoryx_posh/test/integrationtests/test_roudi_findservice.cpp b/iceoryx_posh/test/integrationtests/test_roudi_findservice.cpp index 817c78af42..6f105f8b26 100644 --- a/iceoryx_posh/test/integrationtests/test_roudi_findservice.cpp +++ b/iceoryx_posh/test/integrationtests/test_roudi_findservice.cpp @@ -332,7 +332,7 @@ TEST_F(RoudiFindService_test, InterfacePort) auto caproMessage = maybeCaProMessage.value(); if ((caproMessage.m_serviceDescription.getServiceIDString() == IdString_t("service1")) && (caproMessage.m_serviceDescription.getInstanceIDString() == IdString_t("instance1")) - && ((caproMessage.m_serviceDescription.getEventIDString() == IdString_t(iox::roudi::Wildcard)))) + && ((caproMessage.m_serviceDescription.getEventIDString() == iox::roudi::Wildcard))) { serviceFound = true; break; From ea0ed4264fe9c338d1a03ee9edc32e8671af2886 Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Wed, 30 Jun 2021 16:39:51 +0200 Subject: [PATCH 07/32] iox-#415 Add asserts to RoudiFindService_test Signed-off-by: Simon Hoinkis --- .../iceoryx_posh/iceoryx_posh_types.hpp | 8 ++-- .../internal/roudi/service_registry.hpp | 2 + .../source/runtime/posh_runtime_impl.cpp | 2 +- .../test_roudi_findservice.cpp | 44 ++++++++++++++----- .../test_roudi_service_registry.cpp | 6 +-- 5 files changed, 41 insertions(+), 21 deletions(-) diff --git a/iceoryx_posh/include/iceoryx_posh/iceoryx_posh_types.hpp b/iceoryx_posh/include/iceoryx_posh/iceoryx_posh_types.hpp index a29204ae2b..958e196ce2 100644 --- a/iceoryx_posh/include/iceoryx_posh/iceoryx_posh_types.hpp +++ b/iceoryx_posh/include/iceoryx_posh/iceoryx_posh_types.hpp @@ -139,11 +139,11 @@ constexpr uint32_t APP_MESSAGE_SIZE = 512U; // Processes constexpr uint32_t MAX_PROCESS_NUMBER = 300U; -/// Maximum number of instances of a given service, which can be found. +/// Maximum number of service, which can be found. /// This limitation is coming due to the fixed capacity of the cxx::vector (This doesn't limit the offered number of /// instances) -/// @todo #415 increase number to 50 once service registry is available via shared memory -constexpr uint32_t MAX_NUMBER_OF_INSTANCES = 10U; +/// @todo #415 increase number back to 50 once service registry is available via shared memory +constexpr uint32_t MAX_NUMBER_OF_SERVICES = 10U; // Nodes constexpr uint32_t MAX_NODE_NUMBER = 1000U; @@ -241,7 +241,7 @@ using TimePointNs_t = std::chrono::time_point; namespace runtime { -using ServiceContainer = iox::cxx::vector; +using ServiceContainer = iox::cxx::vector; using namespace units::duration_literals; constexpr units::Duration PROCESS_WAITING_FOR_ROUDI_TIMEOUT = 60_s; constexpr units::Duration PROCESS_KEEP_ALIVE_INTERVAL = 3 * roudi::DISCOVERY_INTERVAL; // > DISCOVERY_INTERVAL diff --git a/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp b/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp index 421acea3c7..3eaed34864 100644 --- a/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp +++ b/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp @@ -40,6 +40,7 @@ class ServiceRegistry SERVICE_DESCRIPTION_ALREADY_ADDED, SERVICE_REGISTRY_FULL, }; + /// @todo #415 should be connected with iox::MAX_NUMBER_OF_SERVICES static constexpr uint32_t MAX_SERVICE_DESCRIPTIONS = 100U; using ServiceDescriptionVector_t = cxx::vector; @@ -51,6 +52,7 @@ class ServiceRegistry const ServiceDescriptionVector_t getAllServices() const; private: + /// @todo #859 replace std::map with prefix tree ::std::multimap m_serviceMap; ::std::multimap m_instanceMap; ServiceDescriptionVector_t m_serviceDescriptionVector; diff --git a/iceoryx_posh/source/runtime/posh_runtime_impl.cpp b/iceoryx_posh/source/runtime/posh_runtime_impl.cpp index b40582d6aa..26cdecf71e 100644 --- a/iceoryx_posh/source/runtime/posh_runtime_impl.cpp +++ b/iceoryx_posh/source/runtime/posh_runtime_impl.cpp @@ -425,7 +425,7 @@ PoshRuntimeImpl::findService(const cxx::variant servic if (numberOfElements > capacity) { LogWarn() << numberOfElements << " instances found for service \"" << serviceString - << "\" which is more than supported number of instances(" << MAX_NUMBER_OF_INSTANCES << "\n"; + << "\" which is more than supported number of instances(" << MAX_NUMBER_OF_SERVICES << "\n"; errorHandler(Error::kPOSH__SERVICE_DISCOVERY_INSTANCE_CONTAINER_OVERFLOW, nullptr, ErrorLevel::MODERATE); return cxx::error(FindServiceError::INSTANCE_CONTAINER_OVERFLOW); } diff --git a/iceoryx_posh/test/integrationtests/test_roudi_findservice.cpp b/iceoryx_posh/test/integrationtests/test_roudi_findservice.cpp index 6f105f8b26..2665b00ec7 100644 --- a/iceoryx_posh/test/integrationtests/test_roudi_findservice.cpp +++ b/iceoryx_posh/test/integrationtests/test_roudi_findservice.cpp @@ -47,6 +47,7 @@ TEST_F(RoudiFindService_test, OfferSingleMethodServiceSingleInstance) auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); + ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); ASSERT_EQ(true, isServiceOffered); @@ -89,6 +90,7 @@ TEST_F(RoudiFindService_test, ReofferedServiceWithValidServiceDescriptionCanBeFo auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); + ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); } @@ -102,6 +104,7 @@ TEST_F(RoudiFindService_test, OfferExsistingServiceMultipleTimesIsRedundant) auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); + ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); } @@ -112,10 +115,12 @@ TEST_F(RoudiFindService_test, FindSameServiceMultipleTimesReturnsSingleInstance) this->InterOpWait(); auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); + ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); + ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); } @@ -128,14 +133,17 @@ TEST_F(RoudiFindService_test, OfferMultiMethodServiceSingleInstance) this->InterOpWait(); auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); + ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); instanceContainer = receiverRuntime->findService(IdString_t("service2"), IdString_t("instance1")); + ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service2", "instance1", "event1"})); instanceContainer = receiverRuntime->findService(IdString_t("service3"), IdString_t("instance1")); + ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service3", "instance1", "event1"})); } @@ -147,13 +155,16 @@ TEST_F(RoudiFindService_test, OfferMultiMethodServiceWithDistinctSingleInstance) this->InterOpWait(); auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); + ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); instanceContainer = receiverRuntime->findService(IdString_t("service2"), IdString_t("instance1")); + ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(0u)); instanceContainer = receiverRuntime->findService(IdString_t("service2"), IdString_t("instance2")); + ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service2", "instance2", "event1"})); } @@ -170,8 +181,7 @@ TEST_F(RoudiFindService_test, SubscribeAnyInstance) instanceContainerExp.push_back({"service1", "instance3", "event3"}); auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), iox::runtime::Any_t()); - - + ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(3u)); EXPECT_TRUE(instanceContainer.value() == instanceContainerExp); } @@ -184,14 +194,17 @@ TEST_F(RoudiFindService_test, OfferSingleMethodServiceMultiInstance) this->InterOpWait(); auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); + ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance2")); + ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance2", "event2"})); instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance3")); + ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance3", "event3"})); } @@ -207,26 +220,32 @@ TEST_F(RoudiFindService_test, OfferMultiMethodServiceMultiInstance) this->InterOpWait(); auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); + ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance2")); + ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance2", "event2"})); instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance3")); + ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance3", "event3"})); instanceContainer = receiverRuntime->findService(IdString_t("service2"), IdString_t("instance1")); + ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service2", "instance1", "event1"})); instanceContainer = receiverRuntime->findService(IdString_t("service2"), IdString_t("instance2")); + ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service2", "instance2", "event2"})); instanceContainer = receiverRuntime->findService(IdString_t("service2"), IdString_t("instance3")); + ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service2", "instance3", "event3"})); } @@ -245,6 +264,7 @@ TEST_F(RoudiFindService_test, StopOfferSingleMethodServiceSingleInstance) this->InterOpWait(); auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); + ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(0u)); } @@ -282,7 +302,7 @@ TEST_F(RoudiFindService_test, StopOfferServiceRedundantCall) this->InterOpWait(); auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); - + ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(0u)); } @@ -295,7 +315,7 @@ TEST_F(RoudiFindService_test, StopNonExistingService) this->InterOpWait(); auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); - + ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(1)); ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); } @@ -308,12 +328,15 @@ TEST_F(RoudiFindService_test, FindNonExistingServices) this->InterOpWait(); auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("schlomo")); + ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(0u)); instanceContainer = receiverRuntime->findService(IdString_t("ignatz"), IdString_t("instance1")); + ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(0u)); instanceContainer = receiverRuntime->findService(IdString_t("ignatz"), IdString_t("schlomo")); + ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(0u)); } @@ -342,11 +365,10 @@ TEST_F(RoudiFindService_test, InterfacePort) EXPECT_THAT(serviceFound, Eq(true)); } -TEST_F(RoudiFindService_test, findServiceMaxInstances) +TEST_F(RoudiFindService_test, findServiceMaxServices) { - size_t noOfInstances = iox::MAX_NUMBER_OF_INSTANCES; ServiceContainer instanceContainerExp; - for (size_t i = 0; i < noOfInstances; i++) + for (size_t i = 0; i < iox::MAX_NUMBER_OF_SERVICES; i++) { // Service & Instance string is kept short , to reduce the response size in find service request , // (message queue has a limit of 512) @@ -358,14 +380,14 @@ TEST_F(RoudiFindService_test, findServiceMaxInstances) auto instanceContainer = receiverRuntime->findService(IdString_t("s"), iox::runtime::Any_t()); - EXPECT_THAT(instanceContainer.value().size(), Eq(iox::MAX_NUMBER_OF_INSTANCES)); + ASSERT_FALSE(instanceContainer.has_error()); + EXPECT_THAT(instanceContainer.value().size(), Eq(iox::MAX_NUMBER_OF_SERVICES)); EXPECT_TRUE(instanceContainer.value() == instanceContainerExp); - ASSERT_THAT(instanceContainer.has_error(), Eq(false)); -} // namespace +} TEST_F(RoudiFindService_test, findServiceInstanceContainerOverflowError) { - size_t noOfInstances = (iox::MAX_NUMBER_OF_INSTANCES + 1); + size_t noOfInstances = (iox::MAX_NUMBER_OF_SERVICES + 1); ServiceContainer instanceContainerExp; for (size_t i = 0; i < noOfInstances; i++) { diff --git a/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp b/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp index 7cad22af95..cada79fe23 100644 --- a/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp +++ b/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp @@ -48,8 +48,7 @@ class ServiceRegistry_test : public Test iox::roudi::ServiceRegistry::ServiceDescriptionVector_t searchResults; }; -/// @todo Add EXPECT_THAT(add(), Eq(ExpectedError::FOO)) - +/// @todo #415 implement missing tests // TEST_F(ServiceRegistry_test, AddMaximumNumberOfServiceDescriptionsWorks) // TEST_F(ServiceRegistry_test, AddMoreThanMaximumNumberOfServiceDescriptionsFails) // TEST_F(ServiceRegistry_test, AddMoreThanMaximumNumberOfServiceDescriptionsFails) @@ -57,7 +56,6 @@ class ServiceRegistry_test : public Test // TEST_F(ServiceRegistry_test, AddInvalidServiceDescriptionsFails) // TEST_F(ServiceRegistry_test, RemovingServiceDescriptionsWhichWasntAddedFails) - TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFound) { iox::capro::ServiceDescription service1("a", "b", "c"); @@ -240,6 +238,4 @@ TEST_F(ServiceRegistry_test, AddingVariousServiceDescriptionAndGetAllServicesDoe EXPECT_THAT(service1Found && service2Found && service4Found, Eq(true)); } -/// @todo implement missing tests - } // namespace From fb209560d6eae0dd5559a077afe21fcf9c8ffa05 Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Wed, 30 Jun 2021 16:42:46 +0200 Subject: [PATCH 08/32] iox-#415 Add copyright header and doxygen Signed-off-by: Simon Hoinkis --- .../internal/roudi/service_registry.hpp | 15 +++++++++++++++ .../include/iceoryx_posh/runtime/posh_runtime.hpp | 4 ++-- .../moduletests/test_roudi_service_registry.cpp | 8 ++++---- 3 files changed, 21 insertions(+), 6 deletions(-) diff --git a/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp b/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp index 3eaed34864..1e0e6d1788 100644 --- a/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp +++ b/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp @@ -44,11 +44,26 @@ class ServiceRegistry static constexpr uint32_t MAX_SERVICE_DESCRIPTIONS = 100U; using ServiceDescriptionVector_t = cxx::vector; + /// @brief Adds given service description to registry + /// @param[in] serviceDescription, service to be added + /// @return ServiceRegistryError, error wrapped in cxx::expected cxx::expected add(const capro::ServiceDescription& serviceDescription); + + /// @brief Removes given service description from registry + /// @param[in] serviceDescription, service to be removed + /// @return true, if service description was removed, false otherwise bool remove(const capro::ServiceDescription& serviceDescription); + + /// @brief Removes given service description from registry + /// @param[in] searchResult, reference to the vector which will be filled with the results + /// @param[in] service, string or wildcard to search for + /// @param[in] service, string or wildcard to search for void find(ServiceDescriptionVector_t& searchResult, const capro::IdString_t& service = Wildcard, const capro::IdString_t& instinstanceance = Wildcard) const; + + /// @brief Returns all service descriptions as copy + /// @return ServiceDescriptionVector_t, copy of complete service registry const ServiceDescriptionVector_t getAllServices() const; private: diff --git a/iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp b/iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp index e1f6555abc..5d9962476e 100644 --- a/iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp +++ b/iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp @@ -89,8 +89,8 @@ class PoshRuntime /// @brief find all services that match the provided service description /// @param[in] service service string to search for (wildcards allowed) /// @param[in] instance instance string to search for (wildcards allowed) - /// @return cxx::expected - /// InstanceContainer: on success, container that is filled with all matching instances + /// @return cxx::expected + /// ServiceContainer: on success, container that is filled with all matching instances /// FindServiceError: if any, encountered during the operation virtual cxx::expected findService(const cxx::variant service, diff --git a/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp b/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp index cada79fe23..27b2a6ce85 100644 --- a/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp +++ b/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp @@ -129,7 +129,7 @@ TEST_F(ServiceRegistry_test, MultipleServiceDescriptionWithSameServiceNameFindsS EXPECT_THAT(searchResults[0], Eq(service2)); } -TEST_F(ServiceRegistry_test, FindSpecificNonExistingServiceDescription) +TEST_F(ServiceRegistry_test, FindSpecificNonExistingServiceDescriptionFails) { iox::capro::ServiceDescription service1("a", "b", "b"); iox::capro::ServiceDescription service2("a", "c", "c"); @@ -178,9 +178,9 @@ TEST_F(ServiceRegistry_test, TEST_F(ServiceRegistry_test, AddingMultipleServiceDescriptionAndRemovingAllDoesNotFindAnything) { - iox::capro::ServiceDescription service1("a", "b", "b"); // 0 - iox::capro::ServiceDescription service2("a", "c", "c"); // 1 - iox::capro::ServiceDescription service3("a", "d", "d"); // 2 + iox::capro::ServiceDescription service1("a", "b", "b"); + iox::capro::ServiceDescription service2("a", "c", "c"); + iox::capro::ServiceDescription service3("a", "d", "d"); ASSERT_FALSE(registry.add(service1).has_error()); ASSERT_FALSE(registry.add(service2).has_error()); From b7c3f97c5761f3629cbcb8e964a5aef08c5b7416 Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Wed, 30 Jun 2021 16:46:38 +0200 Subject: [PATCH 09/32] iox-#415 Update some comments Signed-off-by: Simon Hoinkis --- iceoryx_posh/source/runtime/posh_runtime_impl.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/iceoryx_posh/source/runtime/posh_runtime_impl.cpp b/iceoryx_posh/source/runtime/posh_runtime_impl.cpp index 26cdecf71e..839675dd2c 100644 --- a/iceoryx_posh/source/runtime/posh_runtime_impl.cpp +++ b/iceoryx_posh/source/runtime/posh_runtime_impl.cpp @@ -414,9 +414,9 @@ PoshRuntimeImpl::findService(const cxx::variant servic uint32_t numberOfElements = requestResponse.getNumberOfElements(); uint32_t capacity = static_cast(serviceContainer.capacity()); - // Limit the instances (max value is the capacity of serviceContainer) - uint32_t numberOfInstances = ((numberOfElements > capacity) ? capacity : numberOfElements); - for (uint32_t i = 0; i < numberOfInstances; ++i) + // Limit the services (max value is the capacity of serviceContainer) + uint32_t numberOfServices = ((numberOfElements > capacity) ? capacity : numberOfElements); + for (uint32_t i = 0; i < numberOfServices; ++i) { capro::ServiceDescription service(cxx::Serialization(requestResponse.getElementAtIndex(i))); serviceContainer.push_back(service); @@ -425,7 +425,7 @@ PoshRuntimeImpl::findService(const cxx::variant servic if (numberOfElements > capacity) { LogWarn() << numberOfElements << " instances found for service \"" << serviceString - << "\" which is more than supported number of instances(" << MAX_NUMBER_OF_SERVICES << "\n"; + << "\" which is more than supported number of services(" << MAX_NUMBER_OF_SERVICES << "\n"; errorHandler(Error::kPOSH__SERVICE_DISCOVERY_INSTANCE_CONTAINER_OVERFLOW, nullptr, ErrorLevel::MODERATE); return cxx::error(FindServiceError::INSTANCE_CONTAINER_OVERFLOW); } From 5e0f5b004deb4bd406904e4bfb97864c4c3534c9 Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Wed, 30 Jun 2021 16:48:20 +0200 Subject: [PATCH 10/32] iox-#415 Write down some more ServiceRegistry test cases Signed-off-by: Simon Hoinkis --- iceoryx_posh/include/iceoryx_posh/iceoryx_posh_types.hpp | 2 +- .../test/moduletests/test_roudi_service_registry.cpp | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/iceoryx_posh/include/iceoryx_posh/iceoryx_posh_types.hpp b/iceoryx_posh/include/iceoryx_posh/iceoryx_posh_types.hpp index 958e196ce2..9989c68f8d 100644 --- a/iceoryx_posh/include/iceoryx_posh/iceoryx_posh_types.hpp +++ b/iceoryx_posh/include/iceoryx_posh/iceoryx_posh_types.hpp @@ -139,7 +139,7 @@ constexpr uint32_t APP_MESSAGE_SIZE = 512U; // Processes constexpr uint32_t MAX_PROCESS_NUMBER = 300U; -/// Maximum number of service, which can be found. +/// Maximum number of services, which can be found. /// This limitation is coming due to the fixed capacity of the cxx::vector (This doesn't limit the offered number of /// instances) /// @todo #415 increase number back to 50 once service registry is available via shared memory diff --git a/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp b/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp index 27b2a6ce85..045a836099 100644 --- a/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp +++ b/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp @@ -29,7 +29,6 @@ class ServiceRegistry_test : public Test { public: using ServiceDescription = iox::capro::ServiceDescription; - virtual void SetUp() { internal::CaptureStdout(); @@ -49,14 +48,17 @@ class ServiceRegistry_test : public Test }; /// @todo #415 implement missing tests +// TEST_F(ServiceRegistry_test, AddNoServiceDescriptionsAndWildcardSearchReturnsNothing) +// TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFoundWithWildcardSearch) +// TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFoundWithInstanceName) // TEST_F(ServiceRegistry_test, AddMaximumNumberOfServiceDescriptionsWorks) // TEST_F(ServiceRegistry_test, AddMoreThanMaximumNumberOfServiceDescriptionsFails) -// TEST_F(ServiceRegistry_test, AddMoreThanMaximumNumberOfServiceDescriptionsFails) // TEST_F(ServiceRegistry_test, AddServiceDescriptionsWhichWasAlreadyAddedDoesNotWork) // TEST_F(ServiceRegistry_test, AddInvalidServiceDescriptionsFails) // TEST_F(ServiceRegistry_test, RemovingServiceDescriptionsWhichWasntAddedFails) +// TEST_F(ServiceRegistry_test, RemovingInvalidServiceDescriptionsFails) -TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFound) +TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFoundWithServiceName) { iox::capro::ServiceDescription service1("a", "b", "c"); ASSERT_FALSE(registry.add(service1).has_error()); From 8e240290bf07417670eade750041dc9c963aed10 Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Wed, 30 Jun 2021 16:49:30 +0200 Subject: [PATCH 11/32] iox-#415 Add noexcept to ServiceRegistry and rename ServiceRegistry::Error Signed-off-by: Simon Hoinkis --- .../internal/roudi/service_registry.hpp | 10 +++++----- iceoryx_posh/source/roudi/port_manager.cpp | 12 ++++-------- iceoryx_posh/source/roudi/service_registry.cpp | 13 ++++++------- iceoryx_posh/source/runtime/posh_runtime_impl.cpp | 3 ++- .../moduletests/test_roudi_service_registry.cpp | 7 +++---- 5 files changed, 20 insertions(+), 25 deletions(-) diff --git a/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp b/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp index 1e0e6d1788..1c14b9ec61 100644 --- a/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp +++ b/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp @@ -34,7 +34,7 @@ static const capro::IdString_t Wildcard{"*"}; class ServiceRegistry { public: - enum class ServiceRegistryError + enum class Error { INVALID_STATE, SERVICE_DESCRIPTION_ALREADY_ADDED, @@ -47,12 +47,12 @@ class ServiceRegistry /// @brief Adds given service description to registry /// @param[in] serviceDescription, service to be added /// @return ServiceRegistryError, error wrapped in cxx::expected - cxx::expected add(const capro::ServiceDescription& serviceDescription); + cxx::expected add(const capro::ServiceDescription& serviceDescription) noexcept; /// @brief Removes given service description from registry /// @param[in] serviceDescription, service to be removed /// @return true, if service description was removed, false otherwise - bool remove(const capro::ServiceDescription& serviceDescription); + bool remove(const capro::ServiceDescription& serviceDescription) noexcept; /// @brief Removes given service description from registry /// @param[in] searchResult, reference to the vector which will be filled with the results @@ -60,11 +60,11 @@ class ServiceRegistry /// @param[in] service, string or wildcard to search for void find(ServiceDescriptionVector_t& searchResult, const capro::IdString_t& service = Wildcard, - const capro::IdString_t& instinstanceance = Wildcard) const; + const capro::IdString_t& instinstanceance = Wildcard) const noexcept; /// @brief Returns all service descriptions as copy /// @return ServiceDescriptionVector_t, copy of complete service registry - const ServiceDescriptionVector_t getAllServices() const; + const ServiceDescriptionVector_t getServices() const noexcept; private: /// @todo #859 replace std::map with prefix tree diff --git a/iceoryx_posh/source/roudi/port_manager.cpp b/iceoryx_posh/source/roudi/port_manager.cpp index 95bf96ec0e..c12576d9c5 100644 --- a/iceoryx_posh/source/roudi/port_manager.cpp +++ b/iceoryx_posh/source/roudi/port_manager.cpp @@ -268,7 +268,7 @@ void PortManager::handleInterfaces() noexcept // also forward services from service registry /// @todo #415 do we still need this? yes but return a copy here to be stored in shared memory via new /// StatusPort's - auto serviceVector = m_serviceRegistry.getAllServices(); + auto serviceVector = m_serviceRegistry.getServices(); caproMessage.m_subType = capro::CaproMessageSubType::SERVICE; @@ -278,8 +278,7 @@ void PortManager::handleInterfaces() noexcept for (auto& interfacePortData : interfacePortsForInitialForwarding) { - auto interfacePort = popo::InterfacePort(interfacePortData); - interfacePort.dispatchCaProMessage(caproMessage); + popo::InterfacePort(interfacePortData).dispatchCaProMessage(caproMessage); } } } @@ -303,15 +302,12 @@ void PortManager::handleApplications() noexcept { case capro::CaproMessageType::OFFER: { - auto serviceDescription = caproMessage.m_serviceDescription; - addEntryToServiceRegistry(serviceDescription); + addEntryToServiceRegistry(caproMessage.m_serviceDescription); break; } case capro::CaproMessageType::STOP_OFFER: { - auto serviceDescription = caproMessage.m_serviceDescription; - removeEntryFromServiceRegistry(serviceDescription); - + removeEntryFromServiceRegistry(caproMessage.m_serviceDescription); break; } default: diff --git a/iceoryx_posh/source/roudi/service_registry.cpp b/iceoryx_posh/source/roudi/service_registry.cpp index cf0f8ab2d6..5782b2a483 100644 --- a/iceoryx_posh/source/roudi/service_registry.cpp +++ b/iceoryx_posh/source/roudi/service_registry.cpp @@ -23,28 +23,27 @@ namespace iox { namespace roudi { -cxx::expected -ServiceRegistry::add(const capro::ServiceDescription& serviceDescription) +cxx::expected ServiceRegistry::add(const capro::ServiceDescription& serviceDescription) noexcept { // Forbid duplicate service descriptions entries for (auto& element : m_serviceDescriptionVector) { if (element == serviceDescription) { - return cxx::error(ServiceRegistryError::SERVICE_DESCRIPTION_ALREADY_ADDED); + return cxx::error(Error::SERVICE_DESCRIPTION_ALREADY_ADDED); } } if (!m_serviceDescriptionVector.push_back(serviceDescription)) { - return cxx::error(ServiceRegistryError::SERVICE_REGISTRY_FULL); + return cxx::error(Error::SERVICE_REGISTRY_FULL); } m_serviceMap.insert({serviceDescription.getServiceIDString(), m_serviceDescriptionVector.size() - 1}); m_instanceMap.insert({serviceDescription.getInstanceIDString(), m_serviceDescriptionVector.size() - 1}); return cxx::success<>(); } -bool ServiceRegistry::remove(const capro::ServiceDescription& serviceDescription) +bool ServiceRegistry::remove(const capro::ServiceDescription& serviceDescription) noexcept { bool removedElement{false}; @@ -104,7 +103,7 @@ bool ServiceRegistry::remove(const capro::ServiceDescription& serviceDescription void ServiceRegistry::find(ServiceDescriptionVector_t& searchResult, const capro::IdString_t& service, - const capro::IdString_t& instance) const + const capro::IdString_t& instance) const noexcept { cxx::vector intersection; @@ -169,7 +168,7 @@ void ServiceRegistry::find(ServiceDescriptionVector_t& searchResult, } } -const ServiceRegistry::ServiceDescriptionVector_t ServiceRegistry::getAllServices() const +const ServiceRegistry::ServiceDescriptionVector_t ServiceRegistry::getServices() const noexcept { return m_serviceDescriptionVector; } diff --git a/iceoryx_posh/source/runtime/posh_runtime_impl.cpp b/iceoryx_posh/source/runtime/posh_runtime_impl.cpp index 839675dd2c..81b5dbd8d6 100644 --- a/iceoryx_posh/source/runtime/posh_runtime_impl.cpp +++ b/iceoryx_posh/source/runtime/posh_runtime_impl.cpp @@ -17,6 +17,7 @@ #include "iceoryx_posh/internal/runtime/posh_runtime_impl.hpp" +#include "iceoryx_hoofs/cxx/algorithm.hpp" #include "iceoryx_hoofs/cxx/convert.hpp" #include "iceoryx_hoofs/cxx/helplets.hpp" #include "iceoryx_hoofs/cxx/variant.hpp" @@ -415,7 +416,7 @@ PoshRuntimeImpl::findService(const cxx::variant servic uint32_t capacity = static_cast(serviceContainer.capacity()); // Limit the services (max value is the capacity of serviceContainer) - uint32_t numberOfServices = ((numberOfElements > capacity) ? capacity : numberOfElements); + uint32_t numberOfServices = algorithm::min(capacity, numberOfElements); for (uint32_t i = 0; i < numberOfServices; ++i) { capro::ServiceDescription service(cxx::Serialization(requestResponse.getElementAtIndex(i))); diff --git a/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp b/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp index 045a836099..304f3b3ca5 100644 --- a/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp +++ b/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp @@ -196,7 +196,7 @@ TEST_F(ServiceRegistry_test, AddingMultipleServiceDescriptionAndRemovingAllDoesN EXPECT_THAT(searchResults.size(), Eq(0)); } -TEST_F(ServiceRegistry_test, AddingVariousServiceDescriptionAndGetAllServicesDoesNotReturnDuplicate) +TEST_F(ServiceRegistry_test, AddingVariousServiceDescriptionAndGetServicesDoesNotReturnDuplicate) { iox::capro::ServiceDescription service1("a", "b", "b"); iox::capro::ServiceDescription service2("a", "c", "c"); @@ -208,12 +208,11 @@ TEST_F(ServiceRegistry_test, AddingVariousServiceDescriptionAndGetAllServicesDoe ASSERT_FALSE(registry.add(service2).has_error()); auto result = registry.add(service2); ASSERT_TRUE(result.has_error()); - EXPECT_THAT(result.get_error(), - Eq(iox::roudi::ServiceRegistry::ServiceRegistryError::SERVICE_DESCRIPTION_ALREADY_ADDED)); + EXPECT_THAT(result.get_error(), Eq(iox::roudi::ServiceRegistry::Error::SERVICE_DESCRIPTION_ALREADY_ADDED)); ASSERT_FALSE(registry.add(service3).has_error()); ASSERT_FALSE(registry.add(service4).has_error()); - auto serviceDescriptionVector = registry.getAllServices(); + auto serviceDescriptionVector = registry.getServices(); bool service1Found = false; bool service2Found = false; From 486a0088f9b0201173852ef3531fb89bba8587ce Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Mon, 28 Jun 2021 17:53:41 +0200 Subject: [PATCH 12/32] iox-#415 Add todo for new discovery API and remove obsolete forward declaration Signed-off-by: Simon Hoinkis --- iceoryx_posh/include/iceoryx_posh/iceoryx_posh_types.hpp | 4 ---- iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp | 4 ++-- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/iceoryx_posh/include/iceoryx_posh/iceoryx_posh_types.hpp b/iceoryx_posh/include/iceoryx_posh/iceoryx_posh_types.hpp index 9989c68f8d..2fe99f2c19 100644 --- a/iceoryx_posh/include/iceoryx_posh/iceoryx_posh_types.hpp +++ b/iceoryx_posh/include/iceoryx_posh/iceoryx_posh_types.hpp @@ -46,10 +46,6 @@ namespace capro { class ServiceDescription; } -namespace posix -{ -class UnixDomainSocket; -} // namespace posix using PublisherPortRouDiType = iox::popo::PublisherPortRouDi; using PublisherPortUserType = iox::popo::PublisherPortUser; diff --git a/iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp b/iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp index 5d9962476e..932512fa8f 100644 --- a/iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp +++ b/iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp @@ -47,8 +47,8 @@ class NodeData; enum class FindServiceError { INVALID_STATE, - UNABLE_TO_WRITE_TO_ROUDI_CHANNEL, - INSTANCE_CONTAINER_OVERFLOW + UNABLE_TO_WRITE_TO_ROUDI_CHANNEL, /// @todo #415 remove as IPC channel won't be used + INSTANCE_CONTAINER_OVERFLOW /// @todo #415 set container to iox::MAX_NUMBER_OF_SERVICES and remove error }; /// @brief Used to search for any string (wildcard) From d2c0aaf6b6c59f9e5ddc6d6000e8eb3ca29269c9 Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Wed, 30 Jun 2021 10:58:00 +0200 Subject: [PATCH 13/32] iox-#415 Add new error in case adding entry to ServiceRegistry fails Signed-off-by: Simon Hoinkis --- .../include/iceoryx_hoofs/error_handling/error_handling.hpp | 1 + iceoryx_posh/source/roudi/port_manager.cpp | 5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/iceoryx_hoofs/include/iceoryx_hoofs/error_handling/error_handling.hpp b/iceoryx_hoofs/include/iceoryx_hoofs/error_handling/error_handling.hpp index 97071b7ca9..54027925af 100644 --- a/iceoryx_hoofs/include/iceoryx_hoofs/error_handling/error_handling.hpp +++ b/iceoryx_hoofs/include/iceoryx_hoofs/error_handling/error_handling.hpp @@ -56,6 +56,7 @@ namespace iox error(POSH__RUNTIME_NAME_EMPTY) \ error(POSH__RUNTIME_LEADING_SLASH_PROVIDED) \ error(POSH__PORT_MANAGER_PUBLISHERPORT_NOT_UNIQUE) \ + error(POSH__PORT_MANAGER_COULD_NOT_ADD_SERVICE_TO_REGISTRY) \ error(POSH__MEMPOOL_POSSIBLE_DOUBLE_FREE) \ error(POSH__RECEIVERPORT_DELIVERYFIFO_OVERFLOW) \ error(POSH__SENDERPORT_SAMPLE_SIZE_CHANGED_FOR_ACTIVE_PORT) \ diff --git a/iceoryx_posh/source/roudi/port_manager.cpp b/iceoryx_posh/source/roudi/port_manager.cpp index c12576d9c5..0f256e2de8 100644 --- a/iceoryx_posh/source/roudi/port_manager.cpp +++ b/iceoryx_posh/source/roudi/port_manager.cpp @@ -715,8 +715,9 @@ popo::ApplicationPortData* PortManager::acquireApplicationPortData(const Runtime void PortManager::addEntryToServiceRegistry(const capro::ServiceDescription& service) noexcept { - m_serviceRegistry.add(service).or_else([](auto&) { - /// @todo #415 return something? + m_serviceRegistry.add(service).or_else([&](auto&) { + LogWarn() << "Could not add service " << service.getServiceIDString() << " to service registry!"; + errorHandler(Error::kPOSH__PORT_MANAGER_COULD_NOT_ADD_SERVICE_TO_REGISTRY, nullptr, ErrorLevel::MODERATE); }); m_portPool->serviceRegistryChangeCounter()->fetch_add(1, std::memory_order_relaxed); } From cef63b544f7a2b79e9c9d8c3329965aa591e3407 Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Wed, 30 Jun 2021 17:22:44 +0200 Subject: [PATCH 14/32] iox-#415 Fix PoshRuntime and FindService tests Signed-off-by: Simon Hoinkis --- .../integrationtests/test_roudi_findservice.cpp | 4 ++-- .../test/moduletests/test_posh_runtime.cpp | 15 +++++++++++---- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/iceoryx_posh/test/integrationtests/test_roudi_findservice.cpp b/iceoryx_posh/test/integrationtests/test_roudi_findservice.cpp index 2665b00ec7..9159c611da 100644 --- a/iceoryx_posh/test/integrationtests/test_roudi_findservice.cpp +++ b/iceoryx_posh/test/integrationtests/test_roudi_findservice.cpp @@ -166,7 +166,7 @@ TEST_F(RoudiFindService_test, OfferMultiMethodServiceWithDistinctSingleInstance) instanceContainer = receiverRuntime->findService(IdString_t("service2"), IdString_t("instance2")); ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service2", "instance2", "event1"})); + ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service2", "instance2", "event2"})); } TEST_F(RoudiFindService_test, SubscribeAnyInstance) @@ -355,7 +355,7 @@ TEST_F(RoudiFindService_test, InterfacePort) auto caproMessage = maybeCaProMessage.value(); if ((caproMessage.m_serviceDescription.getServiceIDString() == IdString_t("service1")) && (caproMessage.m_serviceDescription.getInstanceIDString() == IdString_t("instance1")) - && ((caproMessage.m_serviceDescription.getEventIDString() == iox::roudi::Wildcard))) + && ((caproMessage.m_serviceDescription.getEventIDString() == IdString_t("event1")))) { serviceFound = true; break; diff --git a/iceoryx_posh/test/moduletests/test_posh_runtime.cpp b/iceoryx_posh/test/moduletests/test_posh_runtime.cpp index 5127d4bc5e..ed10b11c2e 100644 --- a/iceoryx_posh/test/moduletests/test_posh_runtime.cpp +++ b/iceoryx_posh/test/moduletests/test_posh_runtime.cpp @@ -672,15 +672,22 @@ TEST_F(PoshRuntime_test, OfferEmptyServiceIsInvalid) EXPECT_FALSE(isServiceOffered); } -TEST_F(PoshRuntime_test, FindServiceReturnsNoInstanceForDefaultDescription) +TEST_F(PoshRuntime_test, FindServiceWithWildcardsReturnsOnlyIntrospectionServices) { PoshRuntime* m_receiverRuntime{&iox::runtime::PoshRuntime::initRuntime("subscriber")}; - m_runtime->offerService(iox::capro::ServiceDescription()); + EXPECT_FALSE(m_runtime->offerService(iox::capro::ServiceDescription())); this->InterOpWait(); - auto instanceContainer = m_receiverRuntime->findService(iox::runtime::Any_t(), iox::runtime::Any_t()); - EXPECT_THAT(0u, instanceContainer.value().size()); + auto serviceContainer = m_receiverRuntime->findService(iox::runtime::Any_t(), iox::runtime::Any_t()); + ASSERT_FALSE(serviceContainer.has_error()); + + auto searchResult = serviceContainer.value(); + + for (auto& service : searchResult) + { + EXPECT_THAT(service.getServiceIDString().c_str(), StrEq("Introspection")); + } } TEST_F(PoshRuntime_test, ShutdownUnblocksBlockingPublisher) From 2fec024fa0dc4d599408c7261e60351f05421274 Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Thu, 1 Jul 2021 11:12:59 +0200 Subject: [PATCH 15/32] iox-#415 Implement out-commented tests for ServiceRegistry Signed-off-by: Simon Hoinkis --- .../internal/roudi/service_registry.hpp | 1 + .../test_roudi_service_registry.cpp | 98 +++++++++++++++++-- 2 files changed, 89 insertions(+), 10 deletions(-) diff --git a/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp b/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp index 1c14b9ec61..046eb75875 100644 --- a/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp +++ b/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp @@ -39,6 +39,7 @@ class ServiceRegistry INVALID_STATE, SERVICE_DESCRIPTION_ALREADY_ADDED, SERVICE_REGISTRY_FULL, + SERVICE_DESCRIPTION_INVALID, }; /// @todo #415 should be connected with iox::MAX_NUMBER_OF_SERVICES static constexpr uint32_t MAX_SERVICE_DESCRIPTIONS = 100U; diff --git a/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp b/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp index 304f3b3ca5..bc4fab39b4 100644 --- a/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp +++ b/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp @@ -47,16 +47,94 @@ class ServiceRegistry_test : public Test iox::roudi::ServiceRegistry::ServiceDescriptionVector_t searchResults; }; -/// @todo #415 implement missing tests -// TEST_F(ServiceRegistry_test, AddNoServiceDescriptionsAndWildcardSearchReturnsNothing) -// TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFoundWithWildcardSearch) -// TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFoundWithInstanceName) -// TEST_F(ServiceRegistry_test, AddMaximumNumberOfServiceDescriptionsWorks) -// TEST_F(ServiceRegistry_test, AddMoreThanMaximumNumberOfServiceDescriptionsFails) -// TEST_F(ServiceRegistry_test, AddServiceDescriptionsWhichWasAlreadyAddedDoesNotWork) -// TEST_F(ServiceRegistry_test, AddInvalidServiceDescriptionsFails) -// TEST_F(ServiceRegistry_test, RemovingServiceDescriptionsWhichWasntAddedFails) -// TEST_F(ServiceRegistry_test, RemovingInvalidServiceDescriptionsFails) +TEST_F(ServiceRegistry_test, AddNoServiceDescriptionsAndWildcardSearchReturnsNothing) +{ + registry.find(searchResults, Wildcard, Wildcard); + + EXPECT_THAT(searchResults.size(), Eq(0)); +} +TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFoundWithWildcardSearch) +{ + auto result = registry.add(ServiceDescription("Foo", "Bar", "Baz")); + ASSERT_FALSE(result.has_error()); + registry.find(searchResults, Wildcard, Wildcard); + + EXPECT_THAT(searchResults.size(), Eq(1)); + EXPECT_THAT(searchResults[0], Eq(ServiceDescription("Foo", "Bar", "Baz"))); +} +TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFoundWithInstanceName) +{ + auto result = registry.add(ServiceDescription("Foo", "Bar", "Baz")); + ASSERT_FALSE(result.has_error()); + registry.find(searchResults, Wildcard, "Bar"); + + EXPECT_THAT(searchResults.size(), Eq(1)); + EXPECT_THAT(searchResults[0], Eq(ServiceDescription("Foo", "Bar", "Baz"))); +} +TEST_F(ServiceRegistry_test, AddMaximumNumberOfServiceDescriptionsWorks) +{ + iox::cxx::vector services; + + for (uint64_t i = 0U; i < ServiceRegistry::MAX_SERVICE_DESCRIPTIONS; i++) + { + services.push_back(iox::capro::ServiceDescription( + "Foo", "Bar", iox::capro::IdString_t(iox::cxx::TruncateToCapacity, iox::cxx::convert::toString(i)))); + } + + for (auto& service : services) + { + auto result = registry.add(service); + ASSERT_FALSE(result.has_error()); + } +} + +TEST_F(ServiceRegistry_test, AddMoreThanMaximumNumberOfServiceDescriptionsFails) +{ + iox::cxx::vector services; + + for (uint64_t i = 0U; i < ServiceRegistry::MAX_SERVICE_DESCRIPTIONS; i++) + { + services.push_back(iox::capro::ServiceDescription( + "Foo", "Bar", iox::capro::IdString_t(iox::cxx::TruncateToCapacity, iox::cxx::convert::toString(i)))); + } + + for (auto& service : services) + { + auto result = registry.add(service); + ASSERT_FALSE(result.has_error()); + } + + auto result = registry.add(iox::capro::ServiceDescription("Foo", "Bar", "Baz")); + ASSERT_TRUE(result.has_error()); + EXPECT_THAT(result.get_error(), Eq(ServiceRegistry::Error::SERVICE_REGISTRY_FULL)); +} + +TEST_F(ServiceRegistry_test, AddServiceDescriptionsWhichWasAlreadyAddedDoesNotWork) +{ + auto result1 = registry.add(ServiceDescription("Li", "La", "Launebaer")); + ASSERT_FALSE(result1.has_error()); + + auto result2 = registry.add(ServiceDescription("Li", "La", "Launebaer")); + ASSERT_TRUE(result2.has_error()); + EXPECT_THAT(result2.get_error(), Eq(ServiceRegistry::Error::SERVICE_DESCRIPTION_ALREADY_ADDED)); +} + +TEST_F(ServiceRegistry_test, AddInvalidServiceDescriptionsFails) +{ + auto result = registry.add(ServiceDescription()); + ASSERT_TRUE(result.has_error()); + EXPECT_THAT(result.get_error(), Eq(ServiceRegistry::Error::SERVICE_DESCRIPTION_INVALID)); +} + +TEST_F(ServiceRegistry_test, RemovingServiceDescriptionsWhichWasntAddedFails) +{ + EXPECT_FALSE(registry.remove(ServiceDescription("Sim", "Sa", "Lambim"))); +} + +TEST_F(ServiceRegistry_test, RemovingInvalidServiceDescriptionsFails) +{ + EXPECT_FALSE(registry.remove(ServiceDescription())); +} TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFoundWithServiceName) { From 479ea0bc7df75c1d3a9b3dba93e840a561d4bb3e Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Thu, 1 Jul 2021 11:20:01 +0200 Subject: [PATCH 16/32] iox-#415 Add validity check to ServiceRegistry::{add,remove} Signed-off-by: Simon Hoinkis --- iceoryx_posh/source/roudi/service_registry.cpp | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/iceoryx_posh/source/roudi/service_registry.cpp b/iceoryx_posh/source/roudi/service_registry.cpp index 5782b2a483..33adb29e41 100644 --- a/iceoryx_posh/source/roudi/service_registry.cpp +++ b/iceoryx_posh/source/roudi/service_registry.cpp @@ -25,6 +25,11 @@ namespace roudi { cxx::expected ServiceRegistry::add(const capro::ServiceDescription& serviceDescription) noexcept { + if (!serviceDescription.isValid()) + { + return cxx::error(Error::SERVICE_DESCRIPTION_INVALID); + } + // Forbid duplicate service descriptions entries for (auto& element : m_serviceDescriptionVector) { @@ -45,8 +50,13 @@ cxx::expected ServiceRegistry::add(const capro::ServiceD bool ServiceRegistry::remove(const capro::ServiceDescription& serviceDescription) noexcept { - bool removedElement{false}; + if (!serviceDescription.isValid()) + { + return false; + } + + bool removedElement{false}; uint64_t index = 0U; for (auto iterator = m_serviceDescriptionVector.begin(); iterator != m_serviceDescriptionVector.end();) { From 10b3658bf0b6c6e04b2c64de254ac925f85aedc2 Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Thu, 1 Jul 2021 11:27:00 +0200 Subject: [PATCH 17/32] iox-#415 Rename Any_t to Wildcard_t Signed-off-by: Simon Hoinkis --- .../iceoryx_posh/internal/runtime/posh_runtime_impl.hpp | 4 ++-- .../include/iceoryx_posh/runtime/posh_runtime.hpp | 8 ++++---- iceoryx_posh/source/runtime/posh_runtime_impl.cpp | 4 ++-- .../test/integrationtests/test_roudi_findservice.cpp | 6 +++--- iceoryx_posh/test/mocks/posh_runtime_mock.hpp | 8 ++++---- iceoryx_posh/test/moduletests/test_posh_runtime.cpp | 2 +- 6 files changed, 16 insertions(+), 16 deletions(-) diff --git a/iceoryx_posh/include/iceoryx_posh/internal/runtime/posh_runtime_impl.hpp b/iceoryx_posh/include/iceoryx_posh/internal/runtime/posh_runtime_impl.hpp index 854e97f033..229e76f283 100644 --- a/iceoryx_posh/include/iceoryx_posh/internal/runtime/posh_runtime_impl.hpp +++ b/iceoryx_posh/include/iceoryx_posh/internal/runtime/posh_runtime_impl.hpp @@ -45,8 +45,8 @@ class PoshRuntimeImpl : public PoshRuntime /// @copydoc PoshRuntime::findService cxx::expected - findService(const cxx::variant service, - const cxx::variant instance) noexcept override; + findService(const cxx::variant service, + const cxx::variant instance) noexcept override; /// @copydoc PoshRuntime::offerService bool offerService(const capro::ServiceDescription& serviceDescription) noexcept override; diff --git a/iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp b/iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp index 932512fa8f..57bad38f61 100644 --- a/iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp +++ b/iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp @@ -51,8 +51,8 @@ enum class FindServiceError INSTANCE_CONTAINER_OVERFLOW /// @todo #415 set container to iox::MAX_NUMBER_OF_SERVICES and remove error }; -/// @brief Used to search for any string (wildcard) -struct Any_t +/// @brief Used to search for any string +struct Wildcard_t { }; @@ -93,8 +93,8 @@ class PoshRuntime /// ServiceContainer: on success, container that is filled with all matching instances /// FindServiceError: if any, encountered during the operation virtual cxx::expected - findService(const cxx::variant service, - const cxx::variant instance) noexcept = 0; + findService(const cxx::variant service, + const cxx::variant instance) noexcept = 0; /// @brief offer the provided service, sends the offer from application to RouDi daemon /// @param[in] service valid ServiceDescription to offer diff --git a/iceoryx_posh/source/runtime/posh_runtime_impl.cpp b/iceoryx_posh/source/runtime/posh_runtime_impl.cpp index 81b5dbd8d6..c583d9cb86 100644 --- a/iceoryx_posh/source/runtime/posh_runtime_impl.cpp +++ b/iceoryx_posh/source/runtime/posh_runtime_impl.cpp @@ -374,8 +374,8 @@ NodeData* PoshRuntimeImpl::createNode(const NodeProperty& nodeProperty) noexcept } cxx::expected -PoshRuntimeImpl::findService(const cxx::variant service, - const cxx::variant instance) noexcept +PoshRuntimeImpl::findService(const cxx::variant service, + const cxx::variant instance) noexcept { /// @todo #415 remove the string mapping, once the find call is done via shared memory capro::IdString_t serviceString; diff --git a/iceoryx_posh/test/integrationtests/test_roudi_findservice.cpp b/iceoryx_posh/test/integrationtests/test_roudi_findservice.cpp index 9159c611da..1ed7955110 100644 --- a/iceoryx_posh/test/integrationtests/test_roudi_findservice.cpp +++ b/iceoryx_posh/test/integrationtests/test_roudi_findservice.cpp @@ -180,7 +180,7 @@ TEST_F(RoudiFindService_test, SubscribeAnyInstance) instanceContainerExp.push_back({"service1", "instance2", "event2"}); instanceContainerExp.push_back({"service1", "instance3", "event3"}); - auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), iox::runtime::Any_t()); + auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), iox::runtime::Wildcard_t()); ASSERT_FALSE(instanceContainer.has_error()); ASSERT_THAT(instanceContainer.value().size(), Eq(3u)); EXPECT_TRUE(instanceContainer.value() == instanceContainerExp); @@ -378,7 +378,7 @@ TEST_F(RoudiFindService_test, findServiceMaxServices) this->InterOpWait(); } - auto instanceContainer = receiverRuntime->findService(IdString_t("s"), iox::runtime::Any_t()); + auto instanceContainer = receiverRuntime->findService(IdString_t("s"), iox::runtime::Wildcard_t()); ASSERT_FALSE(instanceContainer.has_error()); EXPECT_THAT(instanceContainer.value().size(), Eq(iox::MAX_NUMBER_OF_SERVICES)); @@ -397,7 +397,7 @@ TEST_F(RoudiFindService_test, findServiceInstanceContainerOverflowError) this->InterOpWait(); } - auto instanceContainer = receiverRuntime->findService(IdString_t("s"), iox::runtime::Any_t()); + auto instanceContainer = receiverRuntime->findService(IdString_t("s"), iox::runtime::Wildcard_t()); ASSERT_THAT(instanceContainer.has_error(), Eq(true)); } diff --git a/iceoryx_posh/test/mocks/posh_runtime_mock.hpp b/iceoryx_posh/test/mocks/posh_runtime_mock.hpp index b26dc85fae..aecd923ff7 100644 --- a/iceoryx_posh/test/mocks/posh_runtime_mock.hpp +++ b/iceoryx_posh/test/mocks/posh_runtime_mock.hpp @@ -48,8 +48,8 @@ class PoshRuntimeMock : public iox::runtime::PoshRuntime /// @todo iox-#841 simplify this when we switch to gmock v1.10 MOCK_METHOD2(findServiceMock, iox::cxx::expected( - const iox::cxx::variant, - const iox::cxx::variant)); + const iox::cxx::variant, + const iox::cxx::variant)); MOCK_METHOD1(offerServiceMock, bool(const iox::capro::ServiceDescription&)); MOCK_METHOD1(stopOfferServiceMock, bool(const iox::capro::ServiceDescription&)); MOCK_METHOD3(getMiddlewarePublisherMock, @@ -90,8 +90,8 @@ class PoshRuntimeMock : public iox::runtime::PoshRuntime } iox::cxx::expected - findService(const iox::cxx::variant service, - const iox::cxx::variant instance) noexcept override + findService(const iox::cxx::variant service, + const iox::cxx::variant instance) noexcept override { return findServiceMock(service, instance); } diff --git a/iceoryx_posh/test/moduletests/test_posh_runtime.cpp b/iceoryx_posh/test/moduletests/test_posh_runtime.cpp index ed10b11c2e..54a073b28f 100644 --- a/iceoryx_posh/test/moduletests/test_posh_runtime.cpp +++ b/iceoryx_posh/test/moduletests/test_posh_runtime.cpp @@ -679,7 +679,7 @@ TEST_F(PoshRuntime_test, FindServiceWithWildcardsReturnsOnlyIntrospectionService EXPECT_FALSE(m_runtime->offerService(iox::capro::ServiceDescription())); this->InterOpWait(); - auto serviceContainer = m_receiverRuntime->findService(iox::runtime::Any_t(), iox::runtime::Any_t()); + auto serviceContainer = m_receiverRuntime->findService(iox::runtime::Wildcard_t(), iox::runtime::Wildcard_t()); ASSERT_FALSE(serviceContainer.has_error()); auto searchResult = serviceContainer.value(); From fcb415a8a92c7d2e6fe375bff90373f404c996de Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Thu, 1 Jul 2021 15:13:33 +0200 Subject: [PATCH 18/32] iox-#415 Simplify ServiceRegistry::remove Signed-off-by: Simon Hoinkis --- .../source/roudi/service_registry.cpp | 60 +++++++------------ 1 file changed, 22 insertions(+), 38 deletions(-) diff --git a/iceoryx_posh/source/roudi/service_registry.cpp b/iceoryx_posh/source/roudi/service_registry.cpp index 33adb29e41..9523b3248c 100644 --- a/iceoryx_posh/source/roudi/service_registry.cpp +++ b/iceoryx_posh/source/roudi/service_registry.cpp @@ -55,50 +55,12 @@ bool ServiceRegistry::remove(const capro::ServiceDescription& serviceDescription return false; } - bool removedElement{false}; uint64_t index = 0U; for (auto iterator = m_serviceDescriptionVector.begin(); iterator != m_serviceDescriptionVector.end();) { if (m_serviceDescriptionVector[index] == serviceDescription) { - uint64_t removedValue{0U}; - bool removedEntry{false}; - for (auto it = m_serviceMap.begin(); it != m_serviceMap.end();) - { - if (it->second == index) - { - removedValue = it->second; - it = m_serviceMap.erase(it); - removedEntry = true; - continue; - } - else if (removedEntry && it->second > removedValue) - { - // update index due to removed element - it->second--; - } - it++; - } - - removedValue = 0U; - removedEntry = false; - for (auto it = m_instanceMap.begin(); it != m_instanceMap.end();) - { - if (it->second == index) - { - removedValue = it->second; - it = m_instanceMap.erase(it); - removedEntry = true; - continue; - } - else if (removedEntry && it->second > removedValue) - { - // update index due to removed element - it->second--; - } - it++; - } m_serviceDescriptionVector.erase(iterator); removedElement = true; // There can be not more than one element @@ -108,6 +70,28 @@ bool ServiceRegistry::remove(const capro::ServiceDescription& serviceDescription iterator++; } + auto removeIndexFromMap = [](std::multimap& map, uint64_t index) { + bool removedEntry{false}; + for (auto it = map.begin(); it != map.end();) + { + if (it->second == index) + { + it = map.erase(it); + removedEntry = true; + continue; + } + else if (removedEntry && it->second > index) + { + // update index due to removed element + it->second--; + } + it++; + } + }; + + removeIndexFromMap(m_serviceMap, index); + removeIndexFromMap(m_instanceMap, index); + return removedElement; } From 4ffa0255c82e322aff951bb8b7235246b4b60b3f Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Thu, 1 Jul 2021 15:32:06 +0200 Subject: [PATCH 19/32] iox-#415 Move else case in ServiceRegistry::find one level up Signed-off-by: Simon Hoinkis --- .../source/roudi/service_registry.cpp | 43 +++++++++---------- 1 file changed, 20 insertions(+), 23 deletions(-) diff --git a/iceoryx_posh/source/roudi/service_registry.cpp b/iceoryx_posh/source/roudi/service_registry.cpp index 9523b3248c..bd219731ed 100644 --- a/iceoryx_posh/source/roudi/service_registry.cpp +++ b/iceoryx_posh/source/roudi/service_registry.cpp @@ -131,35 +131,32 @@ void ServiceRegistry::find(ServiceDescriptionVector_t& searchResult, searchResult.push_back(m_serviceDescriptionVector[value]); } } - else + // Find (*, K2) + // O(log n + #result) + else if (service == Wildcard && instance != Wildcard) { - // Find (*, K2) - // O(log n + #result) - if (service == Wildcard && instance != Wildcard) - { - auto range = m_instanceMap.equal_range(instance); - for (auto entry = range.first; entry != range.second; ++entry) - { - searchResult.push_back(m_serviceDescriptionVector[entry->second]); - } - } - // Find (K1, *) - // O(log n + #result) - else if (instance == Wildcard && service != Wildcard) + auto range = m_instanceMap.equal_range(instance); + for (auto entry = range.first; entry != range.second; ++entry) { - auto range = m_serviceMap.equal_range(service); - for (auto entry = range.first; entry != range.second; ++entry) - { - searchResult.push_back(m_serviceDescriptionVector[entry->second]); - } + searchResult.push_back(m_serviceDescriptionVector[entry->second]); } - else + } + // Find (K1, *) + // O(log n + #result) + else if (instance == Wildcard && service != Wildcard) + { + auto range = m_serviceMap.equal_range(service); + for (auto entry = range.first; entry != range.second; ++entry) { - // Find (*, *) - // O(1) - searchResult = m_serviceDescriptionVector; + searchResult.push_back(m_serviceDescriptionVector[entry->second]); } } + else + { + // Find (*, *) + // O(1) + searchResult = m_serviceDescriptionVector; + } } const ServiceRegistry::ServiceDescriptionVector_t ServiceRegistry::getServices() const noexcept From 7cc5ee837cf7d292bed6fa330ec75ffeeafd7749 Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Thu, 1 Jul 2021 15:46:42 +0200 Subject: [PATCH 20/32] iox-#415 Rename instanceContainer to serviceContainer in findService tests Signed-off-by: Simon Hoinkis --- .../internal/roudi/service_registry.hpp | 2 +- .../test_roudi_findservice.cpp | 266 +++++++++--------- 2 files changed, 134 insertions(+), 134 deletions(-) diff --git a/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp b/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp index 046eb75875..c6d9645dcc 100644 --- a/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp +++ b/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp @@ -68,7 +68,7 @@ class ServiceRegistry const ServiceDescriptionVector_t getServices() const noexcept; private: - /// @todo #859 replace std::map with prefix tree + /// @todo #859 replace std::multimap with prefix tree ::std::multimap m_serviceMap; ::std::multimap m_instanceMap; ServiceDescriptionVector_t m_serviceDescriptionVector; diff --git a/iceoryx_posh/test/integrationtests/test_roudi_findservice.cpp b/iceoryx_posh/test/integrationtests/test_roudi_findservice.cpp index 1ed7955110..b42263163c 100644 --- a/iceoryx_posh/test/integrationtests/test_roudi_findservice.cpp +++ b/iceoryx_posh/test/integrationtests/test_roudi_findservice.cpp @@ -45,11 +45,11 @@ TEST_F(RoudiFindService_test, OfferSingleMethodServiceSingleInstance) auto isServiceOffered = senderRuntime->offerService({"service1", "instance1", "event1"}); this->InterOpWait(); - auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); + auto serviceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); - ASSERT_FALSE(instanceContainer.has_error()); - ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); + ASSERT_FALSE(serviceContainer.has_error()); + ASSERT_THAT(serviceContainer.value().size(), Eq(1u)); + ASSERT_THAT(*serviceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); ASSERT_EQ(true, isServiceOffered); } @@ -88,11 +88,11 @@ TEST_F(RoudiFindService_test, ReofferedServiceWithValidServiceDescriptionCanBeFo EXPECT_TRUE(senderRuntime->offerService({"service1", "instance1", "event1"})); this->InterOpWait(); - auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); + auto serviceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); - ASSERT_FALSE(instanceContainer.has_error()); - ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); + ASSERT_FALSE(serviceContainer.has_error()); + ASSERT_THAT(serviceContainer.value().size(), Eq(1u)); + ASSERT_THAT(*serviceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); } TEST_F(RoudiFindService_test, OfferExsistingServiceMultipleTimesIsRedundant) @@ -102,11 +102,11 @@ TEST_F(RoudiFindService_test, OfferExsistingServiceMultipleTimesIsRedundant) EXPECT_TRUE(senderRuntime->offerService({"service1", "instance1", "event1"})); this->InterOpWait(); - auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); + auto serviceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); - ASSERT_FALSE(instanceContainer.has_error()); - ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); + ASSERT_FALSE(serviceContainer.has_error()); + ASSERT_THAT(serviceContainer.value().size(), Eq(1u)); + ASSERT_THAT(*serviceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); } TEST_F(RoudiFindService_test, FindSameServiceMultipleTimesReturnsSingleInstance) @@ -114,15 +114,15 @@ TEST_F(RoudiFindService_test, FindSameServiceMultipleTimesReturnsSingleInstance) EXPECT_TRUE(senderRuntime->offerService({"service1", "instance1", "event1"})); this->InterOpWait(); - auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); - ASSERT_FALSE(instanceContainer.has_error()); - ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); + auto serviceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); + ASSERT_FALSE(serviceContainer.has_error()); + ASSERT_THAT(serviceContainer.value().size(), Eq(1u)); + ASSERT_THAT(*serviceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); - instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); - ASSERT_FALSE(instanceContainer.has_error()); - ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); + serviceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); + ASSERT_FALSE(serviceContainer.has_error()); + ASSERT_THAT(serviceContainer.value().size(), Eq(1u)); + ASSERT_THAT(*serviceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); } TEST_F(RoudiFindService_test, OfferMultiMethodServiceSingleInstance) @@ -132,20 +132,20 @@ TEST_F(RoudiFindService_test, OfferMultiMethodServiceSingleInstance) EXPECT_TRUE(senderRuntime->offerService({"service3", "instance1", "event1"})); this->InterOpWait(); - auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); - ASSERT_FALSE(instanceContainer.has_error()); - ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); + auto serviceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); + ASSERT_FALSE(serviceContainer.has_error()); + ASSERT_THAT(serviceContainer.value().size(), Eq(1u)); + ASSERT_THAT(*serviceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); - instanceContainer = receiverRuntime->findService(IdString_t("service2"), IdString_t("instance1")); - ASSERT_FALSE(instanceContainer.has_error()); - ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service2", "instance1", "event1"})); + serviceContainer = receiverRuntime->findService(IdString_t("service2"), IdString_t("instance1")); + ASSERT_FALSE(serviceContainer.has_error()); + ASSERT_THAT(serviceContainer.value().size(), Eq(1u)); + ASSERT_THAT(*serviceContainer.value().begin(), Eq(ServiceDescription{"service2", "instance1", "event1"})); - instanceContainer = receiverRuntime->findService(IdString_t("service3"), IdString_t("instance1")); - ASSERT_FALSE(instanceContainer.has_error()); - ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service3", "instance1", "event1"})); + serviceContainer = receiverRuntime->findService(IdString_t("service3"), IdString_t("instance1")); + ASSERT_FALSE(serviceContainer.has_error()); + ASSERT_THAT(serviceContainer.value().size(), Eq(1u)); + ASSERT_THAT(*serviceContainer.value().begin(), Eq(ServiceDescription{"service3", "instance1", "event1"})); } TEST_F(RoudiFindService_test, OfferMultiMethodServiceWithDistinctSingleInstance) @@ -154,19 +154,19 @@ TEST_F(RoudiFindService_test, OfferMultiMethodServiceWithDistinctSingleInstance) EXPECT_TRUE(senderRuntime->offerService({"service2", "instance2", "event2"})); this->InterOpWait(); - auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); - ASSERT_FALSE(instanceContainer.has_error()); - ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); + auto serviceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); + ASSERT_FALSE(serviceContainer.has_error()); + ASSERT_THAT(serviceContainer.value().size(), Eq(1u)); + ASSERT_THAT(*serviceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); - instanceContainer = receiverRuntime->findService(IdString_t("service2"), IdString_t("instance1")); - ASSERT_FALSE(instanceContainer.has_error()); - ASSERT_THAT(instanceContainer.value().size(), Eq(0u)); + serviceContainer = receiverRuntime->findService(IdString_t("service2"), IdString_t("instance1")); + ASSERT_FALSE(serviceContainer.has_error()); + ASSERT_THAT(serviceContainer.value().size(), Eq(0u)); - instanceContainer = receiverRuntime->findService(IdString_t("service2"), IdString_t("instance2")); - ASSERT_FALSE(instanceContainer.has_error()); - ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service2", "instance2", "event2"})); + serviceContainer = receiverRuntime->findService(IdString_t("service2"), IdString_t("instance2")); + ASSERT_FALSE(serviceContainer.has_error()); + ASSERT_THAT(serviceContainer.value().size(), Eq(1u)); + ASSERT_THAT(*serviceContainer.value().begin(), Eq(ServiceDescription{"service2", "instance2", "event2"})); } TEST_F(RoudiFindService_test, SubscribeAnyInstance) @@ -175,15 +175,15 @@ TEST_F(RoudiFindService_test, SubscribeAnyInstance) EXPECT_TRUE(senderRuntime->offerService({"service1", "instance2", "event2"})); EXPECT_TRUE(senderRuntime->offerService({"service1", "instance3", "event3"})); this->InterOpWait(); - ServiceContainer instanceContainerExp; - instanceContainerExp.push_back({"service1", "instance1", "event1"}); - instanceContainerExp.push_back({"service1", "instance2", "event2"}); - instanceContainerExp.push_back({"service1", "instance3", "event3"}); - - auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), iox::runtime::Wildcard_t()); - ASSERT_FALSE(instanceContainer.has_error()); - ASSERT_THAT(instanceContainer.value().size(), Eq(3u)); - EXPECT_TRUE(instanceContainer.value() == instanceContainerExp); + ServiceContainer serviceContainerExp; + serviceContainerExp.push_back({"service1", "instance1", "event1"}); + serviceContainerExp.push_back({"service1", "instance2", "event2"}); + serviceContainerExp.push_back({"service1", "instance3", "event3"}); + + auto serviceContainer = receiverRuntime->findService(IdString_t("service1"), iox::runtime::Wildcard_t()); + ASSERT_FALSE(serviceContainer.has_error()); + ASSERT_THAT(serviceContainer.value().size(), Eq(3u)); + EXPECT_TRUE(serviceContainer.value() == serviceContainerExp); } TEST_F(RoudiFindService_test, OfferSingleMethodServiceMultiInstance) @@ -193,20 +193,20 @@ TEST_F(RoudiFindService_test, OfferSingleMethodServiceMultiInstance) EXPECT_TRUE(senderRuntime->offerService({"service1", "instance3", "event3"})); this->InterOpWait(); - auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); - ASSERT_FALSE(instanceContainer.has_error()); - ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); + auto serviceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); + ASSERT_FALSE(serviceContainer.has_error()); + ASSERT_THAT(serviceContainer.value().size(), Eq(1u)); + ASSERT_THAT(*serviceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); - instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance2")); - ASSERT_FALSE(instanceContainer.has_error()); - ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance2", "event2"})); + serviceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance2")); + ASSERT_FALSE(serviceContainer.has_error()); + ASSERT_THAT(serviceContainer.value().size(), Eq(1u)); + ASSERT_THAT(*serviceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance2", "event2"})); - instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance3")); - ASSERT_FALSE(instanceContainer.has_error()); - ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance3", "event3"})); + serviceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance3")); + ASSERT_FALSE(serviceContainer.has_error()); + ASSERT_THAT(serviceContainer.value().size(), Eq(1u)); + ASSERT_THAT(*serviceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance3", "event3"})); } TEST_F(RoudiFindService_test, OfferMultiMethodServiceMultiInstance) @@ -219,35 +219,35 @@ TEST_F(RoudiFindService_test, OfferMultiMethodServiceMultiInstance) EXPECT_TRUE(senderRuntime->offerService({"service2", "instance3", "event3"})); this->InterOpWait(); - auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); - ASSERT_FALSE(instanceContainer.has_error()); - ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); - - instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance2")); - ASSERT_FALSE(instanceContainer.has_error()); - ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance2", "event2"})); - - instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance3")); - ASSERT_FALSE(instanceContainer.has_error()); - ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance3", "event3"})); - - instanceContainer = receiverRuntime->findService(IdString_t("service2"), IdString_t("instance1")); - ASSERT_FALSE(instanceContainer.has_error()); - ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service2", "instance1", "event1"})); - - instanceContainer = receiverRuntime->findService(IdString_t("service2"), IdString_t("instance2")); - ASSERT_FALSE(instanceContainer.has_error()); - ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service2", "instance2", "event2"})); - - instanceContainer = receiverRuntime->findService(IdString_t("service2"), IdString_t("instance3")); - ASSERT_FALSE(instanceContainer.has_error()); - ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service2", "instance3", "event3"})); + auto serviceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); + ASSERT_FALSE(serviceContainer.has_error()); + ASSERT_THAT(serviceContainer.value().size(), Eq(1u)); + ASSERT_THAT(*serviceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); + + serviceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance2")); + ASSERT_FALSE(serviceContainer.has_error()); + ASSERT_THAT(serviceContainer.value().size(), Eq(1u)); + ASSERT_THAT(*serviceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance2", "event2"})); + + serviceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance3")); + ASSERT_FALSE(serviceContainer.has_error()); + ASSERT_THAT(serviceContainer.value().size(), Eq(1u)); + ASSERT_THAT(*serviceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance3", "event3"})); + + serviceContainer = receiverRuntime->findService(IdString_t("service2"), IdString_t("instance1")); + ASSERT_FALSE(serviceContainer.has_error()); + ASSERT_THAT(serviceContainer.value().size(), Eq(1u)); + ASSERT_THAT(*serviceContainer.value().begin(), Eq(ServiceDescription{"service2", "instance1", "event1"})); + + serviceContainer = receiverRuntime->findService(IdString_t("service2"), IdString_t("instance2")); + ASSERT_FALSE(serviceContainer.has_error()); + ASSERT_THAT(serviceContainer.value().size(), Eq(1u)); + ASSERT_THAT(*serviceContainer.value().begin(), Eq(ServiceDescription{"service2", "instance2", "event2"})); + + serviceContainer = receiverRuntime->findService(IdString_t("service2"), IdString_t("instance3")); + ASSERT_FALSE(serviceContainer.has_error()); + ASSERT_THAT(serviceContainer.value().size(), Eq(1u)); + ASSERT_THAT(*serviceContainer.value().begin(), Eq(ServiceDescription{"service2", "instance3", "event3"})); } TEST_F(RoudiFindService_test, StopOfferWithInvalidServiceDescriptionFails) @@ -263,9 +263,9 @@ TEST_F(RoudiFindService_test, StopOfferSingleMethodServiceSingleInstance) EXPECT_TRUE(senderRuntime->stopOfferService({"service1", "instance1", "event1"})); this->InterOpWait(); - auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); - ASSERT_FALSE(instanceContainer.has_error()); - ASSERT_THAT(instanceContainer.value().size(), Eq(0u)); + auto serviceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); + ASSERT_FALSE(serviceContainer.has_error()); + ASSERT_THAT(serviceContainer.value().size(), Eq(0u)); } TEST_F(RoudiFindService_test, StopOfferMultiMethodServiceSingleInstance) @@ -278,18 +278,18 @@ TEST_F(RoudiFindService_test, StopOfferMultiMethodServiceSingleInstance) EXPECT_TRUE(senderRuntime->stopOfferService({"service3", "instance1", "event1"})); this->InterOpWait(); - auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); - ASSERT_FALSE(instanceContainer.has_error()); - ASSERT_THAT(instanceContainer.value().size(), Eq(0u)); + auto serviceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); + ASSERT_FALSE(serviceContainer.has_error()); + ASSERT_THAT(serviceContainer.value().size(), Eq(0u)); - instanceContainer = receiverRuntime->findService(IdString_t("service2"), IdString_t("instance1")); - ASSERT_FALSE(instanceContainer.has_error()); - ASSERT_THAT(instanceContainer.value().size(), Eq(1u)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service2", "instance1", "event1"})); + serviceContainer = receiverRuntime->findService(IdString_t("service2"), IdString_t("instance1")); + ASSERT_FALSE(serviceContainer.has_error()); + ASSERT_THAT(serviceContainer.value().size(), Eq(1u)); + ASSERT_THAT(*serviceContainer.value().begin(), Eq(ServiceDescription{"service2", "instance1", "event1"})); - instanceContainer = receiverRuntime->findService(IdString_t("service3"), IdString_t("instance1")); - ASSERT_FALSE(instanceContainer.has_error()); - ASSERT_THAT(instanceContainer.value().size(), Eq(0u)); + serviceContainer = receiverRuntime->findService(IdString_t("service3"), IdString_t("instance1")); + ASSERT_FALSE(serviceContainer.has_error()); + ASSERT_THAT(serviceContainer.value().size(), Eq(0u)); } TEST_F(RoudiFindService_test, StopOfferServiceRedundantCall) @@ -301,9 +301,9 @@ TEST_F(RoudiFindService_test, StopOfferServiceRedundantCall) EXPECT_TRUE(senderRuntime->stopOfferService({"service1", "instance1", "event1"})); this->InterOpWait(); - auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); - ASSERT_FALSE(instanceContainer.has_error()); - ASSERT_THAT(instanceContainer.value().size(), Eq(0u)); + auto serviceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); + ASSERT_FALSE(serviceContainer.has_error()); + ASSERT_THAT(serviceContainer.value().size(), Eq(0u)); } @@ -314,10 +314,10 @@ TEST_F(RoudiFindService_test, StopNonExistingService) EXPECT_TRUE(senderRuntime->stopOfferService({"service2", "instance2", "event2"})); this->InterOpWait(); - auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); - ASSERT_FALSE(instanceContainer.has_error()); - ASSERT_THAT(instanceContainer.value().size(), Eq(1)); - ASSERT_THAT(*instanceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); + auto serviceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("instance1")); + ASSERT_FALSE(serviceContainer.has_error()); + ASSERT_THAT(serviceContainer.value().size(), Eq(1)); + ASSERT_THAT(*serviceContainer.value().begin(), Eq(ServiceDescription{"service1", "instance1", "event1"})); } TEST_F(RoudiFindService_test, FindNonExistingServices) @@ -327,17 +327,17 @@ TEST_F(RoudiFindService_test, FindNonExistingServices) EXPECT_TRUE(senderRuntime->offerService({"service3", "instance1", "event1"})); this->InterOpWait(); - auto instanceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("schlomo")); - ASSERT_FALSE(instanceContainer.has_error()); - ASSERT_THAT(instanceContainer.value().size(), Eq(0u)); + auto serviceContainer = receiverRuntime->findService(IdString_t("service1"), IdString_t("schlomo")); + ASSERT_FALSE(serviceContainer.has_error()); + ASSERT_THAT(serviceContainer.value().size(), Eq(0u)); - instanceContainer = receiverRuntime->findService(IdString_t("ignatz"), IdString_t("instance1")); - ASSERT_FALSE(instanceContainer.has_error()); - ASSERT_THAT(instanceContainer.value().size(), Eq(0u)); + serviceContainer = receiverRuntime->findService(IdString_t("ignatz"), IdString_t("instance1")); + ASSERT_FALSE(serviceContainer.has_error()); + ASSERT_THAT(serviceContainer.value().size(), Eq(0u)); - instanceContainer = receiverRuntime->findService(IdString_t("ignatz"), IdString_t("schlomo")); - ASSERT_FALSE(instanceContainer.has_error()); - ASSERT_THAT(instanceContainer.value().size(), Eq(0u)); + serviceContainer = receiverRuntime->findService(IdString_t("ignatz"), IdString_t("schlomo")); + ASSERT_FALSE(serviceContainer.has_error()); + ASSERT_THAT(serviceContainer.value().size(), Eq(0u)); } TEST_F(RoudiFindService_test, InterfacePort) @@ -367,39 +367,39 @@ TEST_F(RoudiFindService_test, InterfacePort) TEST_F(RoudiFindService_test, findServiceMaxServices) { - ServiceContainer instanceContainerExp; + ServiceContainer serviceContainerExp; for (size_t i = 0; i < iox::MAX_NUMBER_OF_SERVICES; i++) { // Service & Instance string is kept short , to reduce the response size in find service request , // (message queue has a limit of 512) std::string instance = "i" + iox::cxx::convert::toString(i); EXPECT_TRUE(senderRuntime->offerService({"s", IdString_t(iox::cxx::TruncateToCapacity, instance), "foo"})); - instanceContainerExp.push_back({"s", IdString_t(iox::cxx::TruncateToCapacity, instance), "foo"}); + serviceContainerExp.push_back({"s", IdString_t(iox::cxx::TruncateToCapacity, instance), "foo"}); this->InterOpWait(); } - auto instanceContainer = receiverRuntime->findService(IdString_t("s"), iox::runtime::Wildcard_t()); + auto serviceContainer = receiverRuntime->findService(IdString_t("s"), iox::runtime::Wildcard_t()); - ASSERT_FALSE(instanceContainer.has_error()); - EXPECT_THAT(instanceContainer.value().size(), Eq(iox::MAX_NUMBER_OF_SERVICES)); - EXPECT_TRUE(instanceContainer.value() == instanceContainerExp); + ASSERT_FALSE(serviceContainer.has_error()); + EXPECT_THAT(serviceContainer.value().size(), Eq(iox::MAX_NUMBER_OF_SERVICES)); + EXPECT_TRUE(serviceContainer.value() == serviceContainerExp); } -TEST_F(RoudiFindService_test, findServiceInstanceContainerOverflowError) +TEST_F(RoudiFindService_test, findServiceserviceContainerOverflowError) { size_t noOfInstances = (iox::MAX_NUMBER_OF_SERVICES + 1); - ServiceContainer instanceContainerExp; + ServiceContainer serviceContainerExp; for (size_t i = 0; i < noOfInstances; i++) { std::string instance = "i" + iox::cxx::convert::toString(i); EXPECT_TRUE(senderRuntime->offerService({"s", IdString_t(iox::cxx::TruncateToCapacity, instance), "foo"})); - instanceContainerExp.push_back({"s", IdString_t(iox::cxx::TruncateToCapacity, instance), "foo"}); + serviceContainerExp.push_back({"s", IdString_t(iox::cxx::TruncateToCapacity, instance), "foo"}); this->InterOpWait(); } - auto instanceContainer = receiverRuntime->findService(IdString_t("s"), iox::runtime::Wildcard_t()); + auto serviceContainer = receiverRuntime->findService(IdString_t("s"), iox::runtime::Wildcard_t()); - ASSERT_THAT(instanceContainer.has_error(), Eq(true)); + ASSERT_THAT(serviceContainer.has_error(), Eq(true)); } } // namespace From 6493ba53b75ab209a0fb7d4f5feee572e0455bc5 Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Thu, 1 Jul 2021 15:55:43 +0200 Subject: [PATCH 21/32] iox-#415 Reorder ServiceRegistry test cases Signed-off-by: Simon Hoinkis --- .../test_roudi_service_registry.cpp | 37 ++++++++++--------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp b/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp index bc4fab39b4..18581587c7 100644 --- a/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp +++ b/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp @@ -53,24 +53,7 @@ TEST_F(ServiceRegistry_test, AddNoServiceDescriptionsAndWildcardSearchReturnsNot EXPECT_THAT(searchResults.size(), Eq(0)); } -TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFoundWithWildcardSearch) -{ - auto result = registry.add(ServiceDescription("Foo", "Bar", "Baz")); - ASSERT_FALSE(result.has_error()); - registry.find(searchResults, Wildcard, Wildcard); - - EXPECT_THAT(searchResults.size(), Eq(1)); - EXPECT_THAT(searchResults[0], Eq(ServiceDescription("Foo", "Bar", "Baz"))); -} -TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFoundWithInstanceName) -{ - auto result = registry.add(ServiceDescription("Foo", "Bar", "Baz")); - ASSERT_FALSE(result.has_error()); - registry.find(searchResults, Wildcard, "Bar"); - EXPECT_THAT(searchResults.size(), Eq(1)); - EXPECT_THAT(searchResults[0], Eq(ServiceDescription("Foo", "Bar", "Baz"))); -} TEST_F(ServiceRegistry_test, AddMaximumNumberOfServiceDescriptionsWorks) { iox::cxx::vector services; @@ -136,6 +119,26 @@ TEST_F(ServiceRegistry_test, RemovingInvalidServiceDescriptionsFails) EXPECT_FALSE(registry.remove(ServiceDescription())); } +TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFoundWithWildcardSearch) +{ + auto result = registry.add(ServiceDescription("Foo", "Bar", "Baz")); + ASSERT_FALSE(result.has_error()); + registry.find(searchResults, Wildcard, Wildcard); + + EXPECT_THAT(searchResults.size(), Eq(1)); + EXPECT_THAT(searchResults[0], Eq(ServiceDescription("Foo", "Bar", "Baz"))); +} + +TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFoundWithInstanceName) +{ + auto result = registry.add(ServiceDescription("Baz", "Bar", "Foo")); + ASSERT_FALSE(result.has_error()); + registry.find(searchResults, Wildcard, "Bar"); + + EXPECT_THAT(searchResults.size(), Eq(1)); + EXPECT_THAT(searchResults[0], Eq(ServiceDescription("Baz", "Bar", "Foo"))); +} + TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFoundWithServiceName) { iox::capro::ServiceDescription service1("a", "b", "c"); From cb0848530314c22f100c9868f645438056385c2a Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Thu, 1 Jul 2021 16:10:16 +0200 Subject: [PATCH 22/32] iox-#415 Add preliminary changelog entry about new DiscoveryInfo API Signed-off-by: Simon Hoinkis --- CHANGELOG.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3d71902fde..0992de46eb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -105,6 +105,20 @@ ServiceDescription("First", "Second", "DontCare") myServiceDescription2; ServiceDescription("Foo", "Bar", "Baz") myServiceDescription3; ``` +The service-related methods have been move from `PoshRuntime` to a separate class (TBD): + +```cpp +// before +poshRuntime.offerService(myServiceDescription); +poshRuntime.stopOfferService(myServiceDescription); +poshRuntime.findService(myServiceDescription); + +// after +discoveryInfo.offerService(myServiceDescription); +discoveryInfo.stopOfferService(myServiceDescription); +discoveryInfo.findService("ServiceA", Wildcard_t); +``` + ## [v1.0.1](https://github.com/eclipse-iceoryx/iceoryx/tree/v1.0.0) (2021-06-15) [Full Changelog](https://github.com/eclipse-iceoryx/iceoryx/compare/v1.0.0...v1.0.1) From e2ed8c3f83ecb00ae3c22d7c4daa8a58b277117c Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Fri, 2 Jul 2021 10:59:56 +0200 Subject: [PATCH 23/32] iox-#415 Fix bug in ServiceRegistry::remove Signed-off-by: Simon Hoinkis --- .../source/roudi/service_registry.cpp | 4 +--- .../test_roudi_service_registry.cpp | 23 ++++++++++++++++++- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/iceoryx_posh/source/roudi/service_registry.cpp b/iceoryx_posh/source/roudi/service_registry.cpp index bd219731ed..2fc8be6720 100644 --- a/iceoryx_posh/source/roudi/service_registry.cpp +++ b/iceoryx_posh/source/roudi/service_registry.cpp @@ -71,16 +71,14 @@ bool ServiceRegistry::remove(const capro::ServiceDescription& serviceDescription } auto removeIndexFromMap = [](std::multimap& map, uint64_t index) { - bool removedEntry{false}; for (auto it = map.begin(); it != map.end();) { if (it->second == index) { it = map.erase(it); - removedEntry = true; continue; } - else if (removedEntry && it->second > index) + else if (it->second > index) { // update index due to removed element it->second--; diff --git a/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp b/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp index 18581587c7..0f0f7f7620 100644 --- a/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp +++ b/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp @@ -197,7 +197,7 @@ TEST_F(ServiceRegistry_test, MultipleServiceDescriptionWithDifferentServiceNameC EXPECT_THAT(searchResults[0], Eq(service2)); } -TEST_F(ServiceRegistry_test, MultipleServiceDescriptionWithSameServiceNameFindsSpecificInstance) +TEST_F(ServiceRegistry_test, MultipleServiceDescriptionWithSameServiceNameFindsSpecificService) { iox::capro::ServiceDescription service1("a", "b", "b"); iox::capro::ServiceDescription service2("a", "c", "c"); @@ -212,6 +212,27 @@ TEST_F(ServiceRegistry_test, MultipleServiceDescriptionWithSameServiceNameFindsS EXPECT_THAT(searchResults[0], Eq(service2)); } +TEST_F(ServiceRegistry_test, MultipleServiceDescriptionAddedInNonLinearOrderFindsCorrectServices) +{ + iox::capro::ServiceDescription service1("a", "1", "moep"); + iox::capro::ServiceDescription service2("b", "2", "moep"); + iox::capro::ServiceDescription service3("c", "3", "moep"); + iox::capro::ServiceDescription service4("d", "4", "moep"); + iox::capro::ServiceDescription service5("e", "5", "moep"); + + ASSERT_FALSE(registry.add(service5).has_error()); + ASSERT_FALSE(registry.add(service3).has_error()); + ASSERT_FALSE(registry.add(service4).has_error()); + ASSERT_FALSE(registry.add(service2).has_error()); + ASSERT_FALSE(registry.add(service1).has_error()); + + ASSERT_TRUE(registry.remove(service5)); + ASSERT_TRUE(registry.remove(service1)); + registry.find(searchResults, "a", Wildcard); + + EXPECT_THAT(searchResults.size(), Eq(0)); +} + TEST_F(ServiceRegistry_test, FindSpecificNonExistingServiceDescriptionFails) { iox::capro::ServiceDescription service1("a", "b", "b"); From b7e7e9c93ec01b1a0aa783582de10d8bdd6f022a Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Fri, 30 Jul 2021 11:33:23 +0200 Subject: [PATCH 24/32] iox-#415 Update example in changelog Signed-off-by: Simon Hoinkis --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0992de46eb..a334b7129d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -111,12 +111,12 @@ The service-related methods have been move from `PoshRuntime` to a separate clas // before poshRuntime.offerService(myServiceDescription); poshRuntime.stopOfferService(myServiceDescription); -poshRuntime.findService(myServiceDescription); +poshRuntime.findService({"ServiceA", iox::capro::AnyInstanceString}); // after discoveryInfo.offerService(myServiceDescription); discoveryInfo.stopOfferService(myServiceDescription); -discoveryInfo.findService("ServiceA", Wildcard_t); +discoveryInfo.findService("ServiceA", Wildcard); ``` ## [v1.0.1](https://github.com/eclipse-iceoryx/iceoryx/tree/v1.0.0) (2021-06-15) From 93d5a41e7280dda28c20426d0189ebb85ea99f8e Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Tue, 3 Aug 2021 14:50:09 +0200 Subject: [PATCH 25/32] iox-#415 Revert changes in ServiceDescription::operator==() and allow invalid ServiceDescription's being store in ServiceRegistry Signed-off-by: Simon Hoinkis --- .../internal/roudi/service_registry.hpp | 1 - .../iceoryx_posh/runtime/posh_runtime.hpp | 2 +- .../source/capro/service_description.cpp | 5 --- .../source/roudi/service_registry.cpp | 10 ----- .../test/moduletests/test_base_port.cpp | 6 +-- .../test_roudi_service_registry.cpp | 42 ++++++++++++++++--- 6 files changed, 41 insertions(+), 25 deletions(-) diff --git a/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp b/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp index c6d9645dcc..8e684b7587 100644 --- a/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp +++ b/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp @@ -39,7 +39,6 @@ class ServiceRegistry INVALID_STATE, SERVICE_DESCRIPTION_ALREADY_ADDED, SERVICE_REGISTRY_FULL, - SERVICE_DESCRIPTION_INVALID, }; /// @todo #415 should be connected with iox::MAX_NUMBER_OF_SERVICES static constexpr uint32_t MAX_SERVICE_DESCRIPTIONS = 100U; diff --git a/iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp b/iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp index 57bad38f61..91e50d3088 100644 --- a/iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp +++ b/iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp @@ -48,7 +48,7 @@ enum class FindServiceError { INVALID_STATE, UNABLE_TO_WRITE_TO_ROUDI_CHANNEL, /// @todo #415 remove as IPC channel won't be used - INSTANCE_CONTAINER_OVERFLOW /// @todo #415 set container to iox::MAX_NUMBER_OF_SERVICES and remove error + INSTANCE_CONTAINER_OVERFLOW /// @todo #415 set container to iox::MAX_NUMBER_OF_SERVICES and remove error }; /// @brief Used to search for any string diff --git a/iceoryx_posh/source/capro/service_description.cpp b/iceoryx_posh/source/capro/service_description.cpp index dccbe68bd2..78c82953e7 100644 --- a/iceoryx_posh/source/capro/service_description.cpp +++ b/iceoryx_posh/source/capro/service_description.cpp @@ -121,11 +121,6 @@ ServiceDescription::ServiceDescription(const IdString_t& service, bool ServiceDescription::operator==(const ServiceDescription& rhs) const { - if (!isValid() || !rhs.isValid()) - { - return false; - } - if (m_serviceString != rhs.m_serviceString) { return false; diff --git a/iceoryx_posh/source/roudi/service_registry.cpp b/iceoryx_posh/source/roudi/service_registry.cpp index 2fc8be6720..5c45ddd1ff 100644 --- a/iceoryx_posh/source/roudi/service_registry.cpp +++ b/iceoryx_posh/source/roudi/service_registry.cpp @@ -25,11 +25,6 @@ namespace roudi { cxx::expected ServiceRegistry::add(const capro::ServiceDescription& serviceDescription) noexcept { - if (!serviceDescription.isValid()) - { - return cxx::error(Error::SERVICE_DESCRIPTION_INVALID); - } - // Forbid duplicate service descriptions entries for (auto& element : m_serviceDescriptionVector) { @@ -50,11 +45,6 @@ cxx::expected ServiceRegistry::add(const capro::ServiceD bool ServiceRegistry::remove(const capro::ServiceDescription& serviceDescription) noexcept { - if (!serviceDescription.isValid()) - { - return false; - } - bool removedElement{false}; uint64_t index = 0U; for (auto iterator = m_serviceDescriptionVector.begin(); iterator != m_serviceDescriptionVector.end();) diff --git a/iceoryx_posh/test/moduletests/test_base_port.cpp b/iceoryx_posh/test/moduletests/test_base_port.cpp index 946b85c9b9..2f1f2cd105 100644 --- a/iceoryx_posh/test/moduletests/test_base_port.cpp +++ b/iceoryx_posh/test/moduletests/test_base_port.cpp @@ -104,12 +104,12 @@ const ServiceDescription& expectedServiceDescription() template <> const ServiceDescription& expectedServiceDescription() { - return SERVICE_DESCRIPTION_EMPTY; + return SERVICE_DESCRIPTION_VALID; } template <> const ServiceDescription& expectedServiceDescription() { - return SERVICE_DESCRIPTION_EMPTY; + return SERVICE_DESCRIPTION_VALID; } // expected ProcessName factories @@ -169,7 +169,7 @@ class BasePort_test : public Test TYPED_TEST(BasePort_test, CallingGetCaProServiceDescriptionWorks) { using PortData_t = typename TestFixture::PortData_t; - EXPECT_THAT(this->sut.getCaProServiceDescription(), Ne(expectedServiceDescription())); + EXPECT_THAT(this->sut.getCaProServiceDescription(), Eq(expectedServiceDescription())); } TYPED_TEST(BasePort_test, CallingGetRuntimeNameWorks) diff --git a/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp b/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp index 0f0f7f7620..3f6ef8c086 100644 --- a/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp +++ b/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp @@ -102,11 +102,10 @@ TEST_F(ServiceRegistry_test, AddServiceDescriptionsWhichWasAlreadyAddedDoesNotWo EXPECT_THAT(result2.get_error(), Eq(ServiceRegistry::Error::SERVICE_DESCRIPTION_ALREADY_ADDED)); } -TEST_F(ServiceRegistry_test, AddInvalidServiceDescriptionsFails) +TEST_F(ServiceRegistry_test, AddInvalidServiceDescriptionsWorks) { auto result = registry.add(ServiceDescription()); - ASSERT_TRUE(result.has_error()); - EXPECT_THAT(result.get_error(), Eq(ServiceRegistry::Error::SERVICE_DESCRIPTION_INVALID)); + ASSERT_FALSE(result.has_error()); } TEST_F(ServiceRegistry_test, RemovingServiceDescriptionsWhichWasntAddedFails) @@ -114,9 +113,28 @@ TEST_F(ServiceRegistry_test, RemovingServiceDescriptionsWhichWasntAddedFails) EXPECT_FALSE(registry.remove(ServiceDescription("Sim", "Sa", "Lambim"))); } -TEST_F(ServiceRegistry_test, RemovingInvalidServiceDescriptionsFails) +TEST_F(ServiceRegistry_test, RemovingInvalidServiceDescriptionsWorks) +{ + ASSERT_FALSE(registry.add(ServiceDescription()).has_error()); + EXPECT_TRUE(registry.remove(ServiceDescription())); +} + +TEST_F(ServiceRegistry_test, SingleInvalidServiceDescriptionsCanBeFoundWithWildcardSearch) +{ + ASSERT_FALSE(registry.add(ServiceDescription()).has_error()); + registry.find(searchResults, Wildcard, Wildcard); + + EXPECT_THAT(searchResults.size(), Eq(1)); + EXPECT_THAT(searchResults[0], Eq(ServiceDescription())); +} + +TEST_F(ServiceRegistry_test, SingleInvalidServiceDescriptionsCanBeFoundWithEmptyString) { - EXPECT_FALSE(registry.remove(ServiceDescription())); + ASSERT_FALSE(registry.add(ServiceDescription()).has_error()); + registry.find(searchResults, "", ""); + + EXPECT_THAT(searchResults.size(), Eq(1)); + EXPECT_THAT(searchResults[0], Eq(ServiceDescription())); } TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFoundWithWildcardSearch) @@ -149,6 +167,20 @@ TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFoundWithServiceName) EXPECT_THAT(searchResults[0], Eq(service1)); } +TEST_F(ServiceRegistry_test, ValidAndInvalidServiceDescriptionsCanAllBeFoundWithWildcardSearch) +{ + ServiceDescription service1; + ServiceDescription service2("alpha", "bravo", "charlie"); + + ASSERT_FALSE(registry.add(service1).has_error()); + ASSERT_FALSE(registry.add(service2).has_error()); + registry.find(searchResults, Wildcard, Wildcard); + + EXPECT_THAT(searchResults.size(), Eq(2)); + EXPECT_THAT(searchResults[0], Eq(service1)); + EXPECT_THAT(searchResults[1], Eq(service2)); +} + TEST_F(ServiceRegistry_test, MultipleServiceDescriptionWithSameServiceNameCanAllBeFound) { iox::capro::ServiceDescription service1("a", "b", "b"); From 27271df5107273de2409c7cd79ae0b4c88f59dc6 Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Tue, 3 Aug 2021 15:41:40 +0200 Subject: [PATCH 26/32] iox-#415 Remove Error::SERVICE_DESCRIPTION_ALREADY_ADDED Signed-off-by: Simon Hoinkis --- .../internal/roudi/service_registry.hpp | 1 - iceoryx_posh/source/roudi/service_registry.cpp | 3 ++- .../moduletests/test_roudi_service_registry.cpp | 14 ++++++++------ 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp b/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp index 8e684b7587..7abb99f136 100644 --- a/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp +++ b/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp @@ -37,7 +37,6 @@ class ServiceRegistry enum class Error { INVALID_STATE, - SERVICE_DESCRIPTION_ALREADY_ADDED, SERVICE_REGISTRY_FULL, }; /// @todo #415 should be connected with iox::MAX_NUMBER_OF_SERVICES diff --git a/iceoryx_posh/source/roudi/service_registry.cpp b/iceoryx_posh/source/roudi/service_registry.cpp index 5c45ddd1ff..d3b7b28cf0 100644 --- a/iceoryx_posh/source/roudi/service_registry.cpp +++ b/iceoryx_posh/source/roudi/service_registry.cpp @@ -30,7 +30,8 @@ cxx::expected ServiceRegistry::add(const capro::ServiceD { if (element == serviceDescription) { - return cxx::error(Error::SERVICE_DESCRIPTION_ALREADY_ADDED); + // Due to n:m communication we don't store twice but return success + return cxx::success<>(); } } diff --git a/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp b/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp index 3f6ef8c086..60da514220 100644 --- a/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp +++ b/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp @@ -92,14 +92,18 @@ TEST_F(ServiceRegistry_test, AddMoreThanMaximumNumberOfServiceDescriptionsFails) EXPECT_THAT(result.get_error(), Eq(ServiceRegistry::Error::SERVICE_REGISTRY_FULL)); } -TEST_F(ServiceRegistry_test, AddServiceDescriptionsWhichWasAlreadyAddedDoesNotWork) +TEST_F(ServiceRegistry_test, AddServiceDescriptionsWhichWasAlreadyAddedAndReturnsOneResult) { auto result1 = registry.add(ServiceDescription("Li", "La", "Launebaer")); ASSERT_FALSE(result1.has_error()); auto result2 = registry.add(ServiceDescription("Li", "La", "Launebaer")); - ASSERT_TRUE(result2.has_error()); - EXPECT_THAT(result2.get_error(), Eq(ServiceRegistry::Error::SERVICE_DESCRIPTION_ALREADY_ADDED)); + ASSERT_FALSE(result2.has_error()); + + registry.find(searchResults, Wildcard, Wildcard); + + EXPECT_THAT(searchResults.size(), Eq(1)); + EXPECT_THAT(searchResults[0], Eq(ServiceDescription("Li", "La", "Launebaer"))); } TEST_F(ServiceRegistry_test, AddInvalidServiceDescriptionsWorks) @@ -340,9 +344,7 @@ TEST_F(ServiceRegistry_test, AddingVariousServiceDescriptionAndGetServicesDoesNo ASSERT_FALSE(registry.add(service1).has_error()); // add same service a, instance c to check if in registry only one entry is created ASSERT_FALSE(registry.add(service2).has_error()); - auto result = registry.add(service2); - ASSERT_TRUE(result.has_error()); - EXPECT_THAT(result.get_error(), Eq(iox::roudi::ServiceRegistry::Error::SERVICE_DESCRIPTION_ALREADY_ADDED)); + ASSERT_FALSE(registry.add(service2).has_error()); ASSERT_FALSE(registry.add(service3).has_error()); ASSERT_FALSE(registry.add(service4).has_error()); From 43a4f7f48dbb3f511065ca1bbb01e3f75b20e28b Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Tue, 3 Aug 2021 15:44:15 +0200 Subject: [PATCH 27/32] iox-#415 Create temp variable serviceIndex Signed-off-by: Simon Hoinkis --- iceoryx_posh/source/roudi/service_registry.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/iceoryx_posh/source/roudi/service_registry.cpp b/iceoryx_posh/source/roudi/service_registry.cpp index d3b7b28cf0..d878bc662e 100644 --- a/iceoryx_posh/source/roudi/service_registry.cpp +++ b/iceoryx_posh/source/roudi/service_registry.cpp @@ -39,8 +39,10 @@ cxx::expected ServiceRegistry::add(const capro::ServiceD { return cxx::error(Error::SERVICE_REGISTRY_FULL); } - m_serviceMap.insert({serviceDescription.getServiceIDString(), m_serviceDescriptionVector.size() - 1}); - m_instanceMap.insert({serviceDescription.getInstanceIDString(), m_serviceDescriptionVector.size() - 1}); + + auto serviceIndex = m_serviceDescriptionVector.size() - 1; + m_serviceMap.insert({serviceDescription.getServiceIDString(), serviceIndex}); + m_instanceMap.insert({serviceDescription.getInstanceIDString(), serviceIndex}); return cxx::success<>(); } From f862b443fdede941e63079bd6559e48508a17d50 Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Wed, 4 Aug 2021 16:22:29 +0200 Subject: [PATCH 28/32] iox-#415 Take the reference counter into account when removing a ServiceDescription from the ServiceRegistry and add related test cases Signed-off-by: Simon Hoinkis --- .../internal/roudi/service_registry.hpp | 5 +- iceoryx_posh/source/roudi/port_manager.cpp | 4 +- .../source/roudi/service_registry.cpp | 27 +++++++--- .../test_roudi_service_registry.cpp | 52 +++++++++++++------ 4 files changed, 60 insertions(+), 28 deletions(-) diff --git a/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp b/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp index 7abb99f136..c8553c40b1 100644 --- a/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp +++ b/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp @@ -19,6 +19,7 @@ #include "iceoryx_hoofs/cxx/expected.hpp" #include "iceoryx_hoofs/cxx/vector.hpp" +#include "iceoryx_hoofs/internal/cxx/pair.hpp" #include "iceoryx_hoofs/internal/cxx/set.hpp" #include "iceoryx_posh/capro/service_description.hpp" @@ -41,7 +42,9 @@ class ServiceRegistry }; /// @todo #415 should be connected with iox::MAX_NUMBER_OF_SERVICES static constexpr uint32_t MAX_SERVICE_DESCRIPTIONS = 100U; - using ServiceDescriptionVector_t = cxx::vector; + using ReferenceCounter_t = uint64_t; + using ServiceDescriptionVector_t = + cxx::vector, MAX_SERVICE_DESCRIPTIONS>; /// @brief Adds given service description to registry /// @param[in] serviceDescription, service to be added diff --git a/iceoryx_posh/source/roudi/port_manager.cpp b/iceoryx_posh/source/roudi/port_manager.cpp index 0f256e2de8..090d7076af 100644 --- a/iceoryx_posh/source/roudi/port_manager.cpp +++ b/iceoryx_posh/source/roudi/port_manager.cpp @@ -274,7 +274,7 @@ void PortManager::handleInterfaces() noexcept for (auto const& element : serviceVector) { - caproMessage.m_serviceDescription = element; + caproMessage.m_serviceDescription = element.first; for (auto& interfacePortData : interfacePortsForInitialForwarding) { @@ -599,7 +599,7 @@ runtime::IpcMessage PortManager::findService(const capro::IdString_t& service, m_serviceRegistry.find(searchResult, service, instance); for (auto& service : searchResult) { - response << static_cast(service).toString(); + response << static_cast(service.first).toString(); } return response; diff --git a/iceoryx_posh/source/roudi/service_registry.cpp b/iceoryx_posh/source/roudi/service_registry.cpp index d878bc662e..66d33006da 100644 --- a/iceoryx_posh/source/roudi/service_registry.cpp +++ b/iceoryx_posh/source/roudi/service_registry.cpp @@ -28,14 +28,15 @@ cxx::expected ServiceRegistry::add(const capro::ServiceD // Forbid duplicate service descriptions entries for (auto& element : m_serviceDescriptionVector) { - if (element == serviceDescription) + if (element.first == serviceDescription) { - // Due to n:m communication we don't store twice but return success + // Due to n:m communication we don't store twice but increase the reference counter + element.second++; return cxx::success<>(); } } - - if (!m_serviceDescriptionVector.push_back(serviceDescription)) + uint64_t referenceCounter = 1; + if (!m_serviceDescriptionVector.push_back({serviceDescription, referenceCounter})) { return cxx::error(Error::SERVICE_REGISTRY_FULL); } @@ -52,11 +53,21 @@ bool ServiceRegistry::remove(const capro::ServiceDescription& serviceDescription uint64_t index = 0U; for (auto iterator = m_serviceDescriptionVector.begin(); iterator != m_serviceDescriptionVector.end();) { - if (m_serviceDescriptionVector[index] == serviceDescription) + auto& element = m_serviceDescriptionVector[index].first; + if (element == serviceDescription) { - m_serviceDescriptionVector.erase(iterator); - removedElement = true; - // There can be not more than one element + auto& refCounter = m_serviceDescriptionVector[index].second; + refCounter--; + if (refCounter == 0) + { + m_serviceDescriptionVector.erase(iterator); + removedElement = true; + } + else + { + return true; + } + // There can't be more than one element break; } index++; diff --git a/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp b/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp index 60da514220..405cdf0e4f 100644 --- a/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp +++ b/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp @@ -103,7 +103,25 @@ TEST_F(ServiceRegistry_test, AddServiceDescriptionsWhichWasAlreadyAddedAndReturn registry.find(searchResults, Wildcard, Wildcard); EXPECT_THAT(searchResults.size(), Eq(1)); - EXPECT_THAT(searchResults[0], Eq(ServiceDescription("Li", "La", "Launebaer"))); + EXPECT_THAT(searchResults[0].first, Eq(ServiceDescription("Li", "La", "Launebaer"))); + EXPECT_THAT(searchResults[0].second, Eq(2)); +} + +TEST_F(ServiceRegistry_test, AddServiceDescriptionsTwiceAndRemoveOnceAndReturnsOneResult) +{ + auto result1 = registry.add(ServiceDescription("Li", "La", "Launebaerli")); + ASSERT_FALSE(result1.has_error()); + + auto result2 = registry.add(ServiceDescription("Li", "La", "Launebaerli")); + ASSERT_FALSE(result2.has_error()); + + registry.remove(ServiceDescription("Li", "La", "Launebaerli")); + + registry.find(searchResults, Wildcard, Wildcard); + + EXPECT_THAT(searchResults.size(), Eq(1)); + EXPECT_THAT(searchResults[0].first, Eq(ServiceDescription("Li", "La", "Launebaerli"))); + EXPECT_THAT(searchResults[0].second, Eq(1)); } TEST_F(ServiceRegistry_test, AddInvalidServiceDescriptionsWorks) @@ -129,7 +147,7 @@ TEST_F(ServiceRegistry_test, SingleInvalidServiceDescriptionsCanBeFoundWithWildc registry.find(searchResults, Wildcard, Wildcard); EXPECT_THAT(searchResults.size(), Eq(1)); - EXPECT_THAT(searchResults[0], Eq(ServiceDescription())); + EXPECT_THAT(searchResults[0].first, Eq(ServiceDescription())); } TEST_F(ServiceRegistry_test, SingleInvalidServiceDescriptionsCanBeFoundWithEmptyString) @@ -138,7 +156,7 @@ TEST_F(ServiceRegistry_test, SingleInvalidServiceDescriptionsCanBeFoundWithEmpty registry.find(searchResults, "", ""); EXPECT_THAT(searchResults.size(), Eq(1)); - EXPECT_THAT(searchResults[0], Eq(ServiceDescription())); + EXPECT_THAT(searchResults[0].first, Eq(ServiceDescription())); } TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFoundWithWildcardSearch) @@ -148,7 +166,7 @@ TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFoundWithWildcardSearc registry.find(searchResults, Wildcard, Wildcard); EXPECT_THAT(searchResults.size(), Eq(1)); - EXPECT_THAT(searchResults[0], Eq(ServiceDescription("Foo", "Bar", "Baz"))); + EXPECT_THAT(searchResults[0].first, Eq(ServiceDescription("Foo", "Bar", "Baz"))); } TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFoundWithInstanceName) @@ -158,7 +176,7 @@ TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFoundWithInstanceName) registry.find(searchResults, Wildcard, "Bar"); EXPECT_THAT(searchResults.size(), Eq(1)); - EXPECT_THAT(searchResults[0], Eq(ServiceDescription("Baz", "Bar", "Foo"))); + EXPECT_THAT(searchResults[0].first, Eq(ServiceDescription("Baz", "Bar", "Foo"))); } TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFoundWithServiceName) @@ -168,7 +186,7 @@ TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFoundWithServiceName) registry.find(searchResults, "a", Wildcard); EXPECT_THAT(searchResults.size(), Eq(1)); - EXPECT_THAT(searchResults[0], Eq(service1)); + EXPECT_THAT(searchResults[0].first, Eq(service1)); } TEST_F(ServiceRegistry_test, ValidAndInvalidServiceDescriptionsCanAllBeFoundWithWildcardSearch) @@ -181,8 +199,8 @@ TEST_F(ServiceRegistry_test, ValidAndInvalidServiceDescriptionsCanAllBeFoundWith registry.find(searchResults, Wildcard, Wildcard); EXPECT_THAT(searchResults.size(), Eq(2)); - EXPECT_THAT(searchResults[0], Eq(service1)); - EXPECT_THAT(searchResults[1], Eq(service2)); + EXPECT_THAT(searchResults[0].first, Eq(service1)); + EXPECT_THAT(searchResults[1].first, Eq(service2)); } TEST_F(ServiceRegistry_test, MultipleServiceDescriptionWithSameServiceNameCanAllBeFound) @@ -204,11 +222,11 @@ TEST_F(ServiceRegistry_test, MultipleServiceDescriptionWithSameServiceNameCanAll for (auto& e : searchResults) { - if (e == service1) + if (e.first == service1) hasFoundB = true; - if (e == service2) + if (e.first == service2) hasFoundC = true; - if (e == service3) + if (e.first == service3) hasFoundD = true; } @@ -225,12 +243,12 @@ TEST_F(ServiceRegistry_test, MultipleServiceDescriptionWithDifferentServiceNameC registry.find(searchResults, "a", Wildcard); EXPECT_THAT(searchResults.size(), Eq(1)); - EXPECT_THAT(searchResults[0], Eq(service1)); + EXPECT_THAT(searchResults[0].first, Eq(service1)); searchResults.clear(); registry.find(searchResults, "c", Wildcard); EXPECT_THAT(searchResults.size(), Eq(1)); - EXPECT_THAT(searchResults[0], Eq(service2)); + EXPECT_THAT(searchResults[0].first, Eq(service2)); } TEST_F(ServiceRegistry_test, MultipleServiceDescriptionWithSameServiceNameFindsSpecificService) @@ -245,7 +263,7 @@ TEST_F(ServiceRegistry_test, MultipleServiceDescriptionWithSameServiceNameFindsS registry.find(searchResults, "a", "c"); EXPECT_THAT(searchResults.size(), Eq(1)); - EXPECT_THAT(searchResults[0], Eq(service2)); + EXPECT_THAT(searchResults[0].first, Eq(service2)); } TEST_F(ServiceRegistry_test, MultipleServiceDescriptionAddedInNonLinearOrderFindsCorrectServices) @@ -356,17 +374,17 @@ TEST_F(ServiceRegistry_test, AddingVariousServiceDescriptionAndGetServicesDoesNo for (auto const& element : serviceDescriptionVector) { - if (element == service1) + if (element.first == service1) { service1Found = true; } - if (element == service2) + if (element.first == service2) { service2Found = true; } - if (element == service4) + if (element.first == service4) { service4Found = true; } From f8b3ff6509766124cb612e00b480fa7804cd7628 Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Mon, 9 Aug 2021 10:53:25 +0200 Subject: [PATCH 29/32] iox-#415 Use ServiceDescriptionEntry instead of cxx::pair Signed-off-by: Simon Hoinkis --- CHANGELOG.md | 2 +- .../internal/roudi/service_registry.hpp | 13 ++++-- iceoryx_posh/source/roudi/port_manager.cpp | 4 +- .../source/roudi/service_registry.cpp | 8 ++-- .../test_roudi_service_registry.cpp | 40 +++++++++---------- 5 files changed, 36 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e82c63fc44..4dbeb8a3d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -108,7 +108,7 @@ ServiceDescription("First", "Second", "DontCare") myServiceDescription2; ServiceDescription("Foo", "Bar", "Baz") myServiceDescription3; ``` -The service-related methods have been move from `PoshRuntime` to a separate class (TBD): +The service-related methods have been moved from `PoshRuntime` to a separate class (TBD): ```cpp // before diff --git a/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp b/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp index c8553c40b1..f7188db18b 100644 --- a/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp +++ b/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp @@ -19,8 +19,6 @@ #include "iceoryx_hoofs/cxx/expected.hpp" #include "iceoryx_hoofs/cxx/vector.hpp" -#include "iceoryx_hoofs/internal/cxx/pair.hpp" -#include "iceoryx_hoofs/internal/cxx/set.hpp" #include "iceoryx_posh/capro/service_description.hpp" #include @@ -40,11 +38,18 @@ class ServiceRegistry INVALID_STATE, SERVICE_REGISTRY_FULL, }; + + using ReferenceCounter_t = uint64_t; + struct ServiceDescriptionEntry + { + capro::ServiceDescription serviceDescription{}; + ReferenceCounter_t referenceCounter = 0U; + }; + /// @todo #415 should be connected with iox::MAX_NUMBER_OF_SERVICES static constexpr uint32_t MAX_SERVICE_DESCRIPTIONS = 100U; - using ReferenceCounter_t = uint64_t; using ServiceDescriptionVector_t = - cxx::vector, MAX_SERVICE_DESCRIPTIONS>; + cxx::vector; /// @brief Adds given service description to registry /// @param[in] serviceDescription, service to be added diff --git a/iceoryx_posh/source/roudi/port_manager.cpp b/iceoryx_posh/source/roudi/port_manager.cpp index 090d7076af..16ecdef6a9 100644 --- a/iceoryx_posh/source/roudi/port_manager.cpp +++ b/iceoryx_posh/source/roudi/port_manager.cpp @@ -274,7 +274,7 @@ void PortManager::handleInterfaces() noexcept for (auto const& element : serviceVector) { - caproMessage.m_serviceDescription = element.first; + caproMessage.m_serviceDescription = element.serviceDescription; for (auto& interfacePortData : interfacePortsForInitialForwarding) { @@ -599,7 +599,7 @@ runtime::IpcMessage PortManager::findService(const capro::IdString_t& service, m_serviceRegistry.find(searchResult, service, instance); for (auto& service : searchResult) { - response << static_cast(service.first).toString(); + response << static_cast(service.serviceDescription).toString(); } return response; diff --git a/iceoryx_posh/source/roudi/service_registry.cpp b/iceoryx_posh/source/roudi/service_registry.cpp index 66d33006da..66bc78f09e 100644 --- a/iceoryx_posh/source/roudi/service_registry.cpp +++ b/iceoryx_posh/source/roudi/service_registry.cpp @@ -28,10 +28,10 @@ cxx::expected ServiceRegistry::add(const capro::ServiceD // Forbid duplicate service descriptions entries for (auto& element : m_serviceDescriptionVector) { - if (element.first == serviceDescription) + if (element.serviceDescription == serviceDescription) { // Due to n:m communication we don't store twice but increase the reference counter - element.second++; + element.referenceCounter++; return cxx::success<>(); } } @@ -53,10 +53,10 @@ bool ServiceRegistry::remove(const capro::ServiceDescription& serviceDescription uint64_t index = 0U; for (auto iterator = m_serviceDescriptionVector.begin(); iterator != m_serviceDescriptionVector.end();) { - auto& element = m_serviceDescriptionVector[index].first; + auto& element = m_serviceDescriptionVector[index].serviceDescription; if (element == serviceDescription) { - auto& refCounter = m_serviceDescriptionVector[index].second; + auto& refCounter = m_serviceDescriptionVector[index].referenceCounter; refCounter--; if (refCounter == 0) { diff --git a/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp b/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp index 405cdf0e4f..ed640d0b54 100644 --- a/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp +++ b/iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp @@ -103,8 +103,8 @@ TEST_F(ServiceRegistry_test, AddServiceDescriptionsWhichWasAlreadyAddedAndReturn registry.find(searchResults, Wildcard, Wildcard); EXPECT_THAT(searchResults.size(), Eq(1)); - EXPECT_THAT(searchResults[0].first, Eq(ServiceDescription("Li", "La", "Launebaer"))); - EXPECT_THAT(searchResults[0].second, Eq(2)); + EXPECT_THAT(searchResults[0].serviceDescription, Eq(ServiceDescription("Li", "La", "Launebaer"))); + EXPECT_THAT(searchResults[0].referenceCounter, Eq(2)); } TEST_F(ServiceRegistry_test, AddServiceDescriptionsTwiceAndRemoveOnceAndReturnsOneResult) @@ -120,8 +120,8 @@ TEST_F(ServiceRegistry_test, AddServiceDescriptionsTwiceAndRemoveOnceAndReturnsO registry.find(searchResults, Wildcard, Wildcard); EXPECT_THAT(searchResults.size(), Eq(1)); - EXPECT_THAT(searchResults[0].first, Eq(ServiceDescription("Li", "La", "Launebaerli"))); - EXPECT_THAT(searchResults[0].second, Eq(1)); + EXPECT_THAT(searchResults[0].serviceDescription, Eq(ServiceDescription("Li", "La", "Launebaerli"))); + EXPECT_THAT(searchResults[0].referenceCounter, Eq(1)); } TEST_F(ServiceRegistry_test, AddInvalidServiceDescriptionsWorks) @@ -147,7 +147,7 @@ TEST_F(ServiceRegistry_test, SingleInvalidServiceDescriptionsCanBeFoundWithWildc registry.find(searchResults, Wildcard, Wildcard); EXPECT_THAT(searchResults.size(), Eq(1)); - EXPECT_THAT(searchResults[0].first, Eq(ServiceDescription())); + EXPECT_THAT(searchResults[0].serviceDescription, Eq(ServiceDescription())); } TEST_F(ServiceRegistry_test, SingleInvalidServiceDescriptionsCanBeFoundWithEmptyString) @@ -156,7 +156,7 @@ TEST_F(ServiceRegistry_test, SingleInvalidServiceDescriptionsCanBeFoundWithEmpty registry.find(searchResults, "", ""); EXPECT_THAT(searchResults.size(), Eq(1)); - EXPECT_THAT(searchResults[0].first, Eq(ServiceDescription())); + EXPECT_THAT(searchResults[0].serviceDescription, Eq(ServiceDescription())); } TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFoundWithWildcardSearch) @@ -166,7 +166,7 @@ TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFoundWithWildcardSearc registry.find(searchResults, Wildcard, Wildcard); EXPECT_THAT(searchResults.size(), Eq(1)); - EXPECT_THAT(searchResults[0].first, Eq(ServiceDescription("Foo", "Bar", "Baz"))); + EXPECT_THAT(searchResults[0].serviceDescription, Eq(ServiceDescription("Foo", "Bar", "Baz"))); } TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFoundWithInstanceName) @@ -176,7 +176,7 @@ TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFoundWithInstanceName) registry.find(searchResults, Wildcard, "Bar"); EXPECT_THAT(searchResults.size(), Eq(1)); - EXPECT_THAT(searchResults[0].first, Eq(ServiceDescription("Baz", "Bar", "Foo"))); + EXPECT_THAT(searchResults[0].serviceDescription, Eq(ServiceDescription("Baz", "Bar", "Foo"))); } TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFoundWithServiceName) @@ -186,7 +186,7 @@ TEST_F(ServiceRegistry_test, SingleServiceDescriptionCanBeFoundWithServiceName) registry.find(searchResults, "a", Wildcard); EXPECT_THAT(searchResults.size(), Eq(1)); - EXPECT_THAT(searchResults[0].first, Eq(service1)); + EXPECT_THAT(searchResults[0].serviceDescription, Eq(service1)); } TEST_F(ServiceRegistry_test, ValidAndInvalidServiceDescriptionsCanAllBeFoundWithWildcardSearch) @@ -199,8 +199,8 @@ TEST_F(ServiceRegistry_test, ValidAndInvalidServiceDescriptionsCanAllBeFoundWith registry.find(searchResults, Wildcard, Wildcard); EXPECT_THAT(searchResults.size(), Eq(2)); - EXPECT_THAT(searchResults[0].first, Eq(service1)); - EXPECT_THAT(searchResults[1].first, Eq(service2)); + EXPECT_THAT(searchResults[0].serviceDescription, Eq(service1)); + EXPECT_THAT(searchResults[1].serviceDescription, Eq(service2)); } TEST_F(ServiceRegistry_test, MultipleServiceDescriptionWithSameServiceNameCanAllBeFound) @@ -222,11 +222,11 @@ TEST_F(ServiceRegistry_test, MultipleServiceDescriptionWithSameServiceNameCanAll for (auto& e : searchResults) { - if (e.first == service1) + if (e.serviceDescription == service1) hasFoundB = true; - if (e.first == service2) + if (e.serviceDescription == service2) hasFoundC = true; - if (e.first == service3) + if (e.serviceDescription == service3) hasFoundD = true; } @@ -243,12 +243,12 @@ TEST_F(ServiceRegistry_test, MultipleServiceDescriptionWithDifferentServiceNameC registry.find(searchResults, "a", Wildcard); EXPECT_THAT(searchResults.size(), Eq(1)); - EXPECT_THAT(searchResults[0].first, Eq(service1)); + EXPECT_THAT(searchResults[0].serviceDescription, Eq(service1)); searchResults.clear(); registry.find(searchResults, "c", Wildcard); EXPECT_THAT(searchResults.size(), Eq(1)); - EXPECT_THAT(searchResults[0].first, Eq(service2)); + EXPECT_THAT(searchResults[0].serviceDescription, Eq(service2)); } TEST_F(ServiceRegistry_test, MultipleServiceDescriptionWithSameServiceNameFindsSpecificService) @@ -263,7 +263,7 @@ TEST_F(ServiceRegistry_test, MultipleServiceDescriptionWithSameServiceNameFindsS registry.find(searchResults, "a", "c"); EXPECT_THAT(searchResults.size(), Eq(1)); - EXPECT_THAT(searchResults[0].first, Eq(service2)); + EXPECT_THAT(searchResults[0].serviceDescription, Eq(service2)); } TEST_F(ServiceRegistry_test, MultipleServiceDescriptionAddedInNonLinearOrderFindsCorrectServices) @@ -374,17 +374,17 @@ TEST_F(ServiceRegistry_test, AddingVariousServiceDescriptionAndGetServicesDoesNo for (auto const& element : serviceDescriptionVector) { - if (element.first == service1) + if (element.serviceDescription == service1) { service1Found = true; } - if (element.first == service2) + if (element.serviceDescription == service2) { service2Found = true; } - if (element.first == service4) + if (element.serviceDescription == service4) { service4Found = true; } From 41b82945e607de0d0ad116def807b98eff887b0a Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Mon, 9 Aug 2021 11:10:06 +0200 Subject: [PATCH 30/32] iox-#415 Fix some typos Signed-off-by: Simon Hoinkis --- .../include/iceoryx_posh/internal/roudi/service_registry.hpp | 4 ++-- iceoryx_posh/source/roudi/service_registry.cpp | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp b/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp index f7188db18b..18b7aa8e93 100644 --- a/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp +++ b/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp @@ -64,10 +64,10 @@ class ServiceRegistry /// @brief Removes given service description from registry /// @param[in] searchResult, reference to the vector which will be filled with the results /// @param[in] service, string or wildcard to search for - /// @param[in] service, string or wildcard to search for + /// @param[in] instance, string or wildcard to search for void find(ServiceDescriptionVector_t& searchResult, const capro::IdString_t& service = Wildcard, - const capro::IdString_t& instinstanceance = Wildcard) const noexcept; + const capro::IdString_t& instance = Wildcard) const noexcept; /// @brief Returns all service descriptions as copy /// @return ServiceDescriptionVector_t, copy of complete service registry diff --git a/iceoryx_posh/source/roudi/service_registry.cpp b/iceoryx_posh/source/roudi/service_registry.cpp index 66bc78f09e..e740784b6e 100644 --- a/iceoryx_posh/source/roudi/service_registry.cpp +++ b/iceoryx_posh/source/roudi/service_registry.cpp @@ -35,7 +35,7 @@ cxx::expected ServiceRegistry::add(const capro::ServiceD return cxx::success<>(); } } - uint64_t referenceCounter = 1; + uint64_t referenceCounter = 1U; if (!m_serviceDescriptionVector.push_back({serviceDescription, referenceCounter})) { return cxx::error(Error::SERVICE_REGISTRY_FULL); From bb8a3483a3c820225e461b37372b9c249d5da54e Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Mon, 9 Aug 2021 11:40:58 +0200 Subject: [PATCH 31/32] iox-#415 Fix U's and ServiceRegistry::remove Signed-off-by: Simon Hoinkis --- iceoryx_posh/source/roudi/service_registry.cpp | 15 +++++++++------ iceoryx_posh/source/runtime/posh_runtime_impl.cpp | 2 +- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/iceoryx_posh/source/roudi/service_registry.cpp b/iceoryx_posh/source/roudi/service_registry.cpp index e740784b6e..7ce87d8061 100644 --- a/iceoryx_posh/source/roudi/service_registry.cpp +++ b/iceoryx_posh/source/roudi/service_registry.cpp @@ -57,7 +57,7 @@ bool ServiceRegistry::remove(const capro::ServiceDescription& serviceDescription if (element == serviceDescription) { auto& refCounter = m_serviceDescriptionVector[index].referenceCounter; - refCounter--; + --refCounter; if (refCounter == 0) { m_serviceDescriptionVector.erase(iterator); @@ -70,8 +70,8 @@ bool ServiceRegistry::remove(const capro::ServiceDescription& serviceDescription // There can't be more than one element break; } - index++; - iterator++; + ++index; + ++iterator; } auto removeIndexFromMap = [](std::multimap& map, uint64_t index) { @@ -91,8 +91,11 @@ bool ServiceRegistry::remove(const capro::ServiceDescription& serviceDescription } }; - removeIndexFromMap(m_serviceMap, index); - removeIndexFromMap(m_instanceMap, index); + if (removedElement) + { + removeIndexFromMap(m_serviceMap, index); + removeIndexFromMap(m_instanceMap, index); + } return removedElement; } @@ -156,7 +159,7 @@ void ServiceRegistry::find(ServiceDescriptionVector_t& searchResult, else { // Find (*, *) - // O(1) + // O(n) searchResult = m_serviceDescriptionVector; } } diff --git a/iceoryx_posh/source/runtime/posh_runtime_impl.cpp b/iceoryx_posh/source/runtime/posh_runtime_impl.cpp index c583d9cb86..6513707b0d 100644 --- a/iceoryx_posh/source/runtime/posh_runtime_impl.cpp +++ b/iceoryx_posh/source/runtime/posh_runtime_impl.cpp @@ -417,7 +417,7 @@ PoshRuntimeImpl::findService(const cxx::variant s // Limit the services (max value is the capacity of serviceContainer) uint32_t numberOfServices = algorithm::min(capacity, numberOfElements); - for (uint32_t i = 0; i < numberOfServices; ++i) + for (uint32_t i = 0U; i < numberOfServices; ++i) { capro::ServiceDescription service(cxx::Serialization(requestResponse.getElementAtIndex(i))); serviceContainer.push_back(service); From 78d5952c1e1d166b9409aa19ad3f953a6b1e74f9 Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Mon, 9 Aug 2021 11:43:39 +0200 Subject: [PATCH 32/32] iox-#415 Fix code formatting Signed-off-by: Simon Hoinkis --- .../include/iceoryx_posh/internal/roudi/service_registry.hpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp b/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp index 18b7aa8e93..fee392d1bb 100644 --- a/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp +++ b/iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp @@ -48,8 +48,7 @@ class ServiceRegistry /// @todo #415 should be connected with iox::MAX_NUMBER_OF_SERVICES static constexpr uint32_t MAX_SERVICE_DESCRIPTIONS = 100U; - using ServiceDescriptionVector_t = - cxx::vector; + using ServiceDescriptionVector_t = cxx::vector; /// @brief Adds given service description to registry /// @param[in] serviceDescription, service to be added