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.

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

Fix race condition in TopicInfo #432

Merged
merged 3 commits into from
Aug 25, 2023
Merged

Fix race condition in TopicInfo #432

merged 3 commits into from
Aug 25, 2023

Conversation

caguero
Copy link
Collaborator

@caguero caguero commented Aug 24, 2023

🦟 Bug fix

See #430

Summary

This patch fixes a potential race condition when calling TopicList.

See #430 to reproduce the issue/fix with the Gazebo Image Display plugin.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@github-actions github-actions bot added the 🎵 harmonic Gazebo Harmonic label Aug 24, 2023
@caguero caguero changed the title Protect remoteSubscribers with mutex. Fix race condition in TopicInfo Aug 24, 2023
@caguero caguero requested a review from iche033 August 24, 2023 21:23
@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Merging #432 (326cf64) into main (91acfb0) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head 326cf64 differs from pull request most recent head e56c132. Consider uploading reports for the commit e56c132 to get more accurate results

@@            Coverage Diff             @@
##             main     #432      +/-   ##
==========================================
- Coverage   87.79%   87.76%   -0.04%     
==========================================
  Files          59       59              
  Lines        5694     5696       +2     
==========================================
  Hits         4999     4999              
- Misses        695      697       +2     
Files Changed Coverage Δ
include/gz/transport/Discovery.hh 87.00% <100.00%> (+0.04%) ⬆️

... and 1 file with indirect coverage changes

Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

Do you need to protect also this line ?

this->remoteSubscribers.TopicList(remoteSubs);

@caguero
Copy link
Collaborator Author

caguero commented Aug 24, 2023

Do you need to protect also this line ?

this->remoteSubscribers.TopicList(remoteSubs);

There's a mutex after WaitForInit() that protects that line.

Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

fixes the issue for me!

@azeey azeey added the beta Targeting beta release of upcoming collection label Aug 25, 2023
@caguero caguero merged commit c507108 into main Aug 25, 2023
5 of 6 checks passed
@caguero caguero deleted the fix_issue_430 branch August 25, 2023 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 🎵 harmonic Gazebo Harmonic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants