[15158] ReadConditions Implementation #2872

MiguelBarro · 2022-07-22T08:17:45Z

Description

ReadCondition DDS 1.4 feature implementation & testing

Contributor Checklist

Reviewer Checklist

Check contributor checklist is correct.
Check CI results: changes do not issue any warning.
Check CI results: failing tests are unrelated with the changes.

jsan-rt · 2022-07-22T10:52:13Z

src/cpp/fastdds/subscriber/DataReaderImpl.cpp

@@ -304,6 +314,16 @@ DataReaderImpl::~DataReaderImpl()

 bool DataReaderImpl::can_be_deleted() const
 {
+    {
+        std::lock_guard<std::recursive_mutex> _(get_conditions_mutex());


This mutex's scope should cover the whole operation.

Acknowledged. To prevent any issues on notification the get_conditions_mutex() must be taken after the reader_->getMutex() one (doxygen on the mutex has been updated) because notification callbacks are done with the reader mutex taken.
Nevertheless the ReturnCode_t SubscriberImpl::delete_datareader(const DataReader* reader) will not take the mutex between DataReaderImpl::can_be_deleted() check and the actual deletion.
This happens too for the loans. A mechanism is need to disable all resource allocation within this method (flag or something) and assure can_be_deleted value will hold through it.

jsan-rt · 2022-07-22T11:24:01Z

test/unittest/dds/publisher/CMakeLists.txt

    ${PROJECT_SOURCE_DIR}/src/cpp/fastdds/core/policy/ParameterList.cpp
    ${PROJECT_SOURCE_DIR}/src/cpp/fastdds/core/policy/QosPolicyUtils.cpp
+    ${PROJECT_SOURCE_DIR}/src/cpp/fastdds/core/policy/QosPolicyUtils.cpp


Remove duplicated line

MiguelCompany · 2022-07-22T18:16:25Z

@MiguelBarro I think I got #2842 finally right. Would you mind rebasing this?

MiguelBarro · 2022-07-26T13:43:35Z

@richiprosima Please test this for me 🤯

MiguelCompany · 2022-07-26T19:20:27Z

@MiguelBarro #2842 has been merged. This one can be rebased onto master

jsan-rt · 2022-07-26T15:00:31Z

test/blackbox/common/DDSBlackboxTestsReadConditions.cpp

+    }
+    conds.clear();
+
+    // 4- Create several ReadConditions associated to different same masks


Different same masks? Please clarify.

jsan-rt · 2022-07-26T15:02:53Z

test/blackbox/common/DDSBlackboxTestsReadConditions.cpp

+        conds.push_front(reader.create_readcondition( ++sample_states, ++view_states, ++instance_states));
+    }
+
+    EXPECT_EQ(reader.delete_contained_entities(), ReturnCode_t::RETCODE_OK);


If the functionality of delete_contained_entities changed its tests should reflect this by adding new test cases.

all the possible cases are tested here:

Fast-DDS/test/unittest/dds/subscriber/DataReaderTests.cpp

Lines 2660 to 2665 in 6396f33

// 7- Check the DataReader cannot be deleted with outstanding conditions

EXPECT_EQ(subscriber_->delete_datareader(another_reader), ReturnCode_t::RETCODE_PRECONDITION_NOT_MET);

// but delete_contained_entities() succeeds with outstanding ReadConditions

EXPECT_EQ(another_reader->delete_contained_entities(), ReturnCode_t::RETCODE_OK);

// no outstanding conditions (killed above)

EXPECT_EQ(subscriber_->delete_datareader(another_reader), ReturnCode_t::RETCODE_OK);

I was referring to adding ReadConditions deletion to this test. I think that's where the delete_contained_entities functionality tests should go (even if you are using and checking its return code elsewhere)

jsan-rt · 2022-07-26T15:13:34Z

test/blackbox/common/DDSBlackboxTestsReadConditions.cpp


 #include <fastdds/dds/subscriber/ReadCondition.hpp>
+#include <fastdds/dds/core/StackAllocatedSequence.hpp>


As per the style guide, this line should come before the previous one.

this file has been removed in favor of DataReaderTests.cpp

jsan-rt · 2022-07-26T20:01:30Z

test/unittest/dds/subscriber/DataReaderTests.cpp

-        InstanceStateMask instance_states = ANY_INSTANCE_STATE;
-        EXPECT_EQ(
-            nullptr,
-            data_reader->create_readcondition(sample_states, view_states, instance_states));


Remove create_readcondition from the list of unimplemented methods in this test comment.

jsan-rt · 2022-07-26T20:02:01Z

test/unittest/dds/subscriber/DataReaderTests.cpp

