Skip to content

Commit 5b96410

Browse files
mergify[bot]richiwareemiliocuestaf
authored
Reset irrelevant sequence numbers interval in proxy readers (#6241) (#6258)
* Reset irrelevant sequence numbers interval in proxy readers (#6241) * Refs #24038. Regression test Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev> * Refs #24038. Fix Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev> * Refs #24038. Apply suggestion Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev> * Refs #24038. Apply new uncrustify Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev> --------- Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev> (cherry picked from commit 61f8677) # Conflicts: # test/blackbox/common/DDSBlackboxTestsContentFilter.cpp * Fix reusing_reader_proxy test Signed-off-by: Emilio Cuesta <emiliocuesta@eprosima.com> --------- Signed-off-by: Emilio Cuesta <emiliocuesta@eprosima.com> Co-authored-by: Ricardo González <ricardo@richiware.dev> Co-authored-by: Emilio Cuesta <emiliocuesta@eprosima.com>
1 parent 9de24a6 commit 5b96410

File tree

2 files changed

+117
-15
lines changed

2 files changed

+117
-15
lines changed

‎src/cpp/rtps/writer/ReaderProxy.cpp‎

Lines changed: 23 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,9 @@ void ReaderProxy::stop()
173173
next_expected_acknack_count_ = 0;
174174
last_nackfrag_count_ = 0;
175175
changes_low_mark_ = SequenceNumber_t();
176+
177+
first_irrelevant_removed_ = SequenceNumber_t::unknown();
178+
last_irrelevant_removed_ = SequenceNumber_t::unknown();
176179
}
177180

178181
void ReaderProxy::disable_timers()
@@ -348,14 +351,17 @@ bool ReaderProxy::change_is_unsent(
348351
SequenceNumber_t::unknown() != gap_seq)
349352
{
350353
// Check if the hole is due to irrelevant changes removed without informing the reader
351-
if (gap_seq == first_irrelevant_removed_)
352-
{
353-
first_irrelevant_removed_ = SequenceNumber_t::unknown();
354-
last_irrelevant_removed_ = SequenceNumber_t::unknown();
355-
}
356-
else if (gap_seq < last_irrelevant_removed_)
354+
if (first_irrelevant_removed_ <= gap_seq )
357355
{
358-
last_irrelevant_removed_ = gap_seq - 1;
356+
if (gap_seq == first_irrelevant_removed_)
357+
{
358+
first_irrelevant_removed_ = SequenceNumber_t::unknown();
359+
last_irrelevant_removed_ = SequenceNumber_t::unknown();
360+
}
361+
else if (gap_seq < last_irrelevant_removed_)
362+
{
363+
last_irrelevant_removed_ = gap_seq - 1;
364+
}
359365
}
360366
}
361367
}
@@ -470,14 +476,17 @@ bool ReaderProxy::requested_changes_set(
470476
if (SequenceNumber_t::unknown() != first_irrelevant_removed_)
471477
{
472478
// Check if the hole is due to irrelevant changes removed without informing the reader
473-
if (sit == first_irrelevant_removed_)
474-
{
475-
first_irrelevant_removed_ = SequenceNumber_t::unknown();
476-
last_irrelevant_removed_ = SequenceNumber_t::unknown();
477-
}
478-
else if (sit < last_irrelevant_removed_)
479+
if (first_irrelevant_removed_ <= sit )
479480
{
480-
last_irrelevant_removed_ = sit - 1;
481+
if (sit == first_irrelevant_removed_)
482+
{
483+
first_irrelevant_removed_ = SequenceNumber_t::unknown();
484+
last_irrelevant_removed_ = SequenceNumber_t::unknown();
485+
}
486+
else if (sit < last_irrelevant_removed_)
487+
{
488+
last_irrelevant_removed_ = sit - 1;
489+
}
481490
}
482491
}
483492
}

‎test/blackbox/common/DDSBlackboxTestsContentFilter.cpp‎

Lines changed: 94 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -831,7 +831,7 @@ TEST_P(DDSContentFilter, CorrectGAPSendingTwoReader)
831831
{
832832
total_count_2 = status.total_count;
833833
}).init();
834-
ASSERT_TRUE(reader.isInitialized());
834+
ASSERT_TRUE(reader_2.isInitialized());
835835

