Make peek_next_message_from_queue return a SharedPtr. #993
Conversation
It is unsafe to return a pointer to a SharedPtr here, and then store it; if another thread comes along and pops the queue, then we are now holding a pointer to an object that has been freed. Instead, dereference the pointer so we get a SharedPtr (and a reference) immediately, which ensures that the object won't be destroyed if we race with another thread. While I was in here, did a small refactoring of the method to remove some duplicated code. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
clalancette
requested review from
emersonknapp and
hidmic
and removed request for
a team
|
CI:
|
There was a problem hiding this comment.
@clalancette Thank you for the code clean up.
I agree that using shared pointer instead of pointer to the shared pointer will make code less error prone for the race conditions.
Although I disagree with renaming message_ptr to the message where its corresponds to the shared pointer. It's still a pointer and comparison aka message != null_ptr looks very odd in this case.
Could you please revert such renaming?
I'll point out that the the current naming is following what was already there. That is, in all cases where we were returning a That said, I'll go in and rename things to |
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
|
CI:
|
|
The CI failure on Linux is one of the known flaky tests in rosbag2, i.e. test_play_services sometimes just fails. That's the next one for me to look into. |
It is unsafe to return a pointer to a SharedPtr here,
and then store it; if another thread comes along and
pops the queue, then we are now holding a pointer to an
object that has been freed. Instead, dereference the
pointer so we get a SharedPtr (and a reference) immediately,
which ensures that the object won't be destroyed if we
race with another thread.
While I was in here, did a small refactoring of the method
to remove some duplicated code.
Signed-off-by: Chris Lalancette clalancette@openrobotics.org
Before this PR, if I ran the
test_play_seektest in a loop with stress on my machine, it would crash every once in a while. After this PR, I no longer see that crash. This should fix at least one of the flaky tests on the buildfarm.