From 3c91d03f5598291bbeef9646d12f374047134f1e Mon Sep 17 00:00:00 2001 From: Eduardo Ponz Segrelles Date: Wed, 24 Apr 2024 07:16:58 +0200 Subject: [PATCH] DataReader::return_loan returns OK on loanable sequences without loans (#4503) * Refs #20583: Add test and fix doxygen Signed-off-by: EduPonz * Refs #20583: return_loan returns OK when the loanable collection has ownership (does not have loans) Signed-off-by: EduPonz * Refs #20583: Update tests to new behaviour Signed-off-by: EduPonz * Refs #20583: Add entry to versions.md Signed-off-by: EduPonz --------- Signed-off-by: EduPonz --- include/fastdds/dds/subscriber/DataReader.hpp | 6 +- src/cpp/fastdds/subscriber/DataReaderImpl.cpp | 4 +- .../dds/subscriber/DataReaderTests.cpp | 125 ++++++++++-------- versions.md | 2 +- 4 files changed, 77 insertions(+), 60 deletions(-) diff --git a/include/fastdds/dds/subscriber/DataReader.hpp b/include/fastdds/dds/subscriber/DataReader.hpp index fe086d7308..da723d0d6d 100644 --- a/include/fastdds/dds/subscriber/DataReader.hpp +++ b/include/fastdds/dds/subscriber/DataReader.hpp @@ -730,9 +730,9 @@ class DataReader : public DomainEntity * * The use of the @ref return_loan operation is only necessary if the read or take calls "loaned" buffers to the * application. This only occurs if the @c data_values and @c sample_infos collections had max_len == 0 - * at the time read or take was called. The application may also examine the @c owns property of the collection to - * determine if there is an outstanding loan. However, calling @ref return_loan on a collection that does not have - * a loan is safe and has no side effects. + * at the time read or take was called. The application may also examine the @c has_ownership property of the + * collection to determine if there is an outstanding loan. However, calling @ref return_loan on a collection that + * does not have a loan is safe, has no side effects, and returns RETCODE_OK. * * If the collections had a loan, upon return from return_loan the collections will have max_len == 0 . * diff --git a/src/cpp/fastdds/subscriber/DataReaderImpl.cpp b/src/cpp/fastdds/subscriber/DataReaderImpl.cpp index c059bf9845..11653e7ace 100644 --- a/src/cpp/fastdds/subscriber/DataReaderImpl.cpp +++ b/src/cpp/fastdds/subscriber/DataReaderImpl.cpp @@ -646,10 +646,10 @@ ReturnCode_t DataReaderImpl::return_loan( return RETCODE_PRECONDITION_NOT_MET; } - // They should have a loan + // If there is ownership that means there are no loans, case in which we just return OK if (data_values.has_ownership() == true) { - return RETCODE_PRECONDITION_NOT_MET; + return RETCODE_OK; } std::lock_guard lock(reader_->getMutex()); diff --git a/test/unittest/dds/subscriber/DataReaderTests.cpp b/test/unittest/dds/subscriber/DataReaderTests.cpp index 0b34fe9383..ad27cd2148 100644 --- a/test/unittest/dds/subscriber/DataReaderTests.cpp +++ b/test/unittest/dds/subscriber/DataReaderTests.cpp @@ -215,17 +215,19 @@ class DataReaderTests : public ::testing::Test * The return value to expect from `return_loan` will be calculated from this one as follows: * - NOT_ENABLED => NOT_ENABLED (calling `return_loan` on a not enabled reader). * - OK => OK (successfully returning a loan). - * - Any other => RETCODE_PRECONDITION_NOT_MET (trying to return collections which the reader - * did not loan). + * - Any other => RETCODE_PRECONDITION_NOT_MET (trying to return non-empty collections which + * the reader did not loan). * @param data_reader The reader on which to return the loan. * @param data_values The data collection to return. * @param infos The SampleInfo collection to return. + * @param seq_max The value to expect as `maximum` on the collections after return_loan returns OK. */ void check_return_loan( const ReturnCode_t& code, DataReader* data_reader, LoanableCollection& data_values, - SampleInfoSeq& infos) + SampleInfoSeq& infos, + int32_t seq_max) { ReturnCode_t expected_return_loan_ret = RETCODE_PRECONDITION_NOT_MET; if (RETCODE_OK == code || RETCODE_NOT_ENABLED == code) @@ -237,9 +239,9 @@ class DataReaderTests : public ::testing::Test if (RETCODE_OK == expected_return_loan_ret) { EXPECT_TRUE(data_values.has_ownership()); - EXPECT_EQ(0, data_values.maximum()); + EXPECT_EQ(seq_max, data_values.maximum()); EXPECT_TRUE(infos.has_ownership()); - EXPECT_EQ(0, infos.maximum()); + EXPECT_EQ(seq_max, infos.maximum()); } } @@ -261,10 +263,11 @@ class DataReaderTests : public ::testing::Test DataReader* data_reader, LoanableCollection& data_values, SampleInfoSeq& infos, - int32_t max_samples = LENGTH_UNLIMITED) + int32_t max_samples = LENGTH_UNLIMITED, + int32_t seq_max = 0) { EXPECT_EQ(instance_ok_code, data_reader->read_instance(data_values, infos, max_samples, handle)); - check_return_loan(loan_return_code, data_reader, data_values, infos); + check_return_loan(loan_return_code, data_reader, data_values, infos, seq_max); reset_lengths_if_ok(instance_ok_code, data_values, infos); EXPECT_EQ(instance_ok_code, data_reader->take_instance(data_values, infos, max_samples, handle)); @@ -273,7 +276,7 @@ class DataReaderTests : public ::testing::Test // Write received data so it can be taken again send_data(data_values, infos); } - check_return_loan(loan_return_code, data_reader, data_values, infos); + check_return_loan(loan_return_code, data_reader, data_values, infos, seq_max); reset_lengths_if_ok(instance_ok_code, data_values, infos); } @@ -295,12 +298,13 @@ class DataReaderTests : public ::testing::Test DataReader* data_reader, LoanableCollection& data_values, SampleInfoSeq& infos, - int32_t max_samples = LENGTH_UNLIMITED) + int32_t max_samples = LENGTH_UNLIMITED, + int32_t seq_max = 0) { EXPECT_EQ(instance_bad_code, data_reader->read_instance(data_values, infos, max_samples, handle)); - check_return_loan(wrong_loan_code, data_reader, data_values, infos); + check_return_loan(wrong_loan_code, data_reader, data_values, infos, seq_max); EXPECT_EQ(instance_bad_code, data_reader->take_instance(data_values, infos, max_samples, handle)); - check_return_loan(wrong_loan_code, data_reader, data_values, infos); + check_return_loan(wrong_loan_code, data_reader, data_values, infos, seq_max); } /** @@ -314,6 +318,7 @@ class DataReaderTests : public ::testing::Test * @param infos The sample_info collection to use * @param max_samples The value to pass as `max_samples` on calls to `read/take_instance` * @param two_valid_instances Whether `handle_wrong_` is considered a valid instance + * @param seq_max The value to expect as `maximum` on the collections after return_loan returns OK. */ void check_instance_methods( const ReturnCode_t& instance_ok_code, @@ -323,7 +328,8 @@ class DataReaderTests : public ::testing::Test LoanableCollection& data_values, SampleInfoSeq& infos, int32_t max_samples = LENGTH_UNLIMITED, - bool two_valid_instances = false) + bool two_valid_instances = false, + int32_t seq_max = 0) { // Calc expected result of `return_loan` for calls with a wrong instance handle. ReturnCode_t wrong_loan_code = RETCODE_PRECONDITION_NOT_MET; @@ -331,26 +337,30 @@ class DataReaderTests : public ::testing::Test { wrong_loan_code = instance_bad_code; } + else if (RETCODE_OK == loan_return_code) + { + wrong_loan_code = RETCODE_OK; + } // Trying to get data for HANDLE_NIL should always use instance_bad_code. check_wrong_instance_methods(HANDLE_NIL, instance_bad_code, wrong_loan_code, - data_reader, data_values, infos, max_samples); + data_reader, data_values, infos, max_samples, seq_max); // Trying to get data for handle_wrong_ depends on `two_instances` if (two_valid_instances) { check_correct_instance_methods(handle_wrong_, instance_ok_code, loan_return_code, - data_reader, data_values, infos, max_samples); + data_reader, data_values, infos, max_samples, seq_max); } else { check_wrong_instance_methods(handle_wrong_, instance_bad_code, wrong_loan_code, - data_reader, data_values, infos, max_samples); + data_reader, data_values, infos, max_samples, seq_max); } // Trying to get data for handle_ok_ should always use instance_ok_code check_correct_instance_methods(handle_ok_, instance_ok_code, loan_return_code, - data_reader, data_values, infos, max_samples); + data_reader, data_values, infos, max_samples, seq_max); } /** @@ -413,11 +423,18 @@ class DataReaderTests : public ::testing::Test DataSeq data_values; SampleInfoSeq infos; + ReturnCode_t expected_return_loan_ret = code; + if (RETCODE_NO_DATA == code) + { + // Even when read returns data, no loan will be performed + expected_return_loan_ret = RETCODE_OK; + } + EXPECT_EQ(code, data_reader->read(data_values, infos)); - check_return_loan(code, data_reader, data_values, infos); + check_return_loan(expected_return_loan_ret, data_reader, data_values, infos, 0); reset_lengths_if_ok(code, data_values, infos); EXPECT_EQ(code, data_reader->read_next_instance(data_values, infos)); - check_return_loan(code, data_reader, data_values, infos); + check_return_loan(expected_return_loan_ret, data_reader, data_values, infos, 0); reset_lengths_if_ok(code, data_values, infos); EXPECT_EQ(code, data_reader->take(data_values, infos)); @@ -426,7 +443,7 @@ class DataReaderTests : public ::testing::Test send_data(data_values, infos); data_reader->wait_for_unread_message(time_to_wait); } - check_return_loan(code, data_reader, data_values, infos); + check_return_loan(expected_return_loan_ret, data_reader, data_values, infos, 0); reset_lengths_if_ok(code, data_values, infos); EXPECT_EQ(code, data_reader->take_next_instance(data_values, infos)); @@ -435,11 +452,11 @@ class DataReaderTests : public ::testing::Test send_data(data_values, infos); data_reader->wait_for_unread_message(time_to_wait); } - check_return_loan(code, data_reader, data_values, infos); + check_return_loan(expected_return_loan_ret, data_reader, data_values, infos, 0); reset_lengths_if_ok(code, data_values, infos); - check_instance_methods(instance_ok_code, instance_bad_code, instance_ok_code, - data_reader, data_values, infos, LENGTH_UNLIMITED, two_valid_instances); + check_instance_methods(instance_ok_code, instance_bad_code, expected_return_loan_ret, + data_reader, data_values, infos, LENGTH_UNLIMITED, two_valid_instances, 0); } // Check read/take and variants without loan @@ -448,17 +465,16 @@ class DataReaderTests : public ::testing::Test SampleInfoSeq infos(1); ReturnCode_t expected_return_loan_ret = code; - if (RETCODE_OK == code) + if (RETCODE_NO_DATA == code) { - // Even when read returns data, no loan will be performed - expected_return_loan_ret = RETCODE_PRECONDITION_NOT_MET; + expected_return_loan_ret = RETCODE_OK; } EXPECT_EQ(code, data_reader->read(data_values, infos)); - check_return_loan(expected_return_loan_ret, data_reader, data_values, infos); + check_return_loan(expected_return_loan_ret, data_reader, data_values, infos, data_values.maximum()); reset_lengths_if_ok(code, data_values, infos); EXPECT_EQ(code, data_reader->read_next_instance(data_values, infos)); - check_return_loan(expected_return_loan_ret, data_reader, data_values, infos); + check_return_loan(expected_return_loan_ret, data_reader, data_values, infos, data_values.maximum()); reset_lengths_if_ok(code, data_values, infos); EXPECT_EQ(code, data_reader->take(data_values, infos)); @@ -467,7 +483,7 @@ class DataReaderTests : public ::testing::Test send_data(data_values, infos); data_reader->wait_for_unread_message(time_to_wait); } - check_return_loan(expected_return_loan_ret, data_reader, data_values, infos); + check_return_loan(expected_return_loan_ret, data_reader, data_values, infos, data_values.maximum()); reset_lengths_if_ok(code, data_values, infos); EXPECT_EQ(code, data_reader->take_next_instance(data_values, infos)); @@ -476,11 +492,11 @@ class DataReaderTests : public ::testing::Test send_data(data_values, infos); data_reader->wait_for_unread_message(time_to_wait); } - check_return_loan(expected_return_loan_ret, data_reader, data_values, infos); + check_return_loan(expected_return_loan_ret, data_reader, data_values, infos, data_values.maximum()); reset_lengths_if_ok(code, data_values, infos); check_instance_methods(instance_ok_code, instance_bad_code, expected_return_loan_ret, - data_reader, data_values, infos, LENGTH_UNLIMITED, two_valid_instances); + data_reader, data_values, infos, LENGTH_UNLIMITED, two_valid_instances, data_values.maximum()); } } @@ -813,15 +829,16 @@ void check_collection( * take_instance, where no instance would exist and the return will be BAD_PARAMETER. Otherwise, PRECONDITION_NOT_MET * should be returned. * - * As no data will ever be returned, return_loan will always return PRECONDITION_NOT_MET. + * As no data will ever be returned, return_loan will always return OK. */ TEST_F(DataReaderTests, collection_preconditions) { create_entities(); create_instance_handles(); - const ReturnCode_t& ok_code = RETCODE_NO_DATA; + const ReturnCode_t& no_data_code = RETCODE_NO_DATA; const ReturnCode_t& wrong_code = RETCODE_PRECONDITION_NOT_MET; + const ReturnCode_t& return_loan_code = RETCODE_OK; // Helper buffers to create loaned sequences FooArray arr; @@ -902,35 +919,35 @@ TEST_F(DataReaderTests, collection_preconditions) } // Check compatible combinations - using ok_test_case_t = std::pair; + using ok_test_case_t = std::pair>; std::vector ok_cases { // max == 0. Loaned data will be returned. - { {true_0_0, info_true_0_0}, ok_code}, + { {true_0_0, info_true_0_0}, {no_data_code, return_loan_code}}, // max > 0 && owns == true. Data will be copied. - { {true_10_0, info_true_10_0}, ok_code}, - { {true_10_1, info_true_10_1}, ok_code}, + { {true_10_0, info_true_10_0}, {no_data_code, return_loan_code}}, + { {true_10_1, info_true_10_1}, {no_data_code, return_loan_code}}, // max > 0 && owns == false. Precondition not met. - { {false_10_0, info_false_10_0}, wrong_code}, - { {false_10_1, info_false_10_1}, wrong_code} + { {false_10_0, info_false_10_0}, {wrong_code, wrong_code}}, + { {false_10_1, info_false_10_1}, {wrong_code, wrong_code}} }; const ReturnCode_t& instance_bad_code = RETCODE_BAD_PARAMETER; for (const ok_test_case_t& test : ok_cases) { - EXPECT_EQ(test.second, data_reader_->read(test.first.first, test.first.second)); - EXPECT_EQ(wrong_code, data_reader_->return_loan(test.first.first, test.first.second)); - EXPECT_EQ(test.second, data_reader_->read_next_instance(test.first.first, test.first.second)); - EXPECT_EQ(wrong_code, data_reader_->return_loan(test.first.first, test.first.second)); - EXPECT_EQ(test.second, data_reader_->take(test.first.first, test.first.second)); - EXPECT_EQ(wrong_code, data_reader_->return_loan(test.first.first, test.first.second)); - EXPECT_EQ(test.second, data_reader_->take_next_instance(test.first.first, test.first.second)); - EXPECT_EQ(wrong_code, data_reader_->return_loan(test.first.first, test.first.second)); + EXPECT_EQ(test.second.first, data_reader_->read(test.first.first, test.first.second)); + EXPECT_EQ(test.second.second, data_reader_->return_loan(test.first.first, test.first.second)); + EXPECT_EQ(test.second.first, data_reader_->read_next_instance(test.first.first, test.first.second)); + EXPECT_EQ(test.second.second, data_reader_->return_loan(test.first.first, test.first.second)); + EXPECT_EQ(test.second.first, data_reader_->take(test.first.first, test.first.second)); + EXPECT_EQ(test.second.second, data_reader_->return_loan(test.first.first, test.first.second)); + EXPECT_EQ(test.second.first, data_reader_->take_next_instance(test.first.first, test.first.second)); + EXPECT_EQ(test.second.second, data_reader_->return_loan(test.first.first, test.first.second)); // When collection preconditions are ok, as the reader has no data, BAD_PARAMETER will be returned - const ReturnCode_t& instance_code = (test.second == ok_code) ? instance_bad_code : test.second; - check_instance_methods(instance_code, instance_code, wrong_code, - data_reader_, test.first.first, test.first.second); + const ReturnCode_t& instance_code = (test.second.first == no_data_code) ? instance_bad_code : test.second.first; + check_instance_methods(instance_code, instance_code, test.second.second, + data_reader_, test.first.first, test.first.second, LENGTH_UNLIMITED, false, test.first.first.maximum()); } // Check for max_samples > max_len @@ -940,8 +957,8 @@ TEST_F(DataReaderTests, collection_preconditions) EXPECT_EQ(wrong_code, data_reader_->take(true_10_0, info_true_10_0, 20)); EXPECT_EQ(wrong_code, data_reader_->take_next_instance(true_10_0, info_true_10_0, 20)); - check_instance_methods(wrong_code, wrong_code, wrong_code, - data_reader_, true_10_0, info_true_10_0, 20); + check_instance_methods(wrong_code, wrong_code, return_loan_code, + data_reader_, true_10_0, info_true_10_0, 20, false, true_10_0.maximum()); } @@ -1002,6 +1019,9 @@ TEST_F(DataReaderTests, return_loan) EXPECT_EQ(ok_code, data_reader_->enable()); EXPECT_EQ(ok_code, reader2->enable()); + // Calling return loan with empty sequences on an enabled reader should return OK + EXPECT_EQ(RETCODE_OK, data_reader_->return_loan(data_values, infos)); + FooType data; data.index(0); @@ -1011,9 +1031,6 @@ TEST_F(DataReaderTests, return_loan) EXPECT_EQ(ok_code, data_writer_->write(&data, handle_ok_)); } - // Returning a loan without having called read or take should return PRECONDITION_NOT_MET - EXPECT_EQ(precondition_code, data_reader_->return_loan(data_values, infos)); - // Read with loan from both readers EXPECT_EQ(ok_code, data_reader_->read(data_values, infos)); EXPECT_EQ(ok_code, reader2->read(data_values_2, infos_2)); diff --git a/versions.md b/versions.md index d87808948b..dd41d9af90 100644 --- a/versions.md +++ b/versions.md @@ -25,11 +25,11 @@ Forthcoming * TypeLookupService * DBQueue * Added create participant methods that use environment XML profile for participant configuration. -* XML Parser API no longer public. * New TypeObjectRegistry class to register/query TypeObjects/TypeIdentifiers. * New TypeObjectUtils class providing API to build and register TypeObjects/TypeIdentifiers. * Refactor Dynamic Language Binding API according to OMG XTypes v1.3 specification. * Refactor ReturnCode complying with OMG DDS specification. +* Calling `DataReader::return_loan` returns `ReturnCode_t::RETCODE_OK` both for empty sequences and for sequences that were not loaned. Version 2.14.0 --------------