Make DataWriters always send the key hash on keyed topics (#4238) (#4350

) * Refs #20239: Add regression test Signed-off-by: Mario Dominguez <[email protected]> Signed-off-by: EduPonz <[email protected]> * Refs #20239: Fix Signed-off-by: Mario Dominguez <[email protected]> Signed-off-by: EduPonz <[email protected]> * Refs #20239: Linter Signed-off-by: Mario Dominguez <[email protected]> Signed-off-by: EduPonz <[email protected]> * Refs #20239: Change some assert logic in DDSStatus tests since now, a not null instancehandle should be expected Signed-off-by: Mario Dominguez <[email protected]> Signed-off-by: EduPonz <[email protected]> * Refs #20239: Apply rev suggestions Signed-off-by: Mario Dominguez <[email protected]> Signed-off-by: EduPonz <[email protected]> --------- Signed-off-by: Mario Dominguez <[email protected]> Signed-off-by: EduPonz <[email protected]> (cherry picked from commit 4d8c489) Co-authored-by: Mario Domínguez López <[email protected]>
eProsima · Mar 12, 2024 · ae9c1e9 · ae9c1e9
1 parent 5293d0d
commit ae9c1e9
Show file tree

Hide file tree

Showing 3 changed files with 108 additions and 6 deletions.
diff --git a/src/cpp/rtps/messages/submessages/DataMsg.hpp b/src/cpp/rtps/messages/submessages/DataMsg.hpp
@@ -42,7 +42,8 @@ struct DataMsgUtils
     {
         inlineQosFlag =
                 (nullptr != inlineQos) ||
-                ((WITH_KEY == topicKind) && (expectsInlineQos || change->kind != ALIVE)) ||
+                ((WITH_KEY == topicKind) &&
+                (!change->writerGUID.is_builtin() || expectsInlineQos || change->kind != ALIVE)) ||
                 (change->write_params.related_sample_identity() != SampleIdentity::unknown());
 
         dataFlag = ALIVE == change->kind &&
@@ -129,7 +130,7 @@ struct DataMsgUtils
                     change->write_params.related_sample_identity());
         }
 
