Skip to content

Commit a5b7503

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 dbf1097 commit a5b7503

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
@@ -170,6 +170,44 @@ rmw_ret_t rmw_wait(
170170
uint64_t gen = rmw_uds::registry_generation(header);
171171
if (gen != ctx->last_registry_generation) {
172172
ctx->last_registry_generation = gen;
173+
174+
// TRANSIENT_LOCAL late-joiner replay. Lock held across the loop —
175+
// rmw_destroy_publisher takes the same mutex then deletes, so this
176+
// is what keeps each pub pointer alive while we dereference it.
177+
std::lock_guard<std::mutex> tl_lock(ctx->transient_local_pubs_mutex);
178+
for (auto * pub : ctx->transient_local_pubs) {
179+
std::vector<std::string> sub_paths;
180+
{
181+
std::lock_guard<std::mutex> sc_lock(pub->sub_cache_mutex);
182+
if (rmw_uds::registry_generation(header) != pub->cached_generation) {
183+
auto subs = rmw_uds::registry_query(
184+
header, rmw_uds::ENTRY_SUBSCRIPTION,
185+
pub->topic_name.c_str(), nullptr, nullptr);
186+
pub->cached_generation = rmw_uds::registry_generation(header);
187+
pub->cached_subscriber_paths.clear();
188+
pub->cached_subscriber_paths.reserve(subs.size());
189+
for (const auto & s : subs) {
190+
if (!s.socket_path.empty()) {
191+
pub->cached_subscriber_paths.push_back(s.socket_path);
192+
}
193+
}
194+
}
195+
sub_paths = pub->cached_subscriber_paths;
196+
}
197+
std::lock_guard<std::mutex> c_lock(pub->cache_mutex);
198+
for (const auto & path : sub_paths) {
199+
if (pub->known_subscriber_paths.count(path) != 0) {
200+
continue;
201+
}
202+
pub->known_subscriber_paths.insert(path);
203+
for (const auto & cm : pub->message_cache) {
204+
rmw_uds::send_to(
205+
ctx->send_socket_fd,
206+
path, cm.header, cm.payload.data(), cm.payload.size());
207+
}
208+
}
209+
}
210+
173211
// Trigger all graph guard conditions in the guard_conditions list
174212
if (guard_conditions) {
175213
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
@@ -182,6 +182,62 @@ TEST_F(QosTest, PublishReturnsErrorOnEMSGSIZE)
182182
auto _r2 [[maybe_unused]] = rmw_destroy_publisher(node, pub);
183183
}
184184

185+
TEST_F(QosTest, TransientLocalReplayOnWaitNoSubsequentPublish)
186+
{
187+
// Late sub must receive cached msgs from rmw_wait alone — no fresh publish.
188+
auto qos = make_qos(
189+
RMW_QOS_POLICY_RELIABILITY_RELIABLE,
190+
RMW_QOS_POLICY_DURABILITY_TRANSIENT_LOCAL, 5);
191+
192+
auto pub_opts = rmw_get_default_publisher_options();
193+
auto * pub = rmw_create_publisher(node, ts, "/wait_replay", &qos, &pub_opts);
194+
ASSERT_NE(nullptr, pub);
195+
196+
for (int i = 1; i <= 3; ++i) {
197+
test_msgs::msg::BasicTypes m;
198+
m.int32_value = i;
199+
EXPECT_EQ(RMW_RET_OK, rmw_publish(pub, &m, nullptr));
200+
}
201+
202+
auto sub_opts = rmw_get_default_subscription_options();
203+
auto * sub = rmw_create_subscription(node, ts, "/wait_replay", &qos, &sub_opts);
204+
ASSERT_NE(nullptr, sub);
205+
206+
auto * ws = rmw_create_wait_set(&context, 1);
207+
ASSERT_NE(nullptr, ws);
208+
209+
rmw_subscriptions_t subscriptions;
210+
void * sub_array[1] = {sub->data};
211+
subscriptions.subscribers = sub_array;
212+
subscriptions.subscriber_count = 1;
213+
214+
rmw_time_t timeout;
215+
timeout.sec = 0;
216+
timeout.nsec = 500 * 1000 * 1000; // 500 ms
217+
218+
EXPECT_EQ(RMW_RET_OK, rmw_wait(
219+
&subscriptions, nullptr, nullptr, nullptr, nullptr, ws, &timeout));
220+
EXPECT_NE(nullptr, subscriptions.subscribers[0]);
221+
222+
std::vector<int> received;
223+
for (int i = 0; i < 10; ++i) {
224+
test_msgs::msg::BasicTypes recv;
225+
bool taken = false;
226+
EXPECT_EQ(RMW_RET_OK, rmw_take(sub, &recv, &taken, nullptr));
227+
if (!taken) {break;}
228+
received.push_back(recv.int32_value);
229+
}
230+
231+
ASSERT_EQ(3u, received.size());
232+
EXPECT_EQ(1, received[0]);
233+
EXPECT_EQ(2, received[1]);
234+
EXPECT_EQ(3, received[2]);
235+
236+
EXPECT_EQ(RMW_RET_OK, rmw_destroy_wait_set(ws));
237+
auto _r1 [[maybe_unused]] = rmw_destroy_subscription(node, sub);
238+
auto _r2 [[maybe_unused]] = rmw_destroy_publisher(node, pub);
239+
}
240+
185241
TEST_F(QosTest, PublishStillReturnsOkOnSoftDropPeerGone)
186242
{
187243
// ENOENT on a vanished peer must stay RET_OK — only EMSGSIZE escalates.
@@ -212,6 +268,51 @@ TEST_F(QosTest, PublishStillReturnsOkOnSoftDropPeerGone)
212268
auto _r2 [[maybe_unused]] = rmw_destroy_publisher(node, pub);
213269
}
214270

271+
TEST_F(QosTest, TransientLocalReplayOnWaitDepthOne)
272+
{
273+
// depth=1 + late-join: the single cached msg must reach the new sub via wait.
274+
auto qos = make_qos(
275+
RMW_QOS_POLICY_RELIABILITY_RELIABLE,
276+
RMW_QOS_POLICY_DURABILITY_TRANSIENT_LOCAL, 1);
277+
278+
auto pub_opts = rmw_get_default_publisher_options();
279+
auto * pub = rmw_create_publisher(node, ts, "/depth1_wait", &qos, &pub_opts);
280+
ASSERT_NE(nullptr, pub);
281+
282+
test_msgs::msg::BasicTypes m;
283+
m.int32_value = 42;
284+
EXPECT_EQ(RMW_RET_OK, rmw_publish(pub, &m, nullptr));
285+
286+
auto sub_opts = rmw_get_default_subscription_options();
287+
auto * sub = rmw_create_subscription(node, ts, "/depth1_wait", &qos, &sub_opts);
288+
ASSERT_NE(nullptr, sub);
289+
290+
auto * ws = rmw_create_wait_set(&context, 1);
291+
ASSERT_NE(nullptr, ws);
292+
293+
rmw_subscriptions_t subscriptions;
294+
void * sub_array[1] = {sub->data};
295+
subscriptions.subscribers = sub_array;
296+
subscriptions.subscriber_count = 1;
297+
298+
rmw_time_t timeout;
299+
timeout.sec = 0;
300+
timeout.nsec = 500 * 1000 * 1000;
301+
EXPECT_EQ(RMW_RET_OK, rmw_wait(
302+
&subscriptions, nullptr, nullptr, nullptr, nullptr, ws, &timeout));
303+
EXPECT_NE(nullptr, subscriptions.subscribers[0]);
304+
305+
test_msgs::msg::BasicTypes recv;
306+
bool taken = false;
307+
EXPECT_EQ(RMW_RET_OK, rmw_take(sub, &recv, &taken, nullptr));
308+
ASSERT_TRUE(taken);
309+
EXPECT_EQ(42, recv.int32_value);
310+
311+
EXPECT_EQ(RMW_RET_OK, rmw_destroy_wait_set(ws));
312+
auto _r1 [[maybe_unused]] = rmw_destroy_subscription(node, sub);
313+
auto _r2 [[maybe_unused]] = rmw_destroy_publisher(node, pub);
314+
}
315+
215316
TEST_F(QosTest, VolatileNoCache)
216317
{
217318
auto qos = make_qos(

0 commit comments

Comments
 (0)