-    {
-        EXPECT_EQ(
-            ReturnCode_t::RETCODE_UNSUPPORTED,
-            data_reader->delete_readcondition(nullptr));


Remove delete_readcondition from the list of unimplemented methods in this test comment.

jsan-rt · 2022-07-26T20:05:49Z

test/blackbox/common/DDSBlackboxTestsReadConditions.cpp

+    EXPECT_EQ(reader.delete_readcondition(cond), ReturnCode_t::RETCODE_PRECONDITION_NOT_MET);
+
+    // 7- Check the DataReader cannot be deleted with outstanding conditions
+    EXPECT_EQ(another_manager.get_native_subscriber().delete_contained_entities(),


I don't think the existence of ReadConditions should cause the delete_contained_entities call to fail.

a) ReturnCode_t DataReaderImpl::delete_contained_entities()

My implementation clears all ReadConditions (we can do that because Company implementation unlinks the WaitSets on Condition destruction.

According to the DDS standard 1.4 in 2.2.2.2.1.18 & 2.2.2.5.3.31 delete_contained_entities() will be used recursively and remove all API created objects (including Read & QueryConditions object created by the DataReader).

The operation will return PRECONDITION_NOT_MET if the any of the contained entities is in a state where it cannot be deleted. This is quite vague but seems our DataReaderImpl fulfills the spirit.

b) ReturnCode_t SubscriberImpl::delete_contained_entities()

The implementation tries to destroy all DataReaders checking DataReaderImpl::can_be_deleted().
My can_be_deleted() implementation takes into account outstanding ReadConditions (formerly only loans).
This doesn't match the DDS standard 2.2.2.5.2.14 behaviour.

c) ReturnCode_t SubscriberImpl::delete_datareader(const DataReader* reader)
The implementation tries to destroy all the readers querying can_be_deleted() too.
This matches standard 2.2.2.5.2.6 description (any outstanding ReadConditions will lead to failure).

It's clear the issue is b). b) and c) are supposed to behave differently but are calling the same DataReaderImpl::can_be_deleted()
method. The solution is make clear to the method which function is calling it by using a parameter as:

bool DataReaderImpl::can_be_deleted(bool recursive = true) const

jsan-rt · 2022-07-26T20:06:57Z

test/blackbox/common/DDSBlackboxTestsReadConditions.cpp

+    // Check the data is there
+    EXPECT_EQ(reader.get_native_reader().get_unread_count(), 1);
+
+    // Check the conditions triggered where the expected ones


typo: Replace where with were

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

…ies component. This pull request mandatory piggyback. Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

…read_conditions) Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

…hecks Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Signed-off-by: Miguel Barro <miguel.barro@live.com>

…2872, some methods of DataReader.hpp are going to be implemented. This commit removes the not-supported warning for the merge. Signed-off-by: Mikel Rico <mikelrico@eprosima.com>

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

…2879) * Refs #14686: Add warning to indicate unsupported APIs in DomainParticipant.hpp Signed-off-by: Mikel Rico <mikelrico@eprosima.com> * Refs #14686: Add warning to indicate unsupported APIs in DomainParticipantFactory.hpp Signed-off-by: Mikel Rico <mikelrico@eprosima.com> * Refs #14686: Add warning to indicate unsupported APIs in DataWriter.hpp Signed-off-by: Mikel Rico <mikelrico@eprosima.com> * Refs #14686: Add warning to indicate unsupported APIs in Publisher.hpp Signed-off-by: Mikel Rico <mikelrico@eprosima.com> * Refs #14686: Add warning to indicate unsupported APIs in DataReader.hpp Signed-off-by: Mikel Rico <mikelrico@eprosima.com> * Refs #14686: Add warning to indicate unsupported APIs in Subscriber.hpp Signed-off-by: Mikel Rico <mikelrico@eprosima.com> * Refs #14686: Add warning to indicate unsupported APIs in Topic.hpp Signed-off-by: Mikel Rico <mikelrico@eprosima.com> * Refs #14686: In accordance with [15158] ReadConditions Implementation #2872, some methods of DataReader.hpp are going to be implemented. This commit removes the not-supported warning for the merge. Signed-off-by: Mikel Rico <mikelrico@eprosima.com>

MiguelCompany · 2022-07-27T09:35:01Z

@richiprosima Please test this

jsan-rt · 2022-07-27T09:38:28Z

test/unittest/dds/subscriber/DataReaderTests.cpp

+    // Should failed with outstanding ReadConditions
+    ASSERT_EQ(subscriber->delete_datareader(data_reader), ReturnCode_t::RETCODE_PRECONDITION_NOT_MET);
+
+    std::cout << "Before recursive?" << std::endl;