836836
// Set up the writer
837837
PubSubWriter<HelloWorldPubSubType> writer(TEST_TOPIC_NAME);
@@ -903,6 +903,99 @@ TEST(DDSContentFilter, filter_other_type_name)
903903
ASSERT_EQ(dds::DomainParticipantFactory::get_instance()->delete_participant(participant), ReturnCode_t::RETCODE_OK);
904904
}
905905

906+
907+
/*
908+
* Regression test for https://eprosima.easyredmine.com/issues/24038
909+
*
910+
* This test checks that reusing a ReaderProxy object when a new DataReader is matched does not lead to incorrect
911+
* behaviour due to poorly initialised data.
912+
*/
913+
TEST(DDSContentFilter, reusing_reader_proxy)
914+
{
915+
int32_t total_count {0};
916+
int32_t total_count_2 {0};
917+
918+
registerHelloWorldTypes();
919+
920+
PubSubReader<HelloWorldPubSubType> reader(TEST_TOPIC_NAME, "index = 1 OR index = 2 OR index = 6", {}, true, false,
921+
false);
922+
reader
923+
.reliability(RELIABLE_RELIABILITY_QOS)
924+
.durability_kind(TRANSIENT_LOCAL_DURABILITY_QOS)
925+
.sample_lost_status_functor([&total_count](const SampleLostStatus& status)
926+
{
927+
total_count = status.total_count;
928+
}).init();
929+
ASSERT_TRUE(reader.isInitialized());
930+
931+
auto udp_transport = std::make_shared<UDPv4TransportDescriptor>();
932+
933+
// Set up the writer
934+
PubSubWriter<HelloWorldPubSubType> writer(TEST_TOPIC_NAME);
935+
writer
936+
.add_user_transport_to_pparams(udp_transport)
937+
.disable_builtin_transport()
938+
.durability_kind(TRANSIENT_LOCAL_DURABILITY_QOS)
939+
.history_depth(10)
940+
//.heartbeat_period_seconds(100)
941+
.init();
942+
ASSERT_TRUE(writer.isInitialized());
943+
944+
// Wait for discovery
945+
reader.wait_discovery();
946+
writer.wait_discovery();
947+
948+
// Send 10 samples
949+
auto data = default_helloworld_data_generator();
950+
951+
decltype(data) expected_data;
952+
expected_data.push_back(*data.begin()); // index 1
953+
expected_data.push_back(*std::next(data.begin())); // index 2
954+
expected_data.push_back(*std::next(data.begin(), 5)); // index 6
955+
decltype(data) expected_data_2;
956+
expected_data_2.push_back(*std::next(data.begin(), 8)); // index 9
957+
expected_data_2.push_back(*std::next(data.begin(), 2)); // index 3
958+
expected_data_2.push_back(*std::next(data.begin(), 3)); // index 4
959+
960+
reader.startReception(expected_data);
961+
962+
writer.send(data, 50);
963+
964+
// Wait for reception and check
965+
reader.block_for_all();
966+
ASSERT_EQ(0, total_count);
967+
968+
reader.destroy();
969+
970+
data = default_helloworld_data_generator(4);
971+
972+
writer.send(data, 50);
973+
974+
PubSubReader<HelloWorldPubSubType> reader_2(TEST_TOPIC_NAME, "index = 3 OR index = 4 OR index = 9", {}, true, false,
975+
false);
976+
reader_2
977+
.reliability(RELIABLE_RELIABILITY_QOS)
978+
.durability_kind(TRANSIENT_LOCAL_DURABILITY_QOS)
979+
.sample_lost_status_functor([&total_count_2](const SampleLostStatus& status)
980+
{
981+
total_count_2 += status.total_count;
982+
}).init();
983+
ASSERT_TRUE(reader_2.isInitialized());
984+
985+
986+
reader_2.wait_discovery();
987+
writer.wait_discovery();
988+
989+
data = default_helloworld_data_generator(1);
990+
991+
writer.send(data, 50);
992+
993+
reader_2.startReception(expected_data_2);
994+
995+
// Wait for reception and check
996+
reader_2.block_for_all();
997+
}
998+
906999
#ifdef INSTANTIATE_TEST_SUITE_P
9071000
#define GTEST_INSTANTIATE_TEST_MACRO(x, y, z, w) INSTANTIATE_TEST_SUITE_P(x, y, z, w)
9081001
#else

0 commit comments

Comments
 (0)