Skip to content

Commit 1fd5a96

Browse files
Barry-Xu-2018Barry-Xu-2018
authored
Fix bug on the disorder of calling shutdown callback (#2097)
* Fix bug on the disorder of calling shutdown callback Signed-off-by: Barry Xu <[email protected]>
1 parent be8c5d0 commit 1fd5a96

File tree

4 files changed

+235
-6
lines changed

4 files changed

+235
-6
lines changed

‎rclcpp/include/rclcpp/context.hpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -376,10 +376,10 @@ class Context : public std::enable_shared_from_this<Context>
376376
// attempt to acquire another sub context.
377377
std::recursive_mutex sub_contexts_mutex_;
378378

379-
std::unordered_set<std::shared_ptr<OnShutdownCallback>> on_shutdown_callbacks_;
379+
std::vector<std::shared_ptr<OnShutdownCallback>> on_shutdown_callbacks_;
380380
mutable std::mutex on_shutdown_callbacks_mutex_;
381381

382-
std::unordered_set<std::shared_ptr<PreShutdownCallback>> pre_shutdown_callbacks_;
382+
std::vector<std::shared_ptr<PreShutdownCallback>> pre_shutdown_callbacks_;
383383
mutable std::mutex pre_shutdown_callbacks_mutex_;
384384

385385
/// Condition variable for timed sleep (see sleep_for).

‎rclcpp/src/rclcpp/context.cpp‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -400,10 +400,10 @@ Context::add_shutdown_callback(
400400

401401
if constexpr (shutdown_type == ShutdownType::pre_shutdown) {
402402
std::lock_guard<std::mutex> lock(pre_shutdown_callbacks_mutex_);
403-
pre_shutdown_callbacks_.emplace(callback_shared_ptr);
403+
pre_shutdown_callbacks_.emplace_back(callback_shared_ptr);
404404
} else {
405405
std::lock_guard<std::mutex> lock(on_shutdown_callbacks_mutex_);
406-
on_shutdown_callbacks_.emplace(callback_shared_ptr);
406+
on_shutdown_callbacks_.emplace_back(callback_shared_ptr);
407407
}
408408

409409
ShutdownCallbackHandle callback_handle;
@@ -421,9 +421,19 @@ Context::remove_shutdown_callback(
421421
return false;
422422
}
423423

424-
const auto remove_callback = [this, &callback_shared_ptr](auto & mutex, auto & callback_set) {
424+
const auto remove_callback = [&callback_shared_ptr](auto & mutex, auto & callback_vector) {
425425
const std::lock_guard<std::mutex> lock(mutex);
426-
return callback_set.erase(callback_shared_ptr) == 1;
426+
auto iter = callback_vector.begin();
427+
for (; iter != callback_vector.end(); iter++) {
428+
if ((*iter).get() == callback_shared_ptr.get()) {
429+
break;
430+
}
431+
}
432+
if (iter == callback_vector.end()) {
433+
return false;
434+
}
435+
callback_vector.erase(iter);
436+
return true;
427437
};
428438

429439
static_assert(

‎rclcpp/test/rclcpp/CMakeLists.txt‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -582,6 +582,9 @@ target_link_libraries(test_logger ${PROJECT_NAME})
582582
ament_add_gmock(test_logging test_logging.cpp)
583583
target_link_libraries(test_logging ${PROJECT_NAME})
584584

585+
ament_add_gmock(test_context test_context.cpp)
586+
target_link_libraries(test_context ${PROJECT_NAME})
587+
585588
ament_add_gtest(test_time test_time.cpp
586589
APPEND_LIBRARY_DIRS "${append_library_dirs}")
587590
if(TARGET test_time)

‎rclcpp/test/rclcpp/test_context.cpp‎

Lines changed: 216 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,216 @@
1+
// Copyright 2023 Sony Group Corporation.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
#include <gtest/gtest.h>
15+
16+
#include "rclcpp/context.hpp"
17+
#include "rclcpp/rclcpp.hpp"
18+
19+
TEST(TestContext, check_pre_shutdown_callback_order) {
20+
auto context = std::make_shared<rclcpp::Context>();
21+
context->init(0, nullptr);
22+
23+
int result[4] = {0, 0, 0, 0};
24+
int index = 0;
25+
26+
auto callback1 = [&result, &index]() {
27+
result[index] = 1;
28+
index++;
29+
};
30+
auto callback2 = [&result, &index]() {
31+
result[index] = 2;
32+
index++;
33+
};
34+
auto callback3 = [&result, &index]() {
35+
result[index] = 3;
36+
index++;
37+
};
38+
auto callback4 = [&result, &index]() {
39+
result[index] = 4;
40+
index++;
41+
};
42+
43+
context->add_pre_shutdown_callback(callback1);
44+
context->add_pre_shutdown_callback(callback2);
45+
context->add_pre_shutdown_callback(callback3);
46+
context->add_pre_shutdown_callback(callback4);
47+
48+
context->shutdown("for test");
49+
50+
EXPECT_TRUE(result[0] == 1 && result[1] == 2 && result[2] == 3 && result[3] == 4);
51+
}
52+
53+
TEST(TestContext, check_on_shutdown_callback_order) {
54+
auto context = std::make_shared<rclcpp::Context>();
55+
context->init(0, nullptr);
56+
57+
int result[4] = {0, 0, 0, 0};
58+
int index = 0;
59+
60+
auto callback1 = [&result, &index]() {
61+
result[index] = 1;
62+
index++;
63+
};
64+
auto callback2 = [&result, &index]() {
65+
result[index] = 2;
66+
index++;
67+
};
68+
auto callback3 = [&result, &index]() {
69+
result[index] = 3;
70+
index++;
71+
};
72+
auto callback4 = [&result, &index]() {
73+
result[index] = 4;
74+
index++;
75+
};
76+
77+
context->add_on_shutdown_callback(callback1);
78+
context->add_on_shutdown_callback(callback2);
79+
context->add_on_shutdown_callback(callback3);
80+
context->add_on_shutdown_callback(callback4);
81+
82+
context->shutdown("for test");
83+
84+
EXPECT_TRUE(result[0] == 1 && result[1] == 2 && result[2] == 3 && result[3] == 4);
85+
}
86+
87+
TEST(TestContext, check_mixed_register_shutdown_callback_order) {
88+
auto context = std::make_shared<rclcpp::Context>();
89+
context->init(0, nullptr);
90+
91+
int result[8] = {0, 0, 0, 0, 0, 0, 0, 0};
92+
int index = 0;
93+
94+
auto callback1 = [&result, &index]() {
95+
result[index] = 1;
96+
index++;
97+
};
98+
auto callback2 = [&result, &index]() {
99+
result[index] = 2;
100+
index++;
101+
};
102+
auto callback3 = [&result, &index]() {
103+
result[index] = 3;
104+
index++;
105+
};
106+
auto callback4 = [&result, &index]() {
107+
result[index] = 4;
108+
index++;
109+
};
110+
auto callback5 = [&result, &index]() {
111+
result[index] = 5;
112+
index++;
113+
};
114+
auto callback6 = [&result, &index]() {
115+
result[index] = 6;
116+
index++;
117+
};
118+
auto callback7 = [&result, &index]() {
119+
result[index] = 7;
120+
index++;
121+
};
122+
auto callback8 = [&result, &index]() {
123+
result[index] = 8;
124+
index++;
125+
};
126+
127+
// Mixed register
128+
context->add_pre_shutdown_callback(callback1);
129+
context->add_on_shutdown_callback(callback5);
130+
context->add_pre_shutdown_callback(callback2);
131+
context->add_on_shutdown_callback(callback6);
132+
context->add_pre_shutdown_callback(callback3);
133+
context->add_on_shutdown_callback(callback7);
134+
context->add_pre_shutdown_callback(callback4);
135+
context->add_on_shutdown_callback(callback8);
136+
137+
context->shutdown("for test");
138+
139+
EXPECT_TRUE(
140+
result[0] == 1 && result[1] == 2 && result[2] == 3 && result[3] == 4 &&
141+
result[4] == 5 && result[5] == 6 && result[6] == 7 && result[7] == 8);
142+
}
143+
144+
TEST(TestContext, check_pre_shutdown_callback_order_after_del) {
145+
auto context = std::make_shared<rclcpp::Context>();
146+
context->init(0, nullptr);
147+
148+
int result[4] = {0, 0, 0, 0};
149+
int index = 0;
150+
151+
auto callback1 = [&result, &index]() {
152+
result[index] = 1;
153+
index++;
154+
};
155+
auto callback2 = [&result, &index]() {
156+
result[index] = 2;
157+
index++;
158+
};
159+
auto callback3 = [&result, &index]() {
160+
result[index] = 3;
161+
index++;
162+
};
163+
auto callback4 = [&result, &index]() {
164+
result[index] = 4;
165+
index++;
166+
};
167+
168+
context->add_pre_shutdown_callback(callback1);
169+
auto callback_handle = context->add_pre_shutdown_callback(callback2);
170+
context->add_pre_shutdown_callback(callback3);
171+
context->add_pre_shutdown_callback(callback4);
172+
173+
EXPECT_TRUE(context->remove_pre_shutdown_callback(callback_handle));
174+
EXPECT_FALSE(context->remove_pre_shutdown_callback(callback_handle));
175+
176+
context->shutdown("for test");
177+
178+
EXPECT_TRUE(result[0] == 1 && result[1] == 3 && result[2] == 4 && result[3] == 0);
179+
}
180+
181+
TEST(TestContext, check_on_shutdown_callback_order_after_del) {
182+
auto context = std::make_shared<rclcpp::Context>();
183+
context->init(0, nullptr);
184+
185+
int result[4] = {0, 0, 0, 0};
186+
int index = 0;
187+
188+
auto callback1 = [&result, &index]() {
189+
result[index] = 1;
190+
index++;
191+
};
192+
auto callback2 = [&result, &index]() {
193+
result[index] = 2;
194+
index++;
195+
};
196+
auto callback3 = [&result, &index]() {
197+
result[index] = 3;
198+
index++;
199+
};
200+
auto callback4 = [&result, &index]() {
201+
result[index] = 4;
202+
index++;
203+
};
204+
205+
context->add_on_shutdown_callback(callback1);
206+
auto callback_handle = context->add_on_shutdown_callback(callback2);
207+
context->add_on_shutdown_callback(callback3);
208+
context->add_on_shutdown_callback(callback4);
209+
210+
EXPECT_TRUE(context->remove_on_shutdown_callback(callback_handle));
211+
EXPECT_FALSE(context->remove_on_shutdown_callback(callback_handle));
212+
213+
context->shutdown("for test");
214+
215+
EXPECT_TRUE(result[0] == 1 && result[1] == 3 && result[2] == 4 && result[3] == 0);
216+
}

0 commit comments

Comments
 (0)