Make rosbag2_transport::Player::play() run in a separate thread
#1503
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
roncapat
requested review from
emersonknapp and
james-rms
and removed request for
a team
roncapat
force-pushed
the
roncagliolo/async-play
branch
from
6ad02e2 to
24e11e7
Compare
Signed-off-by: Patrick Roncagliolo <[email protected]>
roncapat
force-pushed
the
roncagliolo/async-play
branch
from
24e11e7 to
f557dc5
Compare
|
I did |
MichaelOrlov
requested changes
Nov 29, 2023
There was a problem hiding this comment.
@roncapat See my suggestions for review fixes in the roncapat#6 PR
Signed-off-by: Michael Orlov <[email protected]>
- Fixed multiple race conditions in tests when wait_for_playback_to_end() calls in a separate thread before Player::play() method. Those race conditions would likely lead to a false negative or flaky tests. - Replaced player_future with wait_for_playback_to_end() to the direct call of the player_->wait_for_playback_to_end() in the most of the places. Signed-off-by: Michael Orlov <[email protected]>
Signed-off-by: Michael Orlov <[email protected]>
- Rationale: To support Clang Thread Safety Analysis - Removed locally defined TSAUniqueLock which is a duplicate of the rcpputils::unique_lock. - Also removed some unused includes in player.cpp and player.hpp Signed-off-by: Michael Orlov <[email protected]>
- Rationale: Make wait_for_playback_to_finish(..) to be optionally as a non-blocking call Signed-off-by: Michael Orlov <[email protected]>
|
@MichaelOrlov review branch merged :) |
|
Gist: https://gist.githubusercontent.com/MichaelOrlov/aa32bdbe242c8c06daca683655e76bb4/raw/4be47e99df1fdad56dd18ce5f2d2c4f04ff13980/ros2.repos
|
MichaelOrlov
changed the title
play() run in a separate thread (#1500)play() run in a separate thread
MichaelOrlov
changed the title
play() run in a separate threadrosbag2_transport::Player::play() run in a separate thread
|
https://github.com/Mergifyio backport iron |
✅ Backports have been created
|
mergify bot
pushed a commit
that referenced
this pull request
Nov 29, 2023
) * Play spawns a separate thread Signed-off-by: Patrick Roncagliolo <[email protected]> * Address multiple race conditions in player and some renames after review Signed-off-by: Michael Orlov <[email protected]> * Fixes in the rosbag2_transport tests - Fixed multiple race conditions in tests when wait_for_playback_to_end() calls in a separate thread before Player::play() method. Those race conditions would likely lead to a false negative or flaky tests. - Replaced player_future with wait_for_playback_to_end() to the direct call of the player_->wait_for_playback_to_end() in the most of the places. Signed-off-by: Michael Orlov <[email protected]> * Rename wait_for_playback_to_end() to the wait_for_playback_to_finish() Signed-off-by: Michael Orlov <[email protected]> * Use rcpputils::unique_lock for RCPPUTILS TSA guarded mutexes - Rationale: To support Clang Thread Safety Analysis - Removed locally defined TSAUniqueLock which is a duplicate of the rcpputils::unique_lock. - Also removed some unused includes in player.cpp and player.hpp Signed-off-by: Michael Orlov <[email protected]> * Add timeout parameter to the wait_for_playback_to_finish(timeout) - Rationale: Make wait_for_playback_to_finish(..) to be optionally as a non-blocking call Signed-off-by: Michael Orlov <[email protected]> --------- Signed-off-by: Patrick Roncagliolo <[email protected]> Signed-off-by: Michael Orlov <[email protected]> Co-authored-by: Michael Orlov <[email protected]> (cherry picked from commit 1a52da8) # Conflicts: # rosbag2_transport/src/rosbag2_transport/player.cpp
|
Note:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is intended to solve the issue reported in #1500.
CC: @MichaelOrlov @KenYN
This can be also beneficial introductory work for #1419. As it has been noticed, playback shall start automatically when the
Playercomponent is loaded, but this can't be done via syncronous call by the component constructor.Current status:
Anyway, I believe that a round of refactor can be done here. For example, direclty returning an
std::futurefromwait_for_playback_to_end(). Opinions?@KenYN may I ask you also to do a test with this branch to confirm that the
playservice now returns immediately?