Skip to content

Commit 0bd2c1f

Browse files
fix: replay TRANSIENT_LOCAL cache to late-joining subscribers
The publisher previously only replayed its cached messages inside rmw_publish, so a subscriber that joined after the publisher had gone idle never received the cache. In production this manifested as late-joining subscribers (e.g. `ros2 topic echo`, bridge consumers) silently missing latched topics such as camera_list. Track TRANSIENT_LOCAL publishers on the context and drive cache replay from rmw_wait whenever the registry generation changes. The mutex is held across the loop so rmw_destroy_publisher cannot free a publisher mid-iteration. known_subscriber_paths still deduplicates: whichever path (publish or wait) runs first marks the path, the other no-ops. Tests: - TransientLocalReplayOnWaitNoSubsequentPublish: publisher publishes N messages then goes idle; subscriber joins, calls rmw_wait once, must receive all N cached messages without a fresh publish. - TransientLocalReplayOnWaitDepthOne: same flow with depth=1. Both tests fail on HEAD and pass with this change.
1 parent bfbf725 commit 0bd2c1f

4 files changed

Lines changed: 161 additions & 0 deletions

File tree

‎rmw_unix_socket_cpp/src/rmw_publisher.cpp‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
#include "transport.hpp"
66
#include "types.hpp"
77

8+
#include <algorithm>
89
#include <chrono>
910
#include <cstring>
1011

@@ -129,6 +130,12 @@ rmw_publisher_t * rmw_create_publisher(
129130
pub->topic_name = rcutils_strdup(topic_name, node->context->options.allocator);
130131
pub->options = publisher_options ? *publisher_options : rmw_get_default_publisher_options();
131132
pub->can_loan_messages = false;
133+
134+
if (pub_data->qos.durability == RMW_QOS_POLICY_DURABILITY_TRANSIENT_LOCAL) {
135+
std::lock_guard<std::mutex> lock(ctx->transient_local_pubs_mutex);
136+
ctx->transient_local_pubs.push_back(pub_data);
137+
}
138+
132139
return pub;
133140
}
134141

@@ -142,6 +149,14 @@ rmw_ret_t rmw_destroy_publisher(rmw_node_t * node, rmw_publisher_t * publisher)
142149

