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

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

Merged
merged 6 commits into from
Jan 19, 2023
Merged

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

merged 6 commits into from
Jan 19, 2023

Conversation

MiguelCompany
Copy link
Member

@MiguelCompany MiguelCompany commented Jan 18, 2023 •
edited by EduPonz
Loading

Description

This is a rework of #3203 that avoids the segfault reported on ros2/rmw_fastrtps#659

Backports: We will update the currently created PRs (#3217, #3218, #3219) accordingly

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.
  • N/A 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.

Mario-DL and others added 5 commits January 18, 2023 07:16
@Mario-DL @MiguelCompany
Fix total_unread_ consistent with reader's history after get_first_un…
f1706d4 
…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>
@MiguelCompany
Refs #16608. Reversed unnecessary changes on DDSBlackboxTestsBasic
7a4f7d6 
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@MiguelCompany
Refs #16608. Not using shared pointer for PubSubReader and PubSubWriter.
72f4574 
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@MiguelCompany
Refs #16608. Improved test.
b9d3e01 
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@MiguelCompany
Refs #16608. Improved solution.
3923ee3 
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Mario-DL
Mario-DL previously approved these changes Jan 18, 2023
View reviewed changes
Copy link
Member

@Mario-DL Mario-DL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@MiguelCompany
Refs #16608. Fix linters.
112af3f 
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Mario-DL
Mario-DL approved these changes Jan 18, 2023
View reviewed changes
Copy link
Member

@Mario-DL Mario-DL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@MiguelCompany
Copy link
Member Author

@fujitatomoya Would you mind running a ROS2 CI with this one?

@EduPonz EduPonz added this to the v2.9.1 milestone Jan 18, 2023
@EduPonz EduPonz added the ci-pending PR which CI is running label Jan 18, 2023
@MiguelCompany MiguelCompany added ready-to-merge Ready to be merged. CI and changes have been reviewed and approved. and removed ci-pending PR which CI is running labels Jan 18, 2023
@fujitatomoya
Copy link
Contributor

Full CI for rmw_fastrtps:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@fujitatomoya
Copy link
Contributor

  • Linux Build Status

@MiguelCompany MiguelCompany merged commit 4398841 into master Jan 19, 2023
@MiguelCompany MiguelCompany deleted the hotfix/16608 branch January 19, 2023 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mario-DL Mario-DL Mario-DL approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

Assignees
No one assigned
Labels
ready-to-merge Ready to be merged. CI and changes have been reviewed and approved.
Projects
None yet
Milestone
v2.9.1
Development

Successfully merging this pull request may close these issues.
4 participants
@MiguelCompany @fujitatomoya @Mario-DL @EduPonz