Merge pull request #1131 from ApexAI/iox-#938-check-for-publisher-des…

…truction-in-does-violate-communication-policy

iox-#938 check for publisher destruction in does violate communication policy

doc/website/release-notes/iceoryx-v2-0-0.md

@@ -46,7 +46,8 @@
 - When posix mutex fails a correct error message is reported on the console [\#999](https://github.com/eclipse-iceoryx/iceoryx/issues/999)
 - Only use `std::result_of` for C++14 to be able to use iceoryx in C++20 projects [\#1076](https://github.com/eclipse-iceoryx/iceoryx/issues/1076)
 - Set stack size for windows in `singleprocess` example and posh tests [\#1082](https://github.com/eclipse-iceoryx/iceoryx/issues/1082)
-- Roudi console timestamps is out of date [#1130](https://github.com/eclipse-iceoryx/iceoryx/issues/1130)
+- Roudi console timestamps are out of date [#1130](https://github.com/eclipse-iceoryx/iceoryx/issues/1130)
+- Application can't create publisher repeatedly with previous one already destroyed [\#938](https://github.com/eclipse-iceoryx/iceoryx/issues/938)
 
 **Refactoring:**
 

iceoryx_posh/include/iceoryx_posh/internal/roudi/port_manager.hpp

@@ -128,12 +128,11 @@ class PortManager
     void removeEntryFromServiceRegistry(const capro::ServiceDescription& service) noexcept;
 
     template <typename T, std::enable_if_t<std::is_same<T, iox::build::OneToManyPolicy>::value>* = nullptr>
-    cxx::optional<RuntimeName_t>
-    doesViolateCommunicationPolicy(const capro::ServiceDescription& service) const noexcept;
+    cxx::optional<RuntimeName_t> doesViolateCommunicationPolicy(const capro::ServiceDescription& service) noexcept;
 
     template <typename T, std::enable_if_t<std::is_same<T, iox::build::ManyToManyPolicy>::value>* = nullptr>
     cxx::optional<RuntimeName_t>
-    doesViolateCommunicationPolicy(const capro::ServiceDescription& service IOX_MAYBE_UNUSED) const noexcept;
+    doesViolateCommunicationPolicy(const capro::ServiceDescription& service IOX_MAYBE_UNUSED) noexcept;
 
     void publishServiceRegistry() const noexcept;
 

iceoryx_posh/include/iceoryx_posh/internal/roudi/port_manager.inl

@@ -1,4 +1,5 @@
 // Copyright (c) 2020 by Robert Bosch GmbH. All rights reserved.
+// Copyright (c) 2022 by Apex.AI Inc. All rights reserved.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -16,20 +17,27 @@
 #ifndef IOX_POSH_INTERNAL_ROUDI_PORT_MANAGER_INL
 #define IOX_POSH_INTERNAL_ROUDI_PORT_MANAGER_INL
 
+#include "iceoryx_posh/internal/roudi/port_manager.hpp"
+
 namespace iox
 {
 namespace roudi
 {
 template <typename T, std::enable_if_t<std::is_same<T, iox::build::OneToManyPolicy>::value>*>
 inline cxx::optional<RuntimeName_t>
-PortManager::doesViolateCommunicationPolicy(const capro::ServiceDescription& service) const noexcept
+PortManager::doesViolateCommunicationPolicy(const capro::ServiceDescription& service) noexcept
 {
     // check if the publisher is already in the list
     for (auto publisherPortData : m_portPool->getPublisherPortDataList())
     {
         popo::PublisherPortRouDi publisherPort(publisherPortData);
         if (service == publisherPort.getCaProServiceDescription())
         {
+            if (publisherPortData->m_toBeDestroyed)
+            {
+                destroyPublisherPort(publisherPortData);
+                continue;
+            }
             return cxx::make_optional<RuntimeName_t>(publisherPortData->m_runtimeName);
         }
     }
@@ -38,7 +46,7 @@ PortManager::doesViolateCommunicationPolicy(const capro::ServiceDescription& ser
 
 template <typename T, std::enable_if_t<std::is_same<T, iox::build::ManyToManyPolicy>::value>*>
 inline cxx::optional<RuntimeName_t>
-PortManager::doesViolateCommunicationPolicy(const capro::ServiceDescription& service IOX_MAYBE_UNUSED) const noexcept
+PortManager::doesViolateCommunicationPolicy(const capro::ServiceDescription&) noexcept
 {
     // Duplicates are allowed when using n:m policy
     return cxx::nullopt;

iceoryx_posh/test/moduletests/test_roudi_portmanager.cpp

@@ -442,6 +442,76 @@ TEST_F(PortManager_test, AcquiringOneMoreThanMaximumNumberOfPublishersFails)
     }
 }
 
+constexpr bool IS_COMMUNICATION_POLICY_ONE_TO_MANY_ONLY{
+    std::is_same<iox::build::CommunicationPolicy, iox::build::OneToManyPolicy>::value};
+
+TEST_F(PortManager_test, AcquirePublisherPortDataWithSameServiceDescriptionTwiceWorksAccordingCommunicationPolicy)
+{
+    ::testing::Test::RecordProperty("TEST_ID", "6b26220c-01a3-4f3a-8af4-06c66d6f98ef");
+    const iox::capro::ServiceDescription sd{"hyp", "no", "toad"};
+    const iox::RuntimeName_t runtimeName{"hypnotoad"};
+    auto publisherOptions = createTestPubOptions();
+
+    // first call must be successful
+    m_portManager->acquirePublisherPortData(sd, publisherOptions, runtimeName, m_payloadDataSegmentMemoryManager, {})
+        .or_else([&](const auto& error) {
+            GTEST_FAIL() << "Expected ClientPortData but got PortPoolError: " << static_cast<uint8_t>(error);
+        });
+
+    iox::cxx::optional<iox::Error> detectedError;
+    auto errorHandlerGuard =
+        iox::ErrorHandler::setTemporaryErrorHandler([&](const auto error, const auto, const auto errorLevel) {
+            EXPECT_THAT(error, Eq(iox::Error::kPOSH__PORT_MANAGER_PUBLISHERPORT_NOT_UNIQUE));
+            EXPECT_THAT(errorLevel, Eq(iox::ErrorLevel::MODERATE));
+            detectedError.emplace(error);
+        });
+
+    // second call
+    auto acquirePublisherPortResult = m_portManager->acquirePublisherPortData(
+        sd, publisherOptions, runtimeName, m_payloadDataSegmentMemoryManager, {});
+
+    if (IS_COMMUNICATION_POLICY_ONE_TO_MANY_ONLY)
+    {
+        ASSERT_TRUE(acquirePublisherPortResult.has_error());
+        EXPECT_THAT(acquirePublisherPortResult.get_error(), Eq(PortPoolError::UNIQUE_PUBLISHER_PORT_ALREADY_EXISTS));
+        EXPECT_TRUE(detectedError.has_value());
+    }
+    else
+    {
+        EXPECT_FALSE(detectedError.has_value());
+    }
+}
+
+TEST_F(PortManager_test,
+       AcquirePublisherPortDataWithSameServiceDescriptionTwiceAndFirstPortMarkedToBeDestroyedReturnsPort)
+{
+    ::testing::Test::RecordProperty("TEST_ID", "0840c279-5429-4c93-a120-738735a89100");
+    const iox::capro::ServiceDescription sd{"hyp", "no", "toad"};
+    const iox::RuntimeName_t runtimeName{"hypnotoad"};
+    auto publisherOptions = createTestPubOptions();
+
+    // first call must be successful
+    auto publisherPortDataResult = m_portManager->acquirePublisherPortData(
+        sd, publisherOptions, runtimeName, m_payloadDataSegmentMemoryManager, {});
+
+    ASSERT_FALSE(publisherPortDataResult.has_error());
+
+    publisherPortDataResult.value()->m_toBeDestroyed = true;
+
+    iox::cxx::optional<iox::Error> detectedError;
+    auto errorHandlerGuard = iox::ErrorHandler::setTemporaryErrorHandler(
+        [&](const auto error, const auto, const auto) { detectedError.emplace(error); });
+
+    // second call must now also succeed
+    m_portManager->acquirePublisherPortData(sd, publisherOptions, runtimeName, m_payloadDataSegmentMemoryManager, {})
+        .or_else([&](const auto& error) {
+            GTEST_FAIL() << "Expected ClientPortData but got PortPoolError: " << static_cast<uint8_t>(error);
+        });
+
+    detectedError.and_then(
+        [&](const auto& error) { GTEST_FAIL() << "Expected error handler to not be called but got: " << error; });
+}
+
 TEST_F(PortManager_test, AcquiringOneMoreThanMaximumNumberOfSubscribersFails)
 {
     ::testing::Test::RecordProperty("TEST_ID", "5039eff5-f2d1-4f58-8bd2-fc9768a9bc92");