-        if (WITH_KEY == topicKind && (expectsInlineQos || ALIVE != change->kind))
+        if (WITH_KEY == topicKind && (!change->writerGUID.is_builtin() || expectsInlineQos || ALIVE != change->kind))
         {
             fastdds::dds::ParameterSerializer<Parameter_t>::add_parameter_key(msg, change->instanceHandle);
 

diff --git a/test/blackbox/common/BlackboxTestsKeys.cpp b/test/blackbox/common/BlackboxTestsKeys.cpp
@@ -17,6 +17,8 @@
 #include "PubSubReader.hpp"
 #include "PubSubWriter.hpp"
 
+#include <fastrtps/transport/test_UDPv4TransportDescriptor.h>
+
 TEST(KeyedTopic, RegistrationNonKeyedFail)
 {
     PubSubWriter<HelloWorldPubSubType> writer(TEST_TOPIC_NAME);
@@ -181,6 +183,105 @@ TEST(KeyedTopic, UnregisterWhenHistoryKeepAll)
     ASSERT_TRUE(writer.unregister_instance(data.back(), instance_handle_2));
 }
 
+// Regression test for redmine issue #20239
+TEST(KeyedTopic, DataWriterAlwaysSendTheSerializedKeyViaInlineQoS)
+{
+    PubSubWriter<KeyedHelloWorldPubSubType> writer(TEST_TOPIC_NAME);
+    PubSubReader<KeyedHelloWorldPubSubType> reader(TEST_TOPIC_NAME);
+
+    auto testTransport = std::make_shared<eprosima::fastdds::rtps::test_UDPv4TransportDescriptor>();
+
+    bool writer_sends_inline_qos = true;
+    bool writer_sends_pid_key_hash = true;
+
+    testTransport->drop_data_messages_filter_ = [&writer_sends_inline_qos,
+                    &writer_sends_pid_key_hash](eprosima::fastrtps::rtps::CDRMessage_t& msg) -> bool
+            {
+                // Check for inline_qos
+                uint8_t flags = msg.buffer[msg.pos - 3];
+                auto old_pos = msg.pos;
+
+                // Skip extraFlags, read octetsToInlineQos, and calculate inline qos position.
+                msg.pos += 2;
+                uint16_t to_inline_qos = 0;
+                eprosima::fastrtps::rtps::CDRMessage::readUInt16(&msg, &to_inline_qos);
+                uint32_t inline_qos_pos = msg.pos + to_inline_qos;
+
+                // Filters are only applied to user data
+                // no need to check if the packets comer from a builtin
+
+                writer_sends_inline_qos &= static_cast<bool>((flags & (1 << 1)));
+
+                // Stop seeking if inline qos are not present
+                // Fail the test afterwards
+                if (!writer_sends_inline_qos)
+                {
+                    return false;
+                }
+                else
+                {
+                    // Process inline qos
+                    msg.pos = inline_qos_pos;
+                    bool key_hash_was_found = false;
+                    while (msg.pos < msg.length)
+                    {
+                        uint16_t pid = 0;
+                        uint16_t plen = 0;
+
+                        eprosima::fastrtps::rtps::CDRMessage::readUInt16(&msg, &pid);
+                        eprosima::fastrtps::rtps::CDRMessage::readUInt16(&msg, &plen);
+                        uint32_t next_pos = msg.pos + plen;
+
+                        if (pid == eprosima::fastdds::dds::PID_KEY_HASH)
+                        {
+                            key_hash_was_found = true;
+                        }
+                        else if (pid == eprosima::fastdds::dds::PID_SENTINEL)
+                        {
+                            break;
+                        }
+
+                        msg.pos = next_pos;
+                    }
+
+                    writer_sends_pid_key_hash &= key_hash_was_found;
+                    msg.pos = old_pos;
+                }
+
+                // Do not drop the packet in any case
+                return false;
+            };
+
+    writer.
+            disable_builtin_transport().
+            add_user_transport_to_pparams(testTransport).
+            init();
+
+    ASSERT_TRUE(writer.isInitialized());
+
+    reader.
+            expect_inline_qos(false).
+            init();
+
+    ASSERT_TRUE(reader.isInitialized());
+
+    // Wait for discovery.
+    writer.wait_discovery();
+    reader.wait_discovery();
+
+    auto data = default_keyedhelloworld_data_generator(5);
+
+    reader.startReception(data);
+    writer.send(data);
+
+    // In this test all data should be sent.
+    EXPECT_TRUE(data.empty());
+    reader.block_for_all();
+
+    EXPECT_TRUE(writer_sends_inline_qos);
+    EXPECT_TRUE(writer_sends_pid_key_hash);
+}
+
 /* Uncomment when DDS API supports NO_WRITERS_ALIVE
    TEST(KeyedTopic, WriteSamplesBestEffort)
    {

diff --git a/test/blackbox/common/DDSBlackboxTestsListeners.cpp b/test/blackbox/common/DDSBlackboxTestsListeners.cpp
@@ -1732,7 +1732,7 @@ TEST(DDSStatus, sample_rejected_key_re_dw_re_dr_keep_all_max_samples_2)
     ASSERT_EQ(5u, test_status.total_count);
     ASSERT_EQ(5u, test_status.total_count_change);
     ASSERT_EQ(eprosima::fastdds::dds::REJECTED_BY_SAMPLES_LIMIT, test_status.last_reason);
-    ASSERT_EQ(c_InstanceHandle_Unknown, test_status.last_instance_handle);
+    ASSERT_NE(c_InstanceHandle_Unknown, test_status.last_instance_handle);
 }
 
 /*!
@@ -1832,7 +1832,7 @@ TEST(DDSStatus, sample_rejected_key_large_re_dw_re_dr_keep_all_max_samples_2)
     ASSERT_EQ(5u, test_status.total_count);
     ASSERT_EQ(5u, test_status.total_count_change);
     ASSERT_EQ(eprosima::fastdds::dds::REJECTED_BY_SAMPLES_LIMIT, test_status.last_reason);
-    ASSERT_EQ(c_InstanceHandle_Unknown, test_status.last_instance_handle);
+    ASSERT_NE(c_InstanceHandle_Unknown, test_status.last_instance_handle);
 }
 
 /*!
@@ -1926,7 +1926,7 @@ TEST(DDSStatus, sample_rejected_key_re_dw_re_dr_keep_last_max_samples_2)
     ASSERT_EQ(5u, test_status.total_count);
     ASSERT_EQ(5u, test_status.total_count_change);
     ASSERT_EQ(eprosima::fastdds::dds::REJECTED_BY_SAMPLES_LIMIT, test_status.last_reason);
-    ASSERT_EQ(c_InstanceHandle_Unknown, test_status.last_instance_handle);
+    ASSERT_NE(c_InstanceHandle_Unknown, test_status.last_instance_handle);
 }
 
 /*!
@@ -2026,7 +2026,7 @@ TEST(DDSStatus, sample_rejected_key_large_re_dw_re_dr_keep_last_max_samples_2)
     ASSERT_EQ(5u, test_status.total_count);
     ASSERT_EQ(5u, test_status.total_count_change);
     ASSERT_EQ(eprosima::fastdds::dds::REJECTED_BY_SAMPLES_LIMIT, test_status.last_reason);
-    ASSERT_EQ(c_InstanceHandle_Unknown, test_status.last_instance_handle);
+    ASSERT_NE(c_InstanceHandle_Unknown, test_status.last_instance_handle);
 }
 
 /*!