143150
auto * pub_data = static_cast<rmw_uds::UdsPublisher *>(publisher->data);
144151
if (pub_data) {
152+
if (pub_data->context &&
153+
pub_data->qos.durability == RMW_QOS_POLICY_DURABILITY_TRANSIENT_LOCAL)
154+
{
155+
auto * ctx = pub_data->context;
156+
std::lock_guard<std::mutex> lock(ctx->transient_local_pubs_mutex);
157+
auto & v = ctx->transient_local_pubs;
158+
v.erase(std::remove(v.begin(), v.end(), pub_data), v.end());
159+
}
145160
if (pub_data->context && pub_data->registry_index >= 0) {
146161
auto * header = rmw_uds::registry_header(pub_data->context->registry_ptr);
147162
rmw_uds::registry_remove(header, pub_data->registry_index);

‎rmw_unix_socket_cpp/src/rmw_wait.cpp‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,44 @@ rmw_ret_t rmw_wait(
169169
uint64_t gen = rmw_uds::registry_generation(header);
170170
if (gen != ctx->last_registry_generation) {
171171
ctx->last_registry_generation = gen;
172+
173+
// TRANSIENT_LOCAL late-joiner replay. Lock held across the loop —
174+
// rmw_destroy_publisher takes the same mutex then deletes, so this
175+
// is what keeps each pub pointer alive while we dereference it.
176+
std::lock_guard<std::mutex> tl_lock(ctx->transient_local_pubs_mutex);
177+
for (auto * pub : ctx->transient_local_pubs) {
178+
std::vector<std::string> sub_paths;
179+
{
180+
std::lock_guard<std::mutex> sc_lock(pub->sub_cache_mutex);
181+
if (rmw_uds::registry_generation(header) != pub->cached_generation) {
182+
auto subs = rmw_uds::registry_query(
183+
header, rmw_uds::ENTRY_SUBSCRIPTION,
184+
pub->topic_name.c_str(), nullptr, nullptr);
185+
pub->cached_generation = rmw_uds::registry_generation(header);
186+
pub->cached_subscriber_paths.clear();
187+
pub->cached_subscriber_paths.reserve(subs.size());
188+
for (const auto & s : subs) {
189+
if (!s.socket_path.empty()) {
190+
pub->cached_subscriber_paths.push_back(s.socket_path);
191+
}
192+
}
193+
}
194+
sub_paths = pub->cached_subscriber_paths;
195+
}
196+
std::lock_guard<std::mutex> c_lock(pub->cache_mutex);
197+
for (const auto & path : sub_paths) {
198+
if (pub->known_subscriber_paths.count(path) != 0) {
199+
continue;
200+
}
201+
pub->known_subscriber_paths.insert(path);
202+
for (const auto & cm : pub->message_cache) {
203+
rmw_uds::send_to(
204+
ctx->send_socket_fd,
205+
path, cm.header, cm.payload.data(), cm.payload.size());
206+
}
207+
}
208+
}
209+
172210
// Trigger all graph guard conditions in the guard_conditions list
173211
if (guard_conditions) {
174212
for (size_t i = 0; i < guard_conditions->guard_condition_count; ++i) {

‎rmw_unix_socket_cpp/src/types.hpp‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,9 @@ struct ReceivedMessage
7575
int64_t received_timestamp_ns;
7676
};
7777

78+
// Forward declaration: publisher type defined below.
79+
struct UdsPublisher;
80+
7881
// Per-context implementation data
7982
struct UdsContext
8083
{
@@ -86,6 +89,10 @@ struct UdsContext
8689
std::atomic<bool> is_shutdown{false};
8790
uint64_t last_registry_generation = 0;
8891
rmw_guard_condition_t * graph_guard_condition = nullptr;
92+
93+
// TRANSIENT_LOCAL publishers, for wait-side cache replay on graph change.
94+
std::mutex transient_local_pubs_mutex;
95+
std::vector<UdsPublisher *> transient_local_pubs;
8996
};
9097

9198
// Node data

‎rmw_unix_socket_cpp/test/test_rmw_qos.cpp‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,107 @@ TEST_F(QosTest, TransientLocalCacheDepthEnforced)
134134
auto _r2 [[maybe_unused]] = rmw_destroy_publisher(node, pub);
135135
}
136136

137+
TEST_F(QosTest, TransientLocalReplayOnWaitNoSubsequentPublish)
138+
{
139+
// Late sub must receive cached msgs from rmw_wait alone — no fresh publish.
140+
auto qos = make_qos(
141+
RMW_QOS_POLICY_RELIABILITY_RELIABLE,
142+
RMW_QOS_POLICY_DURABILITY_TRANSIENT_LOCAL, 5);
143+
144+
auto pub_opts = rmw_get_default_publisher_options();
145+
auto * pub = rmw_create_publisher(node, ts, "/wait_replay", &qos, &pub_opts);
146+
ASSERT_NE(nullptr, pub);
147+
148+
for (int i = 1; i <= 3; ++i) {
149+
test_msgs::msg::BasicTypes m;
150+
m.int32_value = i;
151+
EXPECT_EQ(RMW_RET_OK, rmw_publish(pub, &m, nullptr));
152+
}
153+
154+
auto sub_opts = rmw_get_default_subscription_options();
155+
auto * sub = rmw_create_subscription(node, ts, "/wait_replay", &qos, &sub_opts);
156+
ASSERT_NE(nullptr, sub);
157+
158+
auto * ws = rmw_create_wait_set(&context, 1);
159+
ASSERT_NE(nullptr, ws);
160+
161+
rmw_subscriptions_t subscriptions;
162+
void * sub_array[1] = {sub->data};
163+
subscriptions.subscribers = sub_array;
164+
subscriptions.subscriber_count = 1;
165+
166+
rmw_time_t timeout;
167+
timeout.sec = 0;
168+
timeout.nsec = 500 * 1000 * 1000; // 500 ms
169+
170+
EXPECT_EQ(RMW_RET_OK, rmw_wait(
171+
&subscriptions, nullptr, nullptr, nullptr, nullptr, ws, &timeout));
172+
EXPECT_NE(nullptr, subscriptions.subscribers[0]);
173+
174+
std::vector<int> received;
175+
for (int i = 0; i < 10; ++i) {
176+
test_msgs::msg::BasicTypes recv;
177+
bool taken = false;
178+
EXPECT_EQ(RMW_RET_OK, rmw_take(sub, &recv, &taken, nullptr));
179+
if (!taken) {break;}
180+
received.push_back(recv.int32_value);
181+
}
182+
183+
ASSERT_EQ(3u, received.size());
184+
EXPECT_EQ(1, received[0]);
185+
EXPECT_EQ(2, received[1]);
186+
EXPECT_EQ(3, received[2]);
187+
188+
EXPECT_EQ(RMW_RET_OK, rmw_destroy_wait_set(ws));
189+
auto _r1 [[maybe_unused]] = rmw_destroy_subscription(node, sub);
190+
auto _r2 [[maybe_unused]] = rmw_destroy_publisher(node, pub);
191+
}
192+
193+
TEST_F(QosTest, TransientLocalReplayOnWaitDepthOne)
194+
{
195+
// depth=1 + late-join: the single cached msg must reach the new sub via wait.
196+
auto qos = make_qos(
197+
RMW_QOS_POLICY_RELIABILITY_RELIABLE,
198+
RMW_QOS_POLICY_DURABILITY_TRANSIENT_LOCAL, 1);
199+
200+
auto pub_opts = rmw_get_default_publisher_options();
201+
auto * pub = rmw_create_publisher(node, ts, "/depth1_wait", &qos, &pub_opts);
202+
ASSERT_NE(nullptr, pub);
203+
204+
test_msgs::msg::BasicTypes m;
205+
m.int32_value = 42;
206+
EXPECT_EQ(RMW_RET_OK, rmw_publish(pub, &m, nullptr));
207+
208+
auto sub_opts = rmw_get_default_subscription_options();
209+
auto * sub = rmw_create_subscription(node, ts, "/depth1_wait", &qos, &sub_opts);
210+
ASSERT_NE(nullptr, sub);
211+
212+
auto * ws = rmw_create_wait_set(&context, 1);
213+
ASSERT_NE(nullptr, ws);
214+
215+
rmw_subscriptions_t subscriptions;
216+
void * sub_array[1] = {sub->data};
217+
subscriptions.subscribers = sub_array;
218+
subscriptions.subscriber_count = 1;
219+
220+
rmw_time_t timeout;
221+
timeout.sec = 0;
222+
timeout.nsec = 500 * 1000 * 1000;
223+
EXPECT_EQ(RMW_RET_OK, rmw_wait(
224+
&subscriptions, nullptr, nullptr, nullptr, nullptr, ws, &timeout));
225+
EXPECT_NE(nullptr, subscriptions.subscribers[0]);
226+
227+
test_msgs::msg::BasicTypes recv;
228+
bool taken = false;
229+
EXPECT_EQ(RMW_RET_OK, rmw_take(sub, &recv, &taken, nullptr));
230+
ASSERT_TRUE(taken);
231+
EXPECT_EQ(42, recv.int32_value);
232+
233+
EXPECT_EQ(RMW_RET_OK, rmw_destroy_wait_set(ws));
234+
auto _r1 [[maybe_unused]] = rmw_destroy_subscription(node, sub);
235+
auto _r2 [[maybe_unused]] = rmw_destroy_publisher(node, pub);
236+
}
237+
137238
TEST_F(QosTest, VolatileNoCache)
138239
{
139240
auto qos = make_qos(

0 commit comments

Comments
 (0)