Skip to content

Commit 656ec1a

Browse files
committed
fix(ros2): address Copilot round-2 review on PR #9745
Follow-up to 7e5d2d2 that closes the remaining races and semantics bugs surfaced by Copilot's second pass: * SubscriberImpl::GetMessage: clear _new_message under _message_mutex so on_data_available cannot drop a pending sample by racing the flag-clear against its own store(true). Same fix applied to the on_data_available side: the lock now spans both the _message write and the flag store. * SubscriberImpl::on_subscription_matched and PublisherImpl::on_publication_matched: derive _alive from info.current_count > 0, not info.total_count > 0. total_count is cumulative and never decrements, so the previous code latched IsAlive() to true forever after the first match. * ROS2::AddActorRosName: insert_or_assign so a re-registered actor actually picks up its new ros_name. The previous unordered_map ::insert silently kept the stale entry.
1 parent 9f18588 commit 656ec1a

3 files changed

Lines changed: 12 additions & 13 deletions

File tree

LibCarla/source/carla/ros2/ROS2.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,9 @@ void ROS2::SetTimestamp(double timestamp) {
132132
}
133133

134134
void ROS2::AddActorRosName(void *actor, std::string ros_name) {
135-
_actor_ros_name.insert({actor, ros_name});
135+
// insert_or_assign so re-registering an actor with a new ros_name actually
136+
// updates the entry; unordered_map::insert would silently keep the stale one.
137+
_actor_ros_name.insert_or_assign(actor, std::move(ros_name));
136138
}
137139

138140
void ROS2::AddActorParentRosName(void *actor, void* parent) {

LibCarla/source/carla/ros2/publishers/PublisherImpl.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ class PublisherImpl : public efd::DataWriterListener {
5555
void on_publication_matched(
5656
efd::DataWriter * /*writer*/,
5757
const efd::PublicationMatchedStatus &info) override {
58-
_alive.store(info.total_count > 0, std::memory_order_release);
58+
_alive.store(info.current_count > 0, std::memory_order_release);
5959
}
6060

6161
~PublisherImpl() override {

LibCarla/source/carla/ros2/subscribers/SubscriberImpl.h

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -56,14 +56,16 @@ class SubscriberImpl : public efd::DataReaderListener {
5656
void on_subscription_matched(
5757
efd::DataReader * /*reader*/,
5858
const efd::SubscriptionMatchedStatus &info) override {
59-
_alive.store(info.total_count > 0, std::memory_order_release);
59+
_alive.store(info.current_count > 0, std::memory_order_release);
6060
}
6161

6262
void on_data_available(efd::DataReader *reader) override {
6363
// FastDDS invokes this on a DDS listener thread. Take into a stack-local
6464
// sample first so the lock only spans the copy into _message, and so a
6565
// dispose/unregister notification (RETCODE_OK with !info.valid_data) does
66-
// not flip _new_message.
66+
// not flip _new_message. The _new_message store is held inside the lock
67+
// so a concurrent GetMessage cannot race-clear the flag after we publish
68+
// a fresh sample.
6769
efd::SampleInfo info;
6870
msg_type sample{};
6971
erc rcode = reader->take_next_sample(&sample, &info);
@@ -74,10 +76,8 @@ class SubscriberImpl : public efd::DataReaderListener {
7476
if (!info.valid_data) {
7577
return;
7678
}
77-
{
78-
std::lock_guard<std::mutex> lock(_message_mutex);
79-
_message = std::move(sample);
80-
}
79+
std::lock_guard<std::mutex> lock(_message_mutex);
80+
_message = std::move(sample);
8181
_new_message.store(true, std::memory_order_release);
8282
}
8383

@@ -141,11 +141,8 @@ class SubscriberImpl : public efd::DataReaderListener {
141141
[[nodiscard]] bool IsAlive() const noexcept { return _alive.load(std::memory_order_acquire); }
142142

143143
msg_type GetMessage() {
144-
msg_type copy;
145-
{
146-
std::lock_guard<std::mutex> lock(_message_mutex);
147-
copy = _message;
148-
}
144+
std::lock_guard<std::mutex> lock(_message_mutex);
145+
msg_type copy = _message;
149146
_new_message.store(false, std::memory_order_release);
150147
return copy;
151148
}

0 commit comments

Comments
 (0)