eProsima / Fast-DDS

The most complete DDS - Proven: Plenty of success cases. Looking for commercial support? Contact info@eprosima.com
https://eprosima.com
Apache License 2.0
2.16k stars 765 forks source link

[12551] Unable to create multiple multicast locators on the same port #2186

Open ghost opened 3 years ago

ghost commented 3 years ago

It is not possible to create multiple multicast locators on the same port.

Steps to Reproduce

  1. Create DataReader with:
    • Topic name /group/0/some_topic_name
    • UDP4 multicast locator: ip address: 239.0.0.1, port: 7900
  2. Create another DataReader with:
    • Topic name: /group/1/some_topic_name
    • UDP4 multicast locator: ip address: 239.0.0.2, port: 7900

Expected Behavior

Compose a test program and check that Fast DDS creates a listener for each multicast locator:

$ netstat -lup|grep 239
(Not all processes could be identified, non-owned process info
 will not be shown, you would have to be root to see it all.)
udp        0      0 239.0.0.1:7900        0.0.0.0:*                           839787/./test
udp        0      0 239.0.0.2:7900        0.0.0.0:*                           839787/./test

Current Behavior

Fast DDS creates only a single listener:

$ netstat -lup|grep 239
(Not all processes could be identified, non-owned process info
 will not be shown, you would have to be root to see it all.)
udp        0      0 239.0.0.1:7900        0.0.0.0:*                           839787/./test

System information

Additional context

The function UDPTransportInterface::IsInputChannelOpen() is responsible to check if listener has been already created. Unfortunately it compares only port value:

    return IsLocatorSupported(locator) && (mInputSockets.find(IPLocator::getPhysicalPort(
               locator)) != mInputSockets.end());

And completely ignores IP address value.

JesusPoderoso commented 1 year ago

Hi @oleksandr-hryshchuk, thanks for your contribution and sorry for the late response. Can you check if this issue persists in current Fast DDS v2.11.0?

ghost commented 1 year ago

Hi @JesusPoderoso, it does not work correctly, still. Fast DDS is using only UDP port to resolve locators. It does not even send an IGMP join message for the second multicast IP.

i-and commented 7 months ago

The specified error is also present in the version v2.13.0. At the same time, in multicast locators, you can specify 0 as the port number (calculated by default). In this case, the library itself will set the same ports (for example, for domain 0 it will be port 7401), which will lead to a lack of data reception from the second and subsequent locators. That is, the issue is present in this case as well. The patch below solves the problem. It was tested on version v2.13.0 with UDP transport and in configuration options with and without a interface whitelist.

diff --git a/src/cpp/rtps/network/ReceiverResource.cpp b/src/cpp/rtps/network/ReceiverResource.cpp
index f2b1a37a1..f577f390d 100644
--- a/src/cpp/rtps/network/ReceiverResource.cpp
+++ b/src/cpp/rtps/network/ReceiverResource.cpp
@@ -35,6 +35,7 @@ ReceiverResource::ReceiverResource(
         uint32_t max_recv_buffer_size)
     : Cleanup(nullptr)
     , LocatorMapsToManagedChannel(nullptr)
+    , AddMulticastLocatorFunc(nullptr)
     , mValid(false)
     , mtx()
     , cv_()
@@ -58,6 +59,16 @@ ReceiverResource::ReceiverResource(
             {
                 return locator.kind == locatorToCheck.kind && transport.DoInputLocatorsMatch(locator, locatorToCheck);
             };
+    AddMulticastLocatorFunc = [&transport, this] (const Locator_t& locator, bool& same_exists) -> bool
+            {
+                if (!IPLocator::isMulticast(locator) || !SupportsLocator(locator))
+                {
+                    same_exists = false;
+                    return true;
+                }
+                same_exists = true;
+                return transport.OpenInputChannel(locator, this, max_message_size_);
+            };
 }

 ReceiverResource::ReceiverResource(
@@ -67,6 +78,7 @@ ReceiverResource::ReceiverResource(

     Cleanup.swap(rValueResource.Cleanup);
     LocatorMapsToManagedChannel.swap(rValueResource.LocatorMapsToManagedChannel);
+    AddMulticastLocatorFunc.swap(rValueResource.AddMulticastLocatorFunc);
     receiver = rValueResource.receiver;
     rValueResource.receiver = nullptr;
     mValid = rValueResource.mValid;
@@ -86,6 +98,17 @@ bool ReceiverResource::SupportsLocator(
     return false;
 }

+bool ReceiverResource::AddMulticastLocator(
+        const Locator_t& locator,
+        bool& same_exists)
+{
+    if (AddMulticastLocatorFunc)
+    {
+        return AddMulticastLocatorFunc(locator, same_exists);
+    }
+    return false;
+}
+
 void ReceiverResource::RegisterReceiver(
         MessageReceiver* rcv)
 {
diff --git a/src/cpp/rtps/network/ReceiverResource.h b/src/cpp/rtps/network/ReceiverResource.h
index d2744c52f..4acccc914 100644
--- a/src/cpp/rtps/network/ReceiverResource.h
+++ b/src/cpp/rtps/network/ReceiverResource.h
@@ -63,6 +63,13 @@ public:
     bool SupportsLocator(
             const Locator_t& localLocator);

+    /**
+     * Try to add multicast locator to existing ReceiverResource
+     */
+    bool AddMulticastLocator(
+            const Locator_t& locator,
+            bool& same_exists);
+
     /**
      * Register a MessageReceiver object to be called upon reception of data.
      * @param receiver The message receiver to register.
@@ -110,6 +117,7 @@ private:
             uint32_t);
     std::function<void()> Cleanup;
     std::function<bool(const Locator_t&)> LocatorMapsToManagedChannel;
+    std::function<bool(const Locator_t&, bool& same_exists)> AddMulticastLocatorFunc;
     bool mValid; // Post-construction validity check for the NetworkFactory

     std::mutex mtx;
diff --git a/src/cpp/rtps/participant/RTPSParticipantImpl.cpp b/src/cpp/rtps/participant/RTPSParticipantImpl.cpp
index 53ac4626d..3cc47bf4c 100644
--- a/src/cpp/rtps/participant/RTPSParticipantImpl.cpp
+++ b/src/cpp/rtps/participant/RTPSParticipantImpl.cpp
@@ -1727,6 +1727,21 @@ bool RTPSParticipantImpl::createReceiverResources(

     for (auto it_loc = Locator_list.begin(); it_loc != Locator_list.end(); ++it_loc)
     {
+        if (IPLocator::isMulticast(*it_loc))
+        {
+            bool same_multicast_exists = false;
+            std::lock_guard<std::mutex> lock(m_receiverResourcelistMutex);
+            for (auto it = m_receiverResourcelist.begin(); it != m_receiverResourcelist.end(); ++it)
+            {
+                bool exists = false;
+                it->Receiver->AddMulticastLocator(*it_loc, exists);
+                same_multicast_exists = same_multicast_exists || exists;
+            }
+            if (same_multicast_exists)
+            {
+                continue;
+            }
+        }
         bool ret = m_network_Factory.BuildReceiverResources(*it_loc, newItemsBuffer, max_receiver_buffer_size);
         if (!ret && ApplyMutation)
         {
i-and commented 6 months ago

@MiguelCompany, @JesusPoderoso, сan you consider the above patch for inclusion in the next release?

JesusPoderoso commented 6 months ago

Hi @i-and, we have internally considered including it in the next release. As soon as it goes in, we will let you know. Thanks for your patience!