Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[20583] DataReader::return_loan returns OK on loanable sequences without loans #4503

Merged
merged 4 commits into from
Apr 24, 2024
Merged

[20583] DataReader::return_loan returns OK on loanable sequences without loans #4503

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7664b01
Refs #20583: Add test and fix doxygen
EduPonz Mar 5, 2024
d7c67be
Refs #20583: return_loan returns OK when the loanable collection has …
EduPonz Mar 5, 2024
75c739f
Refs #20583: Update tests to new behaviour
EduPonz Mar 5, 2024
5498c31
Refs #20583: Add entry to versions.md
EduPonz Mar 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions include/fastdds/dds/subscriber/DataReader.hpp
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading