Skip to content

Commit f58fbb5

Browse files
mergify[bot]Mario-DL
mergify[bot]
and
Mario-DL
authored
Make DataWriters always send the key hash on keyed topics (#4238) (#4352)
* 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]>
1 parent 292ac5f commit f58fbb5

File tree

3 files changed

+108
-6
lines changed

3 files changed

+108
-6
lines changed

‎src/cpp/rtps/messages/submessages/DataMsg.hpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,8 @@ struct DataMsgUtils
4242
{
4343
inlineQosFlag =
4444
(nullptr != inlineQos) ||
45-
((WITH_KEY == topicKind) && (expectsInlineQos || change->kind != ALIVE)) ||
45+
((WITH_KEY == topicKind) &&
46+
(!change->writerGUID.is_builtin() || expectsInlineQos || change->kind != ALIVE)) ||
4647
(change->write_params.related_sample_identity() != SampleIdentity::unknown());
4748

4849
dataFlag = ALIVE == change->kind &&
@@ -129,7 +130,7 @@ struct DataMsgUtils
129130
change->write_params.related_sample_identity());
130131
}
131132

132-
if (WITH_KEY == topicKind && (expectsInlineQos || ALIVE != change->kind))
133+
if (WITH_KEY == topicKind && (!change->writerGUID.is_builtin() || expectsInlineQos || ALIVE != change->kind))
133134
{
134135
fastdds::dds::ParameterSerializer<Parameter_t>::add_parameter_key(msg, change->instanceHandle);
135136

‎test/blackbox/common/BlackboxTestsKeys.cpp

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@
1717
#include "PubSubReader.hpp"
1818
#include "PubSubWriter.hpp"
1919

20+
#include <fastrtps/transport/test_UDPv4TransportDescriptor.h>
21+
2022
TEST(KeyedTopic, RegistrationNonKeyedFail)
2123
{
2224
PubSubWriter<HelloWorldPubSubType> writer(TEST_TOPIC_NAME);
@@ -181,6 +183,105 @@ TEST(KeyedTopic, UnregisterWhenHistoryKeepAll)
181183
ASSERT_TRUE(writer.unregister_instance(data.back(), instance_handle_2));
182184
}
183185

186+
// Regression test for redmine issue #20239
187+
TEST(KeyedTopic, DataWriterAlwaysSendTheSerializedKeyViaInlineQoS)
188+
{
189+
PubSubWriter<KeyedHelloWorldPubSubType> writer(TEST_TOPIC_NAME);
190+
PubSubReader<KeyedHelloWorldPubSubType> reader(TEST_TOPIC_NAME);
191+
192+
auto testTransport = std::make_shared<eprosima::fastdds::rtps::test_UDPv4TransportDescriptor>();
193+
194+
bool writer_sends_inline_qos = true;
195+
bool writer_sends_pid_key_hash = true;
196+
197+
testTransport->drop_data_messages_filter_ = [&writer_sends_inline_qos,
198+
&writer_sends_pid_key_hash](eprosima::fastrtps::rtps::CDRMessage_t& msg) -> bool
199+
{
200+
// Check for inline_qos
201+
uint8_t flags = msg.buffer[msg.pos - 3];
202+
auto old_pos = msg.pos;
203+
204+
// Skip extraFlags, read octetsToInlineQos, and calculate inline qos position.
205+
msg.pos += 2;
206+
uint16_t to_inline_qos = 0;
207+
eprosima::fastrtps::rtps::CDRMessage::readUInt16(&msg, &to_inline_qos);
208+
uint32_t inline_qos_pos = msg.pos + to_inline_qos;
209+
210+
// Filters are only applied to user data
211+
// no need to check if the packets comer from a builtin
212+
213+
writer_sends_inline_qos &= static_cast<bool>((flags & (1 << 1)));
214+
215+
// Stop seeking if inline qos are not present
216+
// Fail the test afterwards
217+
if (!writer_sends_inline_qos)
218+
{
219+
return false;
220+
}
221+
else
222+
{
223+
// Process inline qos
224+
msg.pos = inline_qos_pos;
225+
bool key_hash_was_found = false;
226+
while (msg.pos < msg.length)
227+
{
228+
uint16_t pid = 0;
229+
uint16_t plen = 0;
230+
231+
eprosima::fastrtps::rtps::CDRMessage::readUInt16(&msg, &pid);
232+
eprosima::fastrtps::rtps::CDRMessage::readUInt16(&msg, &plen);
233+
uint32_t next_pos = msg.pos + plen;
234+
235+
if (pid == eprosima::fastdds::dds::PID_KEY_HASH)
236+
{
237+
key_hash_was_found = true;
238+
}
239+
else if (pid == eprosima::fastdds::dds::PID_SENTINEL)
240+
{
241+
break;
242+
}
243+
244+
msg.pos = next_pos;
245+
}
246+
247+
writer_sends_pid_key_hash &= key_hash_was_found;
248+
msg.pos = old_pos;
249+
}
250+
251+
// Do not drop the packet in any case
252+
return false;
253+
};
254+
255+
writer.
256+
disable_builtin_transport().
257+
add_user_transport_to_pparams(testTransport).
258+
init();
259+
260+
ASSERT_TRUE(writer.isInitialized());
261+
262+
reader.
263+
expect_inline_qos(false).
264+
init();
265+
266+
ASSERT_TRUE(reader.isInitialized());
267+
268+
// Wait for discovery.
269+
writer.wait_discovery();
270+
reader.wait_discovery();
271+
272+
auto data = default_keyedhelloworld_data_generator(5);
273+
274+
reader.startReception(data);
275+
writer.send(data);
276+
277+
// In this test all data should be sent.
278+
EXPECT_TRUE(data.empty());
279+
reader.block_for_all();
280+
281+
EXPECT_TRUE(writer_sends_inline_qos);
282+
EXPECT_TRUE(writer_sends_pid_key_hash);
283+
}
284+
184285
/* Uncomment when DDS API supports NO_WRITERS_ALIVE
185286
TEST(KeyedTopic, WriteSamplesBestEffort)
186287
{

‎test/blackbox/common/DDSBlackboxTestsListeners.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1732,7 +1732,7 @@ TEST(DDSStatus, sample_rejected_key_re_dw_re_dr_keep_all_max_samples_2)
17321732
ASSERT_EQ(5u, test_status.total_count);
17331733
ASSERT_EQ(5u, test_status.total_count_change);
17341734
ASSERT_EQ(eprosima::fastdds::dds::REJECTED_BY_SAMPLES_LIMIT, test_status.last_reason);
1735-
ASSERT_EQ(c_InstanceHandle_Unknown, test_status.last_instance_handle);
1735+
ASSERT_NE(c_InstanceHandle_Unknown, test_status.last_instance_handle);
17361736
}
17371737

17381738
/*!
@@ -1832,7 +1832,7 @@ TEST(DDSStatus, sample_rejected_key_large_re_dw_re_dr_keep_all_max_samples_2)
18321832
ASSERT_EQ(5u, test_status.total_count);
18331833
ASSERT_EQ(5u, test_status.total_count_change);
18341834
ASSERT_EQ(eprosima::fastdds::dds::REJECTED_BY_SAMPLES_LIMIT, test_status.last_reason);
1835-
ASSERT_EQ(c_InstanceHandle_Unknown, test_status.last_instance_handle);
1835+
ASSERT_NE(c_InstanceHandle_Unknown, test_status.last_instance_handle);
18361836
}
18371837

18381838
/*!
@@ -1926,7 +1926,7 @@ TEST(DDSStatus, sample_rejected_key_re_dw_re_dr_keep_last_max_samples_2)
19261926
ASSERT_EQ(5u, test_status.total_count);
19271927
ASSERT_EQ(5u, test_status.total_count_change);
19281928
ASSERT_EQ(eprosima::fastdds::dds::REJECTED_BY_SAMPLES_LIMIT, test_status.last_reason);
1929-
ASSERT_EQ(c_InstanceHandle_Unknown, test_status.last_instance_handle);
1929+
ASSERT_NE(c_InstanceHandle_Unknown, test_status.last_instance_handle);
19301930
}
19311931

19321932
/*!
@@ -2026,7 +2026,7 @@ TEST(DDSStatus, sample_rejected_key_large_re_dw_re_dr_keep_last_max_samples_2)
20262026
ASSERT_EQ(5u, test_status.total_count);
20272027
ASSERT_EQ(5u, test_status.total_count_change);
20282028
ASSERT_EQ(eprosima::fastdds::dds::REJECTED_BY_SAMPLES_LIMIT, test_status.last_reason);
2029-
ASSERT_EQ(c_InstanceHandle_Unknown, test_status.last_instance_handle);
2029+
ASSERT_NE(c_InstanceHandle_Unknown, test_status.last_instance_handle);
20302030
}
20312031

20322032
/*!

0 commit comments

Comments
 (0)