From f0ce76c36aaa514aa65c2f5d5100e9f16b2085cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ricardo=20Gonz=C3=A1lez?= Date: Fri, 17 Mar 2023 08:02:55 +0100 Subject: [PATCH] Fix crash when disable_positive_acks is enable and the remote reader is best-effort (#3324) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Refs #17332. Add regression test Signed-off-by: Ricardo González Moreno * Refs #17332. Fix the crash. Moved block of code before calling flowcontroller::add_new_sample() avoiding the usage of a removed CacheChange_t pointer. Signed-off-by: Ricardo González Moreno --------- Signed-off-by: Ricardo González Moreno (cherry picked from commit 56f19b040b04a7e14bd9dedd5f52903b8f9194df) --- src/cpp/rtps/writer/StatefulWriter.cpp | 20 ++++++----- .../common/BlackboxTestsAcknackQos.cpp | 34 +++++++++++++++++++ 2 files changed, 45 insertions(+), 9 deletions(-) diff --git a/src/cpp/rtps/writer/StatefulWriter.cpp b/src/cpp/rtps/writer/StatefulWriter.cpp index f68e7e5f03c..67ee83c8fec 100644 --- a/src/cpp/rtps/writer/StatefulWriter.cpp +++ b/src/cpp/rtps/writer/StatefulWriter.cpp @@ -435,15 +435,6 @@ void StatefulWriter::unsent_change_added_to_history( } ); - if (should_be_sent) - { - flow_controller_->add_new_sample(this, change, max_blocking_time); - } - else - { - periodic_hb_event_->restart_timer(max_blocking_time); - } - if (disable_positive_acks_) { auto source_timestamp = system_clock::time_point() + nanoseconds(change->sourceTimestamp.to_ns()); @@ -454,6 +445,17 @@ void StatefulWriter::unsent_change_added_to_history( ack_event_->update_interval_millisec((double)duration_cast(interval).count()); ack_event_->restart_timer(max_blocking_time); } + + // After adding the CacheChange_t to flowcontroller, its pointer cannot be used because it may be removed + // internally before exiting the call. For example if the writer matched with a best-effort reader. + if (should_be_sent) + { + flow_controller_->add_new_sample(this, change, max_blocking_time); + } + else + { + periodic_hb_event_->restart_timer(max_blocking_time); + } } else { diff --git a/test/blackbox/common/BlackboxTestsAcknackQos.cpp b/test/blackbox/common/BlackboxTestsAcknackQos.cpp index 0bd9ceb7db2..a977a53f152 100644 --- a/test/blackbox/common/BlackboxTestsAcknackQos.cpp +++ b/test/blackbox/common/BlackboxTestsAcknackQos.cpp @@ -160,3 +160,37 @@ TEST(AcknackQos, NotRecoverAfterLosingCommunicationWithDisablePositiveAck) // Block reader until reception finished or timeout. ASSERT_EQ(reader.block_for_all(std::chrono::seconds(1)), 0u); } + +/*! + * @test Regresion test for Github #3323. + */ +TEST(AcknackQos, DisablePositiveAcksWithBestEffortReader) +{ + PubSubReader reader(TEST_TOPIC_NAME); + PubSubWriter writer(TEST_TOPIC_NAME); + + writer.keep_duration({2, 0}); + writer.reliability(eprosima::fastrtps::RELIABLE_RELIABILITY_QOS); + writer.durability_kind(eprosima::fastrtps::VOLATILE_DURABILITY_QOS); + writer.init(); + + reader.keep_duration({1, 0}); + reader.reliability(eprosima::fastrtps::BEST_EFFORT_RELIABILITY_QOS); + reader.init(); + + ASSERT_TRUE(reader.isInitialized()); + ASSERT_TRUE(writer.isInitialized()); + + // Wait for discovery. + writer.wait_discovery(); + reader.wait_discovery(); + + std::list data = default_helloworld_data_generator(); + reader.startReception(data); + // Send data + writer.send(data); + // In this test all data should be sent. + ASSERT_TRUE(data.empty()); + // Block reader until reception finished or timeout. + reader.block_for_all(); +}