Remove trace.

jsan-rt · 2022-07-27T09:38:35Z

test/unittest/dds/subscriber/DataReaderTests.cpp

@@ -2580,7 +2587,17 @@ TEST_F(DataReaderTests, delete_contained_entities)
    // To be updated when Query Conditions are available
    ASSERT_EQ(query_condition, nullptr);

+    std::cout << "check non-recursive?" << std::endl;


Remove trace

jsan-rt · 2022-07-27T09:38:45Z

test/unittest/dds/subscriber/DataReaderTests.cpp

    ASSERT_EQ(data_reader->delete_contained_entities(), ReturnCode_t::RETCODE_OK);
+
+    std::cout << "Where is the SEH?" << std::endl;


I hope it's nowhere to be seen. Please remove this trace 😄

jsan-rt · 2022-07-27T09:39:15Z

test/unittest/dds/subscriber/DataReaderTests.cpp

@@ -2580,7 +2587,17 @@ TEST_F(DataReaderTests, delete_contained_entities)
    // To be updated when Query Conditions are available
    ASSERT_EQ(query_condition, nullptr);

+    std::cout << "check non-recursive?" << std::endl;
+
+    // Should failed with outstanding ReadConditions


typo: failed -> fail

jsan-rt · 2022-07-27T09:39:23Z

test/unittest/dds/subscriber/DataReaderTests.cpp

+
+    std::cout << "Before recursive?" << std::endl;
+
+    // Should not failed with outstanding ReadConditions


typo: failed -> fail

Signed-off-by: Miguel Barro <miguel.barro@live.com>

jsan-rt

LGTM

EduPonz · 2022-07-27T09:54:29Z

@richiprosima please test this

src/cpp/fastdds/subscriber/DataReaderImpl.cpp

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

Signed-off-by: Miguel Barro <miguel.barro@live.com>

MiguelBarro · 2022-07-27T11:17:57Z

@richiprosima please test this

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

MiguelCompany · 2022-07-27T12:25:52Z

@richiprosima Please test windows

MiguelCompany · 2022-07-27T12:40:22Z

@richiprosima Please test mac and test linux

…thods. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

…e_deleted. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

MiguelCompany · 2022-07-27T15:48:47Z

@richiprosima Please test this

jsan-rt

LGTM.

MiguelCompany added this to the v2.7.1 milestone Jul 22, 2022

MiguelBarro force-pushed the feature/read-condition/implementation branch from 7a6b4d3 to 7c4ed4d Compare July 22, 2022 10:46

jsan-rt suggested changes Jul 22, 2022

View reviewed changes

MiguelBarro force-pushed the feature/read-condition/implementation branch 2 times, most recently from 4398b7d to a3dce99 Compare July 22, 2022 14:20

MiguelBarro force-pushed the feature/read-condition/implementation branch 2 times, most recently from 6586676 to b08745f Compare July 26, 2022 08:17

MiguelCompany force-pushed the feature/read-condition/implementation branch from b08745f to 4eb8f91 Compare July 26, 2022 08:54

JLBuenoLopez added the versions-pending label Jul 26, 2022

MiguelBarro force-pushed the feature/read-condition/implementation branch from 4eb8f91 to d925e4e Compare July 26, 2022 10:20

MiguelBarro removed the versions-pending label Jul 26, 2022

MiguelBarro marked this pull request as ready for review July 26, 2022 14:43

MiguelBarro force-pushed the feature/read-condition/implementation branch from 8de1e55 to 891142b Compare July 26, 2022 15:30

jsan-rt suggested changes Jul 26, 2022

View reviewed changes

MiguelCompany and others added 13 commits July 27, 2022 07:48

Refs #15158. Using bulk notification on DataReaderImpl.

955cd2e

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

Refs #15158. Fixed RTPSReader::get_unread_count.

9372087

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

Refs #15158. Adding DataReaderHistoryCounters struct.

7edff66

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

Refs #15158. Counting samples_unread and samples_read

b7e500a

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

Refs #15158. Update counters when a sample changes from unread to read.

ae41d1a

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

Refs #15158. Update counters for different view and instance states.

48a3ce8

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

Refs #15168. Indicate where to notify.

56773ea

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

Refs 15158. Fixing Fast-DDS install to include the dlls in the librar…

0308195

…ies component. This pull request mandatory piggyback. Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 15158. ReadConditionImpl partially implemented (pending trigger).

5398a46

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 15158. DataWriterImpl partially implemented (pending try_nofity_…

8cca557

…read_conditions) Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 15158. Refactor sync and DDS delete methods implementation and c…

