From 958767d39f1c09dbe0bc2399b305cd79cf6dd172 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Dom=C3=ADnguez=20L=C3=B3pez?= <116071334+Mario-DL@users.noreply.github.com> Date: Fri, 31 May 2024 09:58:16 +0200 Subject: [PATCH 1/2] Correctly call `on_liveliness_changed` when there are multiple readers on the same topic (#4822) * Refs #21065: Add BB test Signed-off-by: Mario Dominguez * Refs #21065: Add fix rework after sync with Miguel Signed-off-by: Mario Dominguez * REfs #21065: Correct windows NIT Signed-off-by: Mario Dominguez * Refs #21065: Apply NIT Signed-off-by: Mario Dominguez --------- Signed-off-by: Mario Dominguez (cherry picked from commit df909438f4442afabc332364dba74c1612209c15) # Conflicts: # include/fastdds/rtps/writer/LivelinessManager.h # src/cpp/rtps/writer/LivelinessManager.cpp # test/unittest/rtps/writer/LivelinessManagerTests.cpp --- .../fastdds/rtps/writer/LivelinessManager.h | 13 ++++-- src/cpp/rtps/builtin/liveliness/WLP.cpp | 8 +++- src/cpp/rtps/reader/StatefulReader.cpp | 14 ++++++- src/cpp/rtps/reader/StatelessReader.cpp | 13 +++++- src/cpp/rtps/writer/LivelinessManager.cpp | 23 ++++------- .../common/BlackboxTestsLivelinessQos.cpp | 41 +++++++++++++++++++ .../rtps/writer/LivelinessManagerTests.cpp | 31 ++++++++++++++ 7 files changed, 121 insertions(+), 22 deletions(-) diff --git a/include/fastdds/rtps/writer/LivelinessManager.h b/include/fastdds/rtps/writer/LivelinessManager.h index a8efa9fecfd..a714a918f71 100644 --- a/include/fastdds/rtps/writer/LivelinessManager.h +++ b/include/fastdds/rtps/writer/LivelinessManager.h @@ -82,15 +82,22 @@ class LivelinessManager /** * @brief Removes a writer - * @param guid GUID of the writer - * @param kind Liveliness kind - * @param lease_duration Liveliness lease duration + * @param [in] guid GUID of the writer + * @param [in] kind Liveliness kind + * @param [in] lease_duration Liveliness lease duration + * @param [out] writer_liveliness_status The liveliness status of the writer * @return True if the writer was successfully removed */ bool remove_writer( GUID_t guid, +<<<<<<< HEAD:include/fastdds/rtps/writer/LivelinessManager.h LivelinessQosPolicyKind kind, Duration_t lease_duration); +======= + fastdds::dds::LivelinessQosPolicyKind kind, + Duration_t lease_duration, + LivelinessData::WriterStatus& writer_liveliness_status); +>>>>>>> df909438f (Correctly call `on_liveliness_changed` when there are multiple readers on the same topic (#4822)):src/cpp/rtps/writer/LivelinessManager.hpp /** * @brief Asserts liveliness of a writer in the set diff --git a/src/cpp/rtps/builtin/liveliness/WLP.cpp b/src/cpp/rtps/builtin/liveliness/WLP.cpp index b8f0a15d849..246b9bab455 100644 --- a/src/cpp/rtps/builtin/liveliness/WLP.cpp +++ b/src/cpp/rtps/builtin/liveliness/WLP.cpp @@ -704,6 +704,8 @@ bool WLP::remove_local_writer( logInfo(RTPS_LIVELINESS, W->getGuid().entityId << " from Liveliness Protocol"); + LivelinessData::WriterStatus writer_status; + if (W->get_liveliness_kind() == AUTOMATIC_LIVELINESS_QOS) { auto it = std::find( @@ -756,7 +758,8 @@ bool WLP::remove_local_writer( if (!pub_liveliness_manager_->remove_writer( W->getGuid(), W->get_liveliness_kind(), - W->get_liveliness_lease_duration())) + W->get_liveliness_lease_duration(), + writer_status)) { logError(RTPS_LIVELINESS, "Could not remove writer " << W->getGuid() << " from liveliness manager"); } @@ -798,7 +801,8 @@ bool WLP::remove_local_writer( if (!pub_liveliness_manager_->remove_writer( W->getGuid(), W->get_liveliness_kind(), - W->get_liveliness_lease_duration())) + W->get_liveliness_lease_duration(), + writer_status)) { logError(RTPS_LIVELINESS, "Could not remove writer " << W->getGuid() << " from liveliness manager"); } diff --git a/src/cpp/rtps/reader/StatefulReader.cpp b/src/cpp/rtps/reader/StatefulReader.cpp index 9845484e4b7..1e295892433 100644 --- a/src/cpp/rtps/reader/StatefulReader.cpp +++ b/src/cpp/rtps/reader/StatefulReader.cpp @@ -343,10 +343,22 @@ bool StatefulReader::matched_writer_remove( auto wlp = this->mp_RTPSParticipant->wlp(); if ( wlp != nullptr) { + LivelinessData::WriterStatus writer_liveliness_status; wlp->sub_liveliness_manager_->remove_writer( writer_guid, liveliness_kind_, - liveliness_lease_duration_); + liveliness_lease_duration_, + writer_liveliness_status); + + if (writer_liveliness_status == LivelinessData::WriterStatus::ALIVE) + { + wlp->update_liveliness_changed_status(writer_guid, this, -1, 0); + } + else if (writer_liveliness_status == LivelinessData::WriterStatus::NOT_ALIVE) + { + wlp->update_liveliness_changed_status(writer_guid, this, 0, -1); + } + } else { diff --git a/src/cpp/rtps/reader/StatelessReader.cpp b/src/cpp/rtps/reader/StatelessReader.cpp index 9e13919a9c2..7c327ecee05 100644 --- a/src/cpp/rtps/reader/StatelessReader.cpp +++ b/src/cpp/rtps/reader/StatelessReader.cpp @@ -194,10 +194,21 @@ bool StatelessReader::matched_writer_remove( auto wlp = mp_RTPSParticipant->wlp(); if ( wlp != nullptr) { + LivelinessData::WriterStatus writer_liveliness_status; wlp->sub_liveliness_manager_->remove_writer( writer_guid, liveliness_kind_, - liveliness_lease_duration_); + liveliness_lease_duration_, + writer_liveliness_status); + + if (writer_liveliness_status == LivelinessData::WriterStatus::ALIVE) + { + wlp->update_liveliness_changed_status(writer_guid, this, -1, 0); + } + else if (writer_liveliness_status == LivelinessData::WriterStatus::NOT_ALIVE) + { + wlp->update_liveliness_changed_status(writer_guid, this, 0, -1); + } } else { diff --git a/src/cpp/rtps/writer/LivelinessManager.cpp b/src/cpp/rtps/writer/LivelinessManager.cpp index fa44155e4fe..397ac102c50 100644 --- a/src/cpp/rtps/writer/LivelinessManager.cpp +++ b/src/cpp/rtps/writer/LivelinessManager.cpp @@ -91,11 +91,16 @@ bool LivelinessManager::add_writer( bool LivelinessManager::remove_writer( GUID_t guid, +<<<<<<< HEAD LivelinessQosPolicyKind kind, Duration_t lease_duration) +======= + fastdds::dds::LivelinessQosPolicyKind kind, + Duration_t lease_duration, + LivelinessData::WriterStatus& writer_status) +>>>>>>> df909438f (Correctly call `on_liveliness_changed` when there are multiple readers on the same topic (#4822)) { bool removed = false; - LivelinessData::WriterStatus status; { // collection guard @@ -103,9 +108,9 @@ bool LivelinessManager::remove_writer( // writers_ elements guard std::lock_guard __(mutex_); - removed = writers_.remove_if([guid, kind, lease_duration, &status](LivelinessData& writer) + removed = writers_.remove_if([guid, kind, lease_duration, &writer_status](LivelinessData& writer) { - status = writer.status; + writer_status = writer.status; return writer.guid == guid && writer.kind == kind && writer.lease_duration == lease_duration && @@ -118,18 +123,6 @@ bool LivelinessManager::remove_writer( return false; } - if (callback_ != nullptr) - { - if (status == LivelinessData::WriterStatus::ALIVE) - { - callback_(guid, kind, lease_duration, -1, 0); - } - else if (status == LivelinessData::WriterStatus::NOT_ALIVE) - { - callback_(guid, kind, lease_duration, 0, -1); - } - } - std::unique_lock lock(mutex_); if (timer_owner_ != nullptr) diff --git a/test/blackbox/common/BlackboxTestsLivelinessQos.cpp b/test/blackbox/common/BlackboxTestsLivelinessQos.cpp index 662cf66ec4e..14aab586e0b 100644 --- a/test/blackbox/common/BlackboxTestsLivelinessQos.cpp +++ b/test/blackbox/common/BlackboxTestsLivelinessQos.cpp @@ -1998,6 +1998,47 @@ TEST(LivelinessTests, Reader_Successfully_Asserts_Liveliness_on_a_Disconnected_W ASSERT_EQ(reader.sub_wait_liveliness_lost_for(1, std::chrono::seconds(4)), 1u); } +// Regression test of Refs #21065, github issue #4610 +TEST(LivelinessTests, correct_liveliness_state_one_writer_multiple_readers) +{ + uint8_t num_readers = 2; + + // Create one writer participant + PubSubWriter writer(TEST_TOPIC_NAME); + + // Create a reader participant containing 2 readers + PubSubParticipant reader(0, num_readers, 0, num_readers); + + reader.init_participant(); + // Define the reader's lease duration in 1.6 secs + reader.sub_liveliness_lease_duration(eprosima::fastrtps::Time_t(1, 600000000)); + // Both readers on the same topic + reader.sub_topic_name(TEST_TOPIC_NAME); + + for (size_t i = 0; i < num_readers; i++) + { + // Create Subscribers and readers, one for each writer + reader.init_subscriber(static_cast(i)); + } + + // Create writers + writer.lease_duration(c_TimeInfinite, 1) + .liveliness_lease_duration(eprosima::fastrtps::Time_t(1, 0)) + .liveliness_kind(eprosima::fastdds::dds::AUTOMATIC_LIVELINESS_QOS) + .liveliness_announcement_period(eprosima::fastrtps::Time_t(0, 500000000)) + .init(); + + // Wait for discovery to occur. Liveliness should be recovered twice, + // one per matched reader. + reader.sub_wait_liveliness_recovered(2); + + // Destroy the writer + writer.destroy(); + + // After 1.6 secs, we should receive a on_liveliness_changed(status lost) on the two readers + ASSERT_EQ(reader.sub_wait_liveliness_lost_for(2, std::chrono::seconds(4)), 2u); +} + #ifdef INSTANTIATE_TEST_SUITE_P #define GTEST_INSTANTIATE_TEST_MACRO(x, y, z, w) INSTANTIATE_TEST_SUITE_P(x, y, z, w) #else diff --git a/test/unittest/rtps/writer/LivelinessManagerTests.cpp b/test/unittest/rtps/writer/LivelinessManagerTests.cpp index 03ed08ababb..254a9d32ee3 100644 --- a/test/unittest/rtps/writer/LivelinessManagerTests.cpp +++ b/test/unittest/rtps/writer/LivelinessManagerTests.cpp @@ -152,7 +152,9 @@ TEST_F(LivelinessManagerTests, WriterCannotBeRemovedTwice) GuidPrefix_t guidP; guidP.value[0] = 1; GUID_t guid(guidP, 0); + LivelinessData::WriterStatus writer_status; +<<<<<<< HEAD EXPECT_EQ(liveliness_manager.add_writer(guid, AUTOMATIC_LIVELINESS_QOS, Duration_t(1)), true); EXPECT_EQ(liveliness_manager.remove_writer(guid, AUTOMATIC_LIVELINESS_QOS, Duration_t(1)), true); EXPECT_EQ(liveliness_manager.remove_writer(guid, AUTOMATIC_LIVELINESS_QOS, Duration_t(1)), false); @@ -164,6 +166,28 @@ TEST_F(LivelinessManagerTests, WriterCannotBeRemovedTwice) EXPECT_EQ(liveliness_manager.add_writer(guid, MANUAL_BY_TOPIC_LIVELINESS_QOS, Duration_t(1)), true); EXPECT_EQ(liveliness_manager.remove_writer(guid, MANUAL_BY_TOPIC_LIVELINESS_QOS, Duration_t(1)), true); EXPECT_EQ(liveliness_manager.remove_writer(guid, MANUAL_BY_TOPIC_LIVELINESS_QOS, Duration_t(1)), false); +======= + EXPECT_EQ(liveliness_manager.add_writer(guid, fastdds::dds::AUTOMATIC_LIVELINESS_QOS, Duration_t(1)), true); + EXPECT_EQ(liveliness_manager.remove_writer(guid, fastdds::dds::AUTOMATIC_LIVELINESS_QOS, Duration_t(1), + writer_status), true); + EXPECT_EQ(liveliness_manager.remove_writer(guid, fastdds::dds::AUTOMATIC_LIVELINESS_QOS, Duration_t(1), + writer_status), false); + + EXPECT_EQ(liveliness_manager.add_writer(guid, fastdds::dds::MANUAL_BY_PARTICIPANT_LIVELINESS_QOS, Duration_t( + 1)), true); + EXPECT_EQ(liveliness_manager.remove_writer(guid, fastdds::dds::MANUAL_BY_PARTICIPANT_LIVELINESS_QOS, Duration_t( + 1), writer_status), true); + EXPECT_EQ(liveliness_manager.remove_writer(guid, fastdds::dds::MANUAL_BY_PARTICIPANT_LIVELINESS_QOS, Duration_t( + 1), writer_status), false); + + EXPECT_EQ(liveliness_manager.add_writer(guid, fastdds::dds::MANUAL_BY_TOPIC_LIVELINESS_QOS, Duration_t(1)), true); + EXPECT_EQ(liveliness_manager.remove_writer(guid, fastdds::dds::MANUAL_BY_TOPIC_LIVELINESS_QOS, Duration_t(1), + writer_status), + true); + EXPECT_EQ(liveliness_manager.remove_writer(guid, fastdds::dds::MANUAL_BY_TOPIC_LIVELINESS_QOS, Duration_t(1), + writer_status), + false); +>>>>>>> df909438f (Correctly call `on_liveliness_changed` when there are multiple readers on the same topic (#4822)) } //! Tests that the assert_liveliness() method that takes liveliness kind as argument sets the alive state and time @@ -479,12 +503,19 @@ TEST_F(LivelinessManagerTests, TimerOwnerRemoved) GuidPrefix_t guidP; guidP.value[0] = 1; + LivelinessData::WriterStatus writer_status; liveliness_manager.add_writer(GUID_t(guidP, 1), AUTOMATIC_LIVELINESS_QOS, Duration_t(0.5)); liveliness_manager.add_writer(GUID_t(guidP, 2), AUTOMATIC_LIVELINESS_QOS, Duration_t(1)); +<<<<<<< HEAD liveliness_manager.assert_liveliness(GUID_t(guidP, 1), AUTOMATIC_LIVELINESS_QOS, Duration_t(0.5)); liveliness_manager.remove_writer(GUID_t(guidP, 1), AUTOMATIC_LIVELINESS_QOS, Duration_t(0.5)); +======= + liveliness_manager.assert_liveliness(GUID_t(guidP, 1), fastdds::dds::AUTOMATIC_LIVELINESS_QOS, Duration_t(0.5)); + liveliness_manager.remove_writer(GUID_t(guidP, 1), fastdds::dds::AUTOMATIC_LIVELINESS_QOS, Duration_t( + 0.5), writer_status); +>>>>>>> df909438f (Correctly call `on_liveliness_changed` when there are multiple readers on the same topic (#4822)) wait_liveliness_lost(1u); EXPECT_EQ(writer_losing_liveliness, GUID_t(guidP, 2)); From 6822b664c1b9053d6c4f6948e6178cc02986ccd9 Mon Sep 17 00:00:00 2001 From: Mario Dominguez Date: Tue, 4 Jun 2024 10:28:06 +0200 Subject: [PATCH 2/2] Solve conflicts Signed-off-by: Mario Dominguez --- .../fastdds/rtps/writer/LivelinessManager.h | 5 ----- src/cpp/rtps/writer/LivelinessManager.cpp | 5 ----- .../rtps/writer/LivelinessManagerTests.cpp | 19 ------------------- 3 files changed, 29 deletions(-) diff --git a/include/fastdds/rtps/writer/LivelinessManager.h b/include/fastdds/rtps/writer/LivelinessManager.h index a714a918f71..7c2d8bbe5f0 100644 --- a/include/fastdds/rtps/writer/LivelinessManager.h +++ b/include/fastdds/rtps/writer/LivelinessManager.h @@ -90,14 +90,9 @@ class LivelinessManager */ bool remove_writer( GUID_t guid, -<<<<<<< HEAD:include/fastdds/rtps/writer/LivelinessManager.h - LivelinessQosPolicyKind kind, - Duration_t lease_duration); -======= fastdds::dds::LivelinessQosPolicyKind kind, Duration_t lease_duration, LivelinessData::WriterStatus& writer_liveliness_status); ->>>>>>> df909438f (Correctly call `on_liveliness_changed` when there are multiple readers on the same topic (#4822)):src/cpp/rtps/writer/LivelinessManager.hpp /** * @brief Asserts liveliness of a writer in the set diff --git a/src/cpp/rtps/writer/LivelinessManager.cpp b/src/cpp/rtps/writer/LivelinessManager.cpp index 397ac102c50..ce338b708d3 100644 --- a/src/cpp/rtps/writer/LivelinessManager.cpp +++ b/src/cpp/rtps/writer/LivelinessManager.cpp @@ -91,14 +91,9 @@ bool LivelinessManager::add_writer( bool LivelinessManager::remove_writer( GUID_t guid, -<<<<<<< HEAD - LivelinessQosPolicyKind kind, - Duration_t lease_duration) -======= fastdds::dds::LivelinessQosPolicyKind kind, Duration_t lease_duration, LivelinessData::WriterStatus& writer_status) ->>>>>>> df909438f (Correctly call `on_liveliness_changed` when there are multiple readers on the same topic (#4822)) { bool removed = false; diff --git a/test/unittest/rtps/writer/LivelinessManagerTests.cpp b/test/unittest/rtps/writer/LivelinessManagerTests.cpp index 254a9d32ee3..09f2571a114 100644 --- a/test/unittest/rtps/writer/LivelinessManagerTests.cpp +++ b/test/unittest/rtps/writer/LivelinessManagerTests.cpp @@ -154,19 +154,6 @@ TEST_F(LivelinessManagerTests, WriterCannotBeRemovedTwice) GUID_t guid(guidP, 0); LivelinessData::WriterStatus writer_status; -<<<<<<< HEAD - EXPECT_EQ(liveliness_manager.add_writer(guid, AUTOMATIC_LIVELINESS_QOS, Duration_t(1)), true); - EXPECT_EQ(liveliness_manager.remove_writer(guid, AUTOMATIC_LIVELINESS_QOS, Duration_t(1)), true); - EXPECT_EQ(liveliness_manager.remove_writer(guid, AUTOMATIC_LIVELINESS_QOS, Duration_t(1)), false); - - EXPECT_EQ(liveliness_manager.add_writer(guid, MANUAL_BY_PARTICIPANT_LIVELINESS_QOS, Duration_t(1)), true); - EXPECT_EQ(liveliness_manager.remove_writer(guid, MANUAL_BY_PARTICIPANT_LIVELINESS_QOS, Duration_t(1)), true); - EXPECT_EQ(liveliness_manager.remove_writer(guid, MANUAL_BY_PARTICIPANT_LIVELINESS_QOS, Duration_t(1)), false); - - EXPECT_EQ(liveliness_manager.add_writer(guid, MANUAL_BY_TOPIC_LIVELINESS_QOS, Duration_t(1)), true); - EXPECT_EQ(liveliness_manager.remove_writer(guid, MANUAL_BY_TOPIC_LIVELINESS_QOS, Duration_t(1)), true); - EXPECT_EQ(liveliness_manager.remove_writer(guid, MANUAL_BY_TOPIC_LIVELINESS_QOS, Duration_t(1)), false); -======= EXPECT_EQ(liveliness_manager.add_writer(guid, fastdds::dds::AUTOMATIC_LIVELINESS_QOS, Duration_t(1)), true); EXPECT_EQ(liveliness_manager.remove_writer(guid, fastdds::dds::AUTOMATIC_LIVELINESS_QOS, Duration_t(1), writer_status), true); @@ -187,7 +174,6 @@ TEST_F(LivelinessManagerTests, WriterCannotBeRemovedTwice) EXPECT_EQ(liveliness_manager.remove_writer(guid, fastdds::dds::MANUAL_BY_TOPIC_LIVELINESS_QOS, Duration_t(1), writer_status), false); ->>>>>>> df909438f (Correctly call `on_liveliness_changed` when there are multiple readers on the same topic (#4822)) } //! Tests that the assert_liveliness() method that takes liveliness kind as argument sets the alive state and time @@ -508,14 +494,9 @@ TEST_F(LivelinessManagerTests, TimerOwnerRemoved) liveliness_manager.add_writer(GUID_t(guidP, 1), AUTOMATIC_LIVELINESS_QOS, Duration_t(0.5)); liveliness_manager.add_writer(GUID_t(guidP, 2), AUTOMATIC_LIVELINESS_QOS, Duration_t(1)); -<<<<<<< HEAD - liveliness_manager.assert_liveliness(GUID_t(guidP, 1), AUTOMATIC_LIVELINESS_QOS, Duration_t(0.5)); - liveliness_manager.remove_writer(GUID_t(guidP, 1), AUTOMATIC_LIVELINESS_QOS, Duration_t(0.5)); -======= liveliness_manager.assert_liveliness(GUID_t(guidP, 1), fastdds::dds::AUTOMATIC_LIVELINESS_QOS, Duration_t(0.5)); liveliness_manager.remove_writer(GUID_t(guidP, 1), fastdds::dds::AUTOMATIC_LIVELINESS_QOS, Duration_t( 0.5), writer_status); ->>>>>>> df909438f (Correctly call `on_liveliness_changed` when there are multiple readers on the same topic (#4822)) wait_liveliness_lost(1u); EXPECT_EQ(writer_losing_liveliness, GUID_t(guidP, 2));