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

[16192] Fix notification lost #3194

Merged
merged 10 commits into from
Jan 10, 2023
Merged

[16192] Fix notification lost #3194

merged 10 commits into from
Jan 10, 2023

Conversation

MiguelCompany
Copy link
Member

@MiguelCompany MiguelCompany commented Jan 9, 2023 •
edited
Loading

Description

This is a re-work of #3087, using the original approach by @iuhilnehc-ynos and fixing the deadlocks on ReadConditionImpl.

@Mergifyio backport 2.8.x 2.7.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.
  • 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.

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.

Chen Lihui and others added 7 commits January 9, 2023 10:06
@MiguelCompany
fix notification lost
6e3a78a 
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
@MiguelCompany
add a regression test
2037b14 
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
@MiguelCompany
rename a variable name and update comments
cff853c 
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
@MiguelCompany
fix uncrustify issue
e03354b 
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
@MiguelCompany
make the regression test better
7de6c61 
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
@MiguelCompany
fix uncrustify
13cc59f 
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
@MiguelCompany
Refs #16192. Fix deadlock on WaitSetImpl.
486112c 
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@MiguelCompany
Copy link
Member Author

CC @iuhilnehc-ynos @fujitatomoya

@MiguelCompany
Refs #16192. Fix deadlock on ReadConditionImpl.
19a9f96 
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@MiguelCompany
Copy link
Member Author

@richiprosima Please test this

1 similar comment
@MiguelCompany
Copy link
Member Author

@richiprosima Please test this

@MiguelCompany
Refs #16192. Use mutex instead of atomic.
805820a 
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@MiguelCompany
Copy link
Member Author

@richiprosima Please test this

This was referenced Jan 9, 2023
Merged
Closed
@iuhilnehc-ynos
Copy link
Contributor

@MiguelCompany

I have confirmed that this PR (#3194) can solve the ros2/rmw_fastrtps#650 issue well.

@MiguelCompany
Refs #16192. Fix deadlock when constructing ReadConditionImpl.
5666533 
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@MiguelCompany
Copy link
Member Author

@richiprosima Please test this

fujitatomoya
fujitatomoya approved these changes Jan 10, 2023
View reviewed changes
Copy link
Contributor

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm

@fujitatomoya
Copy link
Contributor

@iuhilnehc-ynos since you are original author for the base fix, can you review the code as well?

MiguelBarro
MiguelBarro approved these changes Jan 10, 2023
View reviewed changes
Copy link
Contributor

@MiguelBarro MiguelBarro 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 MiguelCompany merged commit df2857a into master Jan 10, 2023
@MiguelCompany MiguelCompany deleted the bugfix/16192 branch January 10, 2023 10:35
@MiguelCompany
Copy link
Member Author

@Mergifyio backport 2.8.x 2.7.x

mergify bot pushed a commit that referenced this pull request Jan 10, 2023
@MiguelCompany @mergify
Fix notification lost (#3194)
96b0c37 
* fix notification lost

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* add a regression test

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* rename a variable name and update comments

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* fix uncrustify issue

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* make the regression test better

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* fix uncrustify

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* Refs #16192. Fix deadlock on WaitSetImpl.

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

* Refs #16192. Fix deadlock on ReadConditionImpl.

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

* Refs #16192. Use mutex instead of atomic.

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

* Refs #16192. Fix deadlock when constructing ReadConditionImpl.

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

Signed-off-by: Chen Lihui <lihui.chen@sony.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Co-authored-by: Chen Lihui <lihui.chen@sony.com>
(cherry picked from commit df2857a)

# Conflicts:
#	src/cpp/fastdds/subscriber/ReadConditionImpl.hpp
@mergify mergify bot mentioned this pull request Jan 10, 2023
Merged
mergify bot pushed a commit that referenced this pull request Jan 10, 2023
@MiguelCompany @mergify
Fix notification lost (#3194)
651e4ac 
* fix notification lost

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* add a regression test

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* rename a variable name and update comments

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* fix uncrustify issue

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* make the regression test better

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* fix uncrustify

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* Refs #16192. Fix deadlock on WaitSetImpl.

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

* Refs #16192. Fix deadlock on ReadConditionImpl.

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

* Refs #16192. Use mutex instead of atomic.

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

* Refs #16192. Fix deadlock when constructing ReadConditionImpl.

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

Signed-off-by: Chen Lihui <lihui.chen@sony.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Co-authored-by: Chen Lihui <lihui.chen@sony.com>
(cherry picked from commit df2857a)

# Conflicts:
#	src/cpp/fastdds/subscriber/ReadConditionImpl.hpp
@mergify mergify bot mentioned this pull request Jan 10, 2023
Merged
@mergify
Copy link
Contributor

mergify bot commented Jan 10, 2023

backport 2.8.x 2.7.x

✅ Backports have been created

MiguelCompany added a commit that referenced this pull request Jan 11, 2023
@mergify @MiguelCompany
Fix notification lost (#3198)
4dd9652 
* Fix notification lost (#3194)

* fix notification lost

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* add a regression test

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* rename a variable name and update comments

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* fix uncrustify issue

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* make the regression test better

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* fix uncrustify

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* Refs #16192. Fix deadlock on WaitSetImpl.

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

* Refs #16192. Fix deadlock on ReadConditionImpl.

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

* Refs #16192. Use mutex instead of atomic.

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

* Refs #16192. Fix deadlock when constructing ReadConditionImpl.

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

Signed-off-by: Chen Lihui <lihui.chen@sony.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Co-authored-by: Chen Lihui <lihui.chen@sony.com>
(cherry picked from commit df2857a)

# Conflicts:
#	src/cpp/fastdds/subscriber/ReadConditionImpl.hpp

* Refs #16192. Fixed conflicts.

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

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Co-authored-by: Miguel Company <miguelcompany@eprosima.com>
MiguelCompany added a commit that referenced this pull request Jan 11, 2023
@mergify @MiguelCompany
Fix notification lost (#3199)
b38ade0 
* Fix notification lost (#3194)

* fix notification lost

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* add a regression test

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* rename a variable name and update comments

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* fix uncrustify issue

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* make the regression test better

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* fix uncrustify

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* Refs #16192. Fix deadlock on WaitSetImpl.

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

* Refs #16192. Fix deadlock on ReadConditionImpl.

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

* Refs #16192. Use mutex instead of atomic.

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

* Refs #16192. Fix deadlock when constructing ReadConditionImpl.

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

Signed-off-by: Chen Lihui <lihui.chen@sony.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Co-authored-by: Chen Lihui <lihui.chen@sony.com>
(cherry picked from commit df2857a)

# Conflicts:
#	src/cpp/fastdds/subscriber/ReadConditionImpl.hpp

* Refs #16192. Fixed conflicts.

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

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Co-authored-by: Miguel Company <miguelcompany@eprosima.com>
This was referenced Jan 11, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fujitatomoya fujitatomoya fujitatomoya approved these changes

@MiguelBarro MiguelBarro MiguelBarro approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MiguelCompany @iuhilnehc-ynos @fujitatomoya @MiguelBarro