2f50c8b

…hecks Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 15158. try_notify_read_conditions implementation & mutex upgrade

47537a7

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 15158. Fixing datarace on collection update

85ea51b

Signed-off-by: Miguel Barro <miguel.barro@live.com>

Refs #15158. Fix iterator invalidation on individual notification.

75ea11b

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

MiguelCompany force-pushed the feature/read-condition/implementation branch from bf26dca to 75ea11b Compare July 27, 2022 09:25

jsan-rt suggested changes Jul 27, 2022

View reviewed changes

MiguelBarro force-pushed the feature/read-condition/implementation branch 2 times, most recently from 53002ba to 66e7784 Compare July 27, 2022 09:47

Refs 15158. Addressing reviewers commits

65c58a1

Signed-off-by: Miguel Barro <miguel.barro@live.com>

MiguelBarro force-pushed the feature/read-condition/implementation branch from 66e7784 to 65c58a1 Compare July 27, 2022 09:51

jsan-rt previously approved these changes Jul 27, 2022

View reviewed changes

MiguelCompany requested changes Jul 27, 2022

View reviewed changes

src/cpp/fastdds/subscriber/DataReaderImpl.cpp Outdated Show resolved Hide resolved

Fixed DataReaderImpl::get_unread_count

e31536c

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

MiguelCompany dismissed jsan-rt’s stale review via e31536c July 27, 2022 10:58

Refs 15158. Fixing windows v141 warnings

4054acb

Signed-off-by: Miguel Barro <miguel.barro@live.com>

Refs #15158. Fix errors on msvc_v141.

ef79996

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

MiguelCompany added 3 commits July 27, 2022 16:09

Refs #15158. Fixed DataReaderUnsupportedTests.UnsupportedDataReaderMe…

6d27caa

…thods. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

Refs #15158. Avoid accessing nullptr reader_ on DataReaderImpl::can_b…

16e404b

…e_deleted. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

Refs #15158. Fix unit test UserListeners.data_available

9774494

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

MiguelCompany force-pushed the feature/read-condition/implementation branch from e0a125c to 9774494 Compare July 27, 2022 15:47

jsan-rt approved these changes Jul 27, 2022

View reviewed changes

MiguelCompany merged commit 919b6f8 into master Jul 28, 2022

MiguelCompany deleted the feature/read-condition/implementation branch July 28, 2022 05:06

This was referenced Aug 5, 2022

[15370] Test regressions on Ros 2 Humble and Rolling after #2872 #2904

Closed

[15375] Skip writer_removed processing for unaccounted instances #2905

Merged

	// 7- Check the DataReader cannot be deleted with outstanding conditions
	EXPECT_EQ(subscriber_->delete_datareader(another_reader), ReturnCode_t::RETCODE_PRECONDITION_NOT_MET);
	// but delete_contained_entities() succeeds with outstanding ReadConditions
	EXPECT_EQ(another_reader->delete_contained_entities(), ReturnCode_t::RETCODE_OK);
	// no outstanding conditions (killed above)
	EXPECT_EQ(subscriber_->delete_datareader(another_reader), ReturnCode_t::RETCODE_OK);


		#include <fastdds/dds/subscriber/ReadCondition.hpp>
		#include <fastdds/dds/core/StackAllocatedSequence.hpp>

		ASSERT_EQ(data_reader->delete_contained_entities(), ReturnCode_t::RETCODE_OK);

		std::cout << "Where is the SEH?" << std::endl;


		std::cout << "Before recursive?" << std::endl;

		// Should not failed with outstanding ReadConditions

[15158] ReadConditions Implementation #2872

[15158] ReadConditions Implementation #2872

Conversation

MiguelBarro commented Jul 22, 2022 • edited Loading

Description

Contributor Checklist

Reviewer Checklist

Choose a reason for hiding this comment

MiguelBarro Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiguelCompany commented Jul 22, 2022

MiguelBarro commented Jul 26, 2022

MiguelCompany commented Jul 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsan-rt Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiguelCompany commented Jul 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsan-rt left a comment

Choose a reason for hiding this comment

EduPonz commented Jul 27, 2022

MiguelBarro commented Jul 27, 2022

MiguelCompany commented Jul 27, 2022

MiguelCompany commented Jul 27, 2022

MiguelCompany commented Jul 27, 2022

jsan-rt left a comment

Choose a reason for hiding this comment

MiguelBarro commented Jul 22, 2022 •

edited

Loading

MiguelBarro Jul 22, 2022 •

edited

Loading

jsan-rt Jul 26, 2022 •

edited

Loading