Skip to content

Commit

Permalink
DataReader::return_loan returns OK on loanable sequences without loans (
Browse files Browse the repository at this point in the history 
#4503)

* Refs #20583: Add test and fix doxygen

Signed-off-by: EduPonz <[email protected]>

* Refs #20583: return_loan returns OK when the loanable collection has ownership (does not have loans)

Signed-off-by: EduPonz <[email protected]>

* Refs #20583: Update tests to new behaviour

Signed-off-by: EduPonz <[email protected]>

* Refs #20583: Add entry to versions.md

Signed-off-by: EduPonz <[email protected]>

---------

Signed-off-by: EduPonz <[email protected]>
  • Loading branch information
@EduPonz
EduPonz authored Apr 24, 2024
1 parent bd83bb2 commit 3c91d03
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 60 deletions.
6 changes: 3 additions & 3 deletions include/fastdds/dds/subscriber/DataReader.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 <tt> max_len == 0 </tt>
* 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 <tt> max_len == 0 </tt>.
*
Expand Down
4 changes: 2 additions & 2 deletions src/cpp/fastdds/subscriber/DataReaderImpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<RecursiveTimedMutex> lock(reader_->getMutex());
Expand Down
125 changes: 71 additions & 54 deletions test/unittest/dds/subscriber/DataReaderTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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());
}
}

Expand All @@ -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));
Expand All @@ -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);
}

Expand All @@ -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);
}

/**
Expand All @@ -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,
Expand All @@ -323,34 +328,39 @@ 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;
if (RETCODE_NOT_ENABLED == instance_bad_code)
{
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);
}

/**
Expand Down Expand Up @@ -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));
Expand All @@ -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));
Expand All @@ -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
Expand All @@ -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));
Expand All @@ -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));
Expand All @@ -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());
}
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -902,35 +919,35 @@ TEST_F(DataReaderTests, collection_preconditions)
}

// Check compatible combinations
using ok_test_case_t = std::pair<test_case_t, const ReturnCode_t&>;
using ok_test_case_t = std::pair<test_case_t, std::pair<const ReturnCode_t&, const ReturnCode_t&>>;
std::vector<ok_test_case_t> 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
Expand All @@ -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());

}

Expand Down Expand Up @@ -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);

Expand All @@ -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));
Expand Down
2 changes: 1 addition & 1 deletion versions.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
--------------
Expand Down

0 comments on commit 3c91d03

Please sign in to comment.