[16608] Fix total_unread_ consistent with reader's history upon get_first_untaken_info() #3203

Mario-DL · 2023-01-11T12:01:58Z

Description

mp_reader->nextUntakenCache() in get_first_untaken_info() is removing the change (it->remove_change()) if matched_writer_lookup() returns false. This, in turn calls change_removed_by_history() but get_last_notified() returns always false in that case because no writerGUID is found, hence a sequence number of 0.0 is returned.

This situation causes that total_unread_ is not decremented despite removing the change, so the history size and total_unread_ variable are inconsistent and the issue arises.

When user calls take_next_sample(), Ret::NO_DATA is returned despite reader->get_unread_count() is different from 0.

Proposed solution

get_first_untaken_info() should be a read-only API and mp_reader->nextUntakenCache() should not be called from inside because it modifies the history in certain cases.

@Mergifyio backport 2.8.x 2.7.x 2.6.x

Contributor Checklist

Commit messages follow the project guidelines.
The code follows the style guidelines of this project.
Tests that thoroughly check the new feature have been added/Regression tests checking the bug and its fix have been added.
Any new/modified methods have been properly documented using Doxygen.
Fast DDS test suite has been run locally.
Changes are ABI compatible.
Changes are API compatible.
N/A Documentation builds and tests pass locally.
N/A New feature has been added to the versions.md file (if applicable).
N/A New feature has been documented/Current behavior is correctly described in the documentation.

Applicable backports have been included in the description.

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.

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>

Mario-DL · 2023-01-15T21:33:46Z

@richiprosima please test this

test/blackbox/common/DDSBlackboxTestsBasic.cpp

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>

test/blackbox/common/DDSBlackboxTestsDataReader.cpp

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>

MiguelCompany

LGTM

MiguelCompany · 2023-01-16T14:38:06Z

@richiprosima Please test this

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>

MiguelCompany

👍

MiguelCompany · 2023-01-17T10:34:43Z

@richiprosima Please test this

MiguelCompany · 2023-01-17T13:17:03Z

@Mergifyio backport 2.8.x 2.7.x 2.6.x

mergify · 2023-01-17T13:18:06Z

backport 2.8.x 2.7.x 2.6.x

✅ Backports have been created

#3217 [16608] Fix total_unread_ consistent with reader's history upon get_first_untaken_info() (backport #3203) has been created for branch 2.8.x
#3218 [16608] Fix total_unread_ consistent with reader's history upon get_first_untaken_info() (backport #3203) has been created for branch 2.7.x
#3219 [16608] Fix total_unread_ consistent with reader's history upon get_first_untaken_info() (backport #3203) has been created for branch 2.6.x

fujitatomoya · 2023-01-17T20:51:43Z

src/cpp/fastdds/subscriber/history/DataReaderHistory.cpp

-                        {
-                            return v == change;
-                        });
+        auto item = instance_changes.cbegin();


this item could be NULL? see ros2/rmw_fastrtps#659

…first_untaken_info (#3203)" This reverts commit de5cd9c. Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

…first_untaken_info (#3203)" (#3221) This reverts commit de5cd9c. Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com> Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

…taken_info (#3203) * Refs 16608: Added BlackBoxTest Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs 16608: Fix. Make get_first_untaken_info() a read-only API call Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Fix tsan warning Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Applied suggested changes Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Applied second-reviewed suggested changes Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Skip test for Data-Sharing Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>

…taken_info (#3203) * Refs 16608: Added BlackBoxTest Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs 16608: Fix. Make get_first_untaken_info() a read-only API call Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Fix tsan warning Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Applied suggested changes Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Applied second-reviewed suggested changes Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Skip test for Data-Sharing Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> (cherry picked from commit de5cd9c) # Conflicts: # test/blackbox/api/dds-pim/PubSubReader.hpp

…taken_info (#3203) * Refs 16608: Added BlackBoxTest Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs 16608: Fix. Make get_first_untaken_info() a read-only API call Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Fix tsan warning Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Applied suggested changes Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Applied second-reviewed suggested changes Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Skip test for Data-Sharing Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>

…taken_info (#3203) * Refs 16608: Added BlackBoxTest Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs 16608: Fix. Make get_first_untaken_info() a read-only API call Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Fix tsan warning Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Applied suggested changes Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Applied second-reviewed suggested changes Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Skip test for Data-Sharing Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> (cherry picked from commit de5cd9c) Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>

…taken_info (#3223) * Fix total_unread_ consistent with reader's history after get_first_untaken_info (#3203) * Refs 16608: Added BlackBoxTest Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs 16608: Fix. Make get_first_untaken_info() a read-only API call Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Fix tsan warning Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Applied suggested changes Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Applied second-reviewed suggested changes Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Skip test for Data-Sharing Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608. Reversed unnecessary changes on DDSBlackboxTestsBasic Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs #16608. Not using shared pointer for PubSubReader and PubSubWriter. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs #16608. Improved test. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs #16608. Improved solution. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs #16608. Fix linters. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> Co-authored-by: Mario Domínguez López <116071334+Mario-DL@users.noreply.github.com>

…aken_info() (#3219) * Fix total_unread_ consistent with reader's history after get_first_untaken_info (#3203) * Refs 16608: Added BlackBoxTest Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs 16608: Fix. Make get_first_untaken_info() a read-only API call Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Fix tsan warning Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Applied suggested changes Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Applied second-reviewed suggested changes Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Skip test for Data-Sharing Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> (cherry picked from commit de5cd9c) # Conflicts: # test/blackbox/api/dds-pim/PubSubReader.hpp * Refs #16608. Reversed unnecessary changes on DDSBlackboxTestsBasic Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs #16608. Not using shared pointer for PubSubReader and PubSubWriter. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs #16608. Improved test. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs #16608. Improved solution. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Linter && Removed conflict marks Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> Co-authored-by: Mario Domínguez López <116071334+Mario-DL@users.noreply.github.com> Co-authored-by: Miguel Company <MiguelCompany@eprosima.com> Co-authored-by: Mario Dominguez <mariodominguez@eprosima.com>

…aken_info() (#3218) * Fix total_unread_ consistent with reader's history after get_first_untaken_info (#3203) * Refs 16608: Added BlackBoxTest Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs 16608: Fix. Make get_first_untaken_info() a read-only API call Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Fix tsan warning Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Applied suggested changes Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Applied second-reviewed suggested changes Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Skip test for Data-Sharing Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> (cherry picked from commit de5cd9c) Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Removed conflicts Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608. Reversed unnecessary changes on DDSBlackboxTestsBasic Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs #16608. Not using shared pointer for PubSubReader and PubSubWriter. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs #16608. Improved test. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs #16608. Improved solution. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Linter Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Removed Multithreaded test from DDSBlackBoxTestsBasic Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> Co-authored-by: Mario Domínguez López <116071334+Mario-DL@users.noreply.github.com> Co-authored-by: Mario Dominguez <mariodominguez@eprosima.com> Co-authored-by: Miguel Company <MiguelCompany@eprosima.com>

…aken_info() (#3217) * Fix total_unread_ consistent with reader's history after get_first_untaken_info (#3203) * Refs 16608: Added BlackBoxTest Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs 16608: Fix. Make get_first_untaken_info() a read-only API call Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Fix tsan warning Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Applied suggested changes Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Applied second-reviewed suggested changes Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608: Skip test for Data-Sharing Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> (cherry picked from commit de5cd9c) Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Resolved conflicts Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #16608. Reversed unnecessary changes on DDSBlackboxTestsBasic Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs #16608. Not using shared pointer for PubSubReader and PubSubWriter. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs #16608. Improved test. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs #16608. Improved solution. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Linter Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> Co-authored-by: Mario Domínguez López <116071334+Mario-DL@users.noreply.github.com> Co-authored-by: Mario Dominguez <mariodominguez@eprosima.com> Co-authored-by: Miguel Company <MiguelCompany@eprosima.com>

Mario-DL added 2 commits January 11, 2023 12:37

Refs 16608: Added BlackBoxTest

1f4c695

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>

Refs 16608: Fix. Make get_first_untaken_info() a read-only API call

14dccd0

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>

Mario-DL added needs-review PR that is ready to be reviewed ci-pending PR which CI is running labels Jan 11, 2023

Mario-DL changed the title ~~[16608] total_unread_ consistent with reader's history upon get_first_untaken_info()~~ [16608] Fix total_unread_ consistent with reader's history upon get_first_untaken_info() Jan 11, 2023

Refs #16608: Fix tsan warning

fd9854a

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>

Mario-DL removed the ci-pending PR which CI is running label Jan 16, 2023

MiguelCompany requested changes Jan 16, 2023

View reviewed changes

Refs #16608: Applied suggested changes

6a1a10f

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>

MiguelCompany requested changes Jan 16, 2023

View reviewed changes

test/blackbox/common/DDSBlackboxTestsDataReader.cpp Outdated Show resolved Hide resolved

Refs #16608: Applied second-reviewed suggested changes

2c6a2db

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>

MiguelCompany previously approved these changes Jan 16, 2023

View reviewed changes

MiguelCompany added ci-pending PR which CI is running and removed needs-review PR that is ready to be reviewed labels Jan 16, 2023

Refs #16608: Skip test for Data-Sharing

614c609

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>

Mario-DL dismissed MiguelCompany’s stale review via 614c609 January 17, 2023 10:32

MiguelCompany approved these changes Jan 17, 2023

View reviewed changes

MiguelCompany merged commit de5cd9c into master Jan 17, 2023

MiguelCompany deleted the bugfix/16608 branch January 17, 2023 13:11

mergify bot mentioned this pull request Jan 17, 2023

[16608] Fix total_unread_ consistent with reader's history upon get_first_untaken_info() (backport #3203) #3217

Merged

mergify bot mentioned this pull request Jan 17, 2023

[16608] Fix total_unread_ consistent with reader's history upon get_first_untaken_info() (backport #3203) #3218

Merged

mergify bot mentioned this pull request Jan 17, 2023

[16608] Fix total_unread_ consistent with reader's history upon get_first_untaken_info() (backport #3203) #3219

Merged

fujitatomoya reviewed Jan 17, 2023

View reviewed changes

fujitatomoya added a commit that referenced this pull request Jan 17, 2023

Revert "Fix total_unread_ consistent with reader's history after get_…

28072ba

…first_untaken_info (#3203)" This reverts commit de5cd9c. Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

fujitatomoya mentioned this pull request Jan 17, 2023

Revert "Fix total_unread_ consistent with reader's history after get_… #3221

Merged

14 tasks

MiguelCompany mentioned this pull request Jan 18, 2023

Fix total_unread_ consistent with reader's history after get_first_untaken_info #3223

Merged

10 tasks

This was referenced Mar 21, 2023

Implement test to reprocude AML-IP issue [16608] #3174

Closed

Add assert in StatefulReader total_unread_ #3172

Closed

Add regression tests in simple data delivery #3164

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[16608] Fix total_unread_ consistent with reader's history upon get_first_untaken_info() #3203

[16608] Fix total_unread_ consistent with reader's history upon get_first_untaken_info() #3203

Mario-DL commented Jan 11, 2023

Mario-DL commented Jan 15, 2023

MiguelCompany left a comment

MiguelCompany commented Jan 16, 2023

MiguelCompany left a comment

MiguelCompany commented Jan 17, 2023

MiguelCompany commented Jan 17, 2023

mergify bot commented Jan 17, 2023

fujitatomoya Jan 17, 2023

[16608] Fix total_unread_ consistent with reader's history upon get_first_untaken_info() #3203

[16608] Fix total_unread_ consistent with reader's history upon get_first_untaken_info() #3203

Conversation

Mario-DL commented Jan 11, 2023

Description

Proposed solution

Contributor Checklist

Reviewer Checklist

Mario-DL commented Jan 15, 2023

MiguelCompany left a comment

Choose a reason for hiding this comment

MiguelCompany commented Jan 16, 2023

MiguelCompany left a comment

Choose a reason for hiding this comment

MiguelCompany commented Jan 17, 2023

MiguelCompany commented Jan 17, 2023

mergify bot commented Jan 17, 2023

✅ Backports have been created

fujitatomoya Jan 17, 2023

Choose a reason for hiding this comment