player owns the reader #725
Conversation
Karsten1987
requested review from
jaisontj and
prajakta-gokhale
and removed request for
a team
Karsten1987
force-pushed
the
kk/player_own_storage
branch
from
2893775
to
c6a24e0
Compare
|
|
I am not sure what's happening with the Windows installer, but it looks like it can't download Qt at the moment. |
This exact thing has definitely been a problem in the past. Seems like the windows agent setup is pretty finicky. |
|
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Karsten1987
force-pushed
the
kk/player_own_storage
branch
from
f9bdc8e
to
de661b0
Compare
|
Fast-RTPS is still unreliable in terms of discovery. I had to limit the amount of "expected messages" when played twice for Fast-RTPS to not introduce flakiness :/
|
| { | ||
| auto current = std::chrono::high_resolution_clock::now(); | ||
| if (current - start_time > std::chrono::seconds(10)) { |
There was a problem hiding this comment.
What is "10 seconds" here?
There was a problem hiding this comment.
That's a 10 seconds time out. If the test messages are not arrived by then, the thread canceled. I needed that to avoid running in a Gtest timeout situation (60 seconds) at which point one does not know why the test timed out.
|
I am a bit hesitant to merge this quite yet as the equivalent changes to the recorder class are far from trivial in order to keep a minimal PR. |
|
|
||
| rosbag2_cpp::Reader * Player::release_reader() | ||
| { | ||
| reader_->reset(); |
There was a problem hiding this comment.
@Karsten1987 What a rational to delete current reader object via reset just before calling release?
Player::release_reader() is going to always return null_ptr.
There was a problem hiding this comment.
This isn't .reset, it's ->reset, which is calling the reset method of the object itself, not the reset method for the smart pointer.
There was a problem hiding this comment.
Sorry I missed that "This isn't .reset, it's ->reset".
But it is still unclear for me why do we need to reset underlying storage in reader before releasing pointer to the reader?
There was a problem hiding this comment.
This makes it so that the reader is not in an intermediate state, like halfway through the bag. The new owner can call open successfully after this.
There was a problem hiding this comment.
Ok good. now it is clear for me.
While it's arguable whether it should be reset or not, at least I wouldn't expect it to be reset on release_reader call.
I think at least we will need to add comment about it somewhere, perhaps in doxygen comment for the Player::release_reader() method.
There was a problem hiding this comment.
@emersonknapp @Karsten1987 BTW. This method Player::release_reader() looks very unsafe. What if someone will call it somewhere in the middle of the playback?
It would be more safely to force "stop" for player before releasing reader from it.
There was a problem hiding this comment.
At this point I think I will default to "PRs welcome" instead of continuing to discuss minor points on merged PRs. If you can provide a test proving the unsafeness of the current behavior, all the better.
There was a problem hiding this comment.
@emersonknapp I didn't expect that someone will take my comments as an action items for fixing already merged PR's.
The purpose of my comments is a discussion to eliminate potential issues and come to the common opinion about findings and perhaps introduce some improvements in future if we would agree up on it.
There was a problem hiding this comment.
I think that this intention is good. In practice, a merged PR is not a very good place to track future work, though. A PR or an enhancement ticket are more likely to move forward - this conversation is likely to get forgotten.
On open source projects, the most important motion comes in the form of contributions. Discussion is fine but if nobody commits to action, it doesn't usually amount to anything. So that's my opinion on this particular conversation - that it would be more productively continued in the form of a code review or backlog ticket.
There was a problem hiding this comment.
@emersonknapp I agree that separate issue or pull request would be more appropriate place for discussion. I've just have some many things to do that I don't have time for it. And if I will not write my thoughts somewhere It will be forgotten completely. At least here is a PR where relevant changes was introduced. I will try to raise a new issue for discussion next time.
One step closer to #716
The player takes a unique_ptr to the reader and thus owns the reader/storage access.
I'll do the same for the recorder in a different PR to keep the diff small.