From 62647fdb512fbad10b00fa819718af6651d4b861 Mon Sep 17 00:00:00 2001 From: Dennis Lanov Date: Tue, 25 Aug 2026 23:32:05 -0500 Subject: [PATCH 1/4] Fix logger node teardown before RMW shutdown Signed-off-by: Dennis Lanov --- moveit_core/utils/src/logger.cpp | 21 ++- moveit_core/utils/src/logger_detail.hpp | 131 +++++++++++++++ moveit_core/utils/test/CMakeLists.txt | 9 ++ moveit_core/utils/test/test_logger.cpp | 205 ++++++++++++++++++++++++ 4 files changed, 364 insertions(+), 2 deletions(-) create mode 100644 moveit_core/utils/src/logger_detail.hpp create mode 100644 moveit_core/utils/test/test_logger.cpp diff --git a/moveit_core/utils/src/logger.cpp b/moveit_core/utils/src/logger.cpp index fa1156f380..2efdf3fc3e 100644 --- a/moveit_core/utils/src/logger.cpp +++ b/moveit_core/utils/src/logger.cpp @@ -36,9 +36,11 @@ #include #include +#include #include #include #include +#include "logger_detail.hpp" namespace moveit { @@ -56,6 +58,12 @@ rclcpp::Logger& getGlobalRootLogger() try { static rclcpp::Node::SharedPtr moveit_node = rclcpp::Node::make_shared(name); + // See registerNodeResetOnPreShutdown()'s documentation for why the + // returned flag must exist even though it is not otherwise used here: + // this call, immediately after constructing moveit_node, is what makes + // moveit_node's destruction ordering (relative to the flag) safe. + static std::shared_ptr flag = detail::registerNodeResetOnPreShutdown(moveit_node); + (void)flag; return moveit_node->get_logger(); } catch (const std::exception& ex) @@ -72,8 +80,17 @@ rclcpp::Logger& getGlobalRootLogger() void setNodeLoggerName(const std::string& name) { - static auto node = std::make_shared("moveit", name); - getGlobalRootLogger() = node->get_logger(); + static rclcpp::Node::SharedPtr node = std::make_shared("moveit", name); + static std::shared_ptr flag = detail::registerNodeResetOnPreShutdown(node); + + std::lock_guard lock(flag->mutex); + if (node) + { + getGlobalRootLogger() = node->get_logger(); + } + // If the node has already been reset by a pre-shutdown callback from an + // earlier rclcpp::shutdown(), leave the global logger untouched rather + // than dereferencing a destroyed node. } rclcpp::Logger getLogger(const std::string& name) diff --git a/moveit_core/utils/src/logger_detail.hpp b/moveit_core/utils/src/logger_detail.hpp new file mode 100644 index 0000000000..8b5b88147b --- /dev/null +++ b/moveit_core/utils/src/logger_detail.hpp @@ -0,0 +1,131 @@ +/********************************************************************* + * Software License Agreement (BSD License) + * + * Copyright (c) 2026, PickNik Inc. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following + * disclaimer in the documentation and/or other materials provided + * with the distribution. + * * Neither the name of PickNik Inc. nor the names of its + * contributors may be used to endorse or promote products derived + * from this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE + * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, + * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER + * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN + * ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE + * POSSIBILITY OF SUCH DAMAGE. + *********************************************************************/ + +// Internal implementation detail of logger.cpp, split out into its own +// non-installed header purely so test_logger.cpp can test it directly +// without exposing it through the public moveit/utils/logger.hpp API. +#pragma once + +#include +#include +#include + +namespace moveit +{ +namespace detail +{ + +/// Out-of-band flag + mutex used to guard a caller-owned rclcpp::Node::SharedPtr +/// against a pre-shutdown callback resetting it concurrently, and to record +/// whether that reset has already happened. Deliberately its own, separately +/// allocated object -- NOT a wrapper that also stores the Node::SharedPtr +/// itself. See registerNodeResetOnPreShutdown() for why. +struct LoggerNodeFlag +{ + std::mutex mutex; + bool retired = false; +}; + +/// Arranges for `node` -- a caller-owned rclcpp::Node::SharedPtr with static +/// storage duration -- to be reset from an rclcpp pre-shutdown callback +/// (which runs *before* rcl_shutdown() tears down the associated RMW +/// context), instead of being left to run its destructor during static +/// destruction at process exit. Some RMW implementations (e.g. rmw_zenoh_cpp) +/// abort the process if RMW calls are made after their own process-wide +/// static state has already been torn down, which otherwise races against +/// the unspecified destruction order of unrelated function-local static +/// objects. See moveit/moveit2#3827. +/// +/// Returns a LoggerNodeFlag the caller must lock (its mutex) before reading +/// or writing `node` afterwards, and can check (`retired`) to know whether +/// the callback already reset it. +/// +/// Two deliberate design choices here, both required together and each +/// empirically verified while developing this fix: +/// +/// 1. The callback captures `node` by *reference*, not by storing a +/// shared_ptr to it (or to a wrapper struct that also owns it). Even a +/// same-process, non-heap struct that bundles an rclcpp::Node::SharedPtr +/// together with any other member was observed to break ordinary Node +/// teardown at process exit when rclcpp::shutdown() is never called +/// explicitly -- independent of, and in addition to, the reference-cycle +/// concern below. Keeping `node` a fully standalone +/// rclcpp::Node::SharedPtr, exactly as in the pre-fix code, avoids that. +/// A plain reference cannot itself ever be part of a shared_ptr reference +/// cycle. +/// +/// 2. The callback captures the returned LoggerNodeFlag by *weak_ptr*, not +/// shared_ptr. rclcpp::Node (via its NodeBase) strongly owns an +/// rclcpp::Context::SharedPtr, and Context strongly owns every +/// pre-shutdown callback registered on it. A callback holding a +/// shared_ptr to state that (transitively) owns the node would close a +/// strong reference cycle back to the context +/// (Context -> callback -> state -> node -> Context) that reference +/// counting alone could never break: if rclcpp::shutdown() were never +/// called, nothing would ever be destroyed at all, rather than merely +/// being destroyed later. A weak_ptr capture means the only edge from +/// Context back to this flag is non-owning, so there is no cycle. +/// +/// Together, this also makes the callback's behavior when +/// rclcpp::shutdown() is never called explicitly well-defined: `node` is a +/// static, so by the time it and the returned flag (constructed +/// immediately afterwards, by the caller) reach static destruction, the +/// flag -- constructed later -- is destroyed first, by the standard's +/// reverse-order-of-completed-construction rule. That drops the callback's +/// only strong-refcounted reference *before* `node` is destroyed, so by the +/// time the callback could possibly fire afterwards (e.g. from within the +/// context's own destructor), locking the weak_ptr fails and the callback +/// safely does nothing, leaving `node` to be destroyed exactly as it would +/// have been before this fix -- not fixed, but not worsened either. +inline std::shared_ptr registerNodeResetOnPreShutdown(rclcpp::Node::SharedPtr& node) +{ + auto flag = std::make_shared(); + std::weak_ptr weak_flag = flag; + node->get_node_base_interface()->get_context()->add_pre_shutdown_callback([weak_flag, &node] { + std::shared_ptr locked = weak_flag.lock(); + if (!locked) + { + return; + } + std::lock_guard lock(locked->mutex); + // Drop the reference so ~rclcpp::Node runs now, while the RMW context is + // still alive, instead of racing against it at static destruction time. + node.reset(); + locked->retired = true; + }); + return flag; +} + +} // namespace detail +} // namespace moveit diff --git a/moveit_core/utils/test/CMakeLists.txt b/moveit_core/utils/test/CMakeLists.txt index 30737b7e72..83a27929f1 100644 --- a/moveit_core/utils/test/CMakeLists.txt +++ b/moveit_core/utils/test/CMakeLists.txt @@ -5,6 +5,15 @@ target_link_libraries(logger_dut rclcpp::rclcpp moveit_utils) # Install is needed to for launchtest to execute install(TARGETS logger_dut DESTINATION lib/${PROJECT_NAME}) +# Unit test for the logger's node lifecycle (regression test for +# moveit/moveit2#3827). Links against the real moveit_utils library; reaches +# registerNodeResetOnPreShutdown()/LoggerNodeFlag via the internal +# (non-installed) src/logger_detail.hpp shared with logger.cpp, without exposing +# them through the public moveit/utils/logger.hpp API. +find_package(ament_cmake_gtest REQUIRED) +ament_add_gtest(test_logger test_logger.cpp) +target_link_libraries(test_logger moveit_utils) + find_package(launch_testing_ament_cmake) # These tests do not work on Humble as /rosout logging from child loggers does diff --git a/moveit_core/utils/test/test_logger.cpp b/moveit_core/utils/test/test_logger.cpp new file mode 100644 index 0000000000..7bd7cd00d4 --- /dev/null +++ b/moveit_core/utils/test/test_logger.cpp @@ -0,0 +1,205 @@ +/********************************************************************* + * Software License Agreement (BSD License) + * + * Copyright (c) 2026, PickNik Inc. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following + * disclaimer in the documentation and/or other materials provided + * with the distribution. + * * Neither the name of PickNik Inc. nor the names of its + * contributors may be used to endorse or promote products derived + * from this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE + * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, + * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER + * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN + * ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE + * POSSIBILITY OF SUCH DAMAGE. + *********************************************************************/ + +// Regression test for moveit/moveit2#3827: node-backed loggers must be +// destroyed before their rclcpp::Context calls rcl_shutdown(), not left to +// run their destructor at static-destruction time -- and must not do so via +// a reference cycle that would leak instead if rclcpp::shutdown() is never +// called. +// +// registerNodeResetOnPreShutdown()/LoggerNodeFlag live in the internal +// (non-installed) logger_detail.hpp, included by both logger.cpp (compiled +// into the real moveit_utils library) and this test, so the white-box tests +// below exercise the exact same code the library uses. This test links +// against the real moveit_utils rather than recompiling logger.cpp, and does +// not expose anything through the public moveit/utils/logger.hpp API. +// +// Test order matters in this file: GetGlobalRootLoggerTest must run before +// SetNodeLoggerNameTest, since the latter is the only test that touches the +// process-wide default rclcpp context (via plain rclcpp::init()), and +// getGlobalRootLogger()'s underlying logger is a function-local static that +// is only ever computed once for the life of the process. GoogleTest runs +// tests within one binary in the order they are defined (no shuffling is +// enabled here), so that ordering is preserved by construction. +#include "../src/logger_detail.hpp" + +#include +#include +#include +#include +#include + +namespace moveit +{ +// getGlobalRootLogger() has external linkage (it is not static/anonymous), +// but is intentionally not declared in the public logger.hpp -- it is an +// implementation detail of setNodeLoggerName()/getLogger(). Forward-declare +// it here to exercise its no-init fallback path directly, without adding it +// to the public header. +rclcpp::Logger& getGlobalRootLogger(); +} // namespace moveit + +namespace +{ + +TEST(GetGlobalRootLoggerTest, FallsBackToNonNodeLoggerBeforeInit) +{ + // getGlobalRootLogger() must not throw or crash when no rclcpp context has + // ever been initialized; it should fall back to a plain, non-node logger. + // (getGlobalRootLogger()'s underlying logger is only ever computed once + // per process, so this only meaningfully verifies the "before init" path + // if it runs before anything else in this binary calls rclcpp::init() -- + // see the file comment above.) + EXPECT_NO_THROW({ rclcpp::Logger logger = moveit::getGlobalRootLogger(); }); +} + +// Each white-box test below uses its own private rclcpp::Context (rather +// than the process default one) so tests are fully isolated from one +// another: rclcpp pre/on-shutdown callbacks are never removed from a +// Context once registered and persist across repeated init() calls on that +// same context, so reusing the global default context across tests would +// let one test's callback fire again during a later test's shutdown. +rclcpp::NodeOptions makeOptionsWithFreshContext(std::shared_ptr& context_out) +{ + context_out = std::make_shared(); + context_out->init(0, nullptr); + rclcpp::NodeOptions options; + options.context(context_out); + return options; +} + +TEST(RegisterNodeResetOnPreShutdownTest, ExplicitShutdownDestroysNode) +{ + std::shared_ptr context; + rclcpp::NodeOptions options = makeOptionsWithFreshContext(context); + + rclcpp::Node::SharedPtr node = std::make_shared("logger_reset_test", options); + std::weak_ptr weak_node = node; + std::shared_ptr flag = moveit::detail::registerNodeResetOnPreShutdown(node); + + EXPECT_FALSE(weak_node.expired()); + EXPECT_FALSE(flag->retired); + + // rcl_shutdown() runs as part of this call. If the node were destroyed + // only afterwards (e.g. at static destruction), an RMW implementation + // whose own process-wide state is torn down around the same time (e.g. + // rmw_zenoh_cpp) could abort. The pre-shutdown callback must destroy the + // node first. + context->shutdown("test shutdown"); + + EXPECT_EQ(node, nullptr) << "the caller's own node slot must be reset by the callback"; + EXPECT_TRUE(weak_node.expired()) << "node must be destroyed before rcl_shutdown(), not after"; + EXPECT_TRUE(flag->retired); +} + +// Proves there is no Context -> callback -> flag -> node -> Context +// reference cycle: if there were, dropping the caller's (only remaining +// strong) reference to the node without ever calling context->shutdown() +// would not be enough to free it, because the context's still-registered +// callback would still be keeping it alive. This tests actual object +// destruction via weak_ptr expiry, not merely a process exit code. +TEST(RegisterNodeResetOnPreShutdownTest, NoReferenceCycleWithoutExplicitShutdown) +{ + std::shared_ptr context; + rclcpp::NodeOptions options = makeOptionsWithFreshContext(context); + + std::weak_ptr weak_node; + std::weak_ptr weak_flag; + { + rclcpp::Node::SharedPtr node = std::make_shared("cycle_test_node", options); + weak_node = node; + std::shared_ptr flag = moveit::detail::registerNodeResetOnPreShutdown(node); + weak_flag = flag; + EXPECT_FALSE(weak_node.expired()); + EXPECT_FALSE(weak_flag.expired()); + // Both `node` and `flag` (the only strong owners of the node and the + // flag, respectively) go out of scope here, *without* ever calling + // context->shutdown() -- this is the "no explicit shutdown" case. + } + + // If the pre-shutdown callback held a strong reference back to the flag + // (or the flag held one to the node) that the context kept alive, these + // would still report `false`: the context is still alive and would still + // be keeping them alive through that callback. + EXPECT_TRUE(weak_flag.expired()) << "flag must not be kept alive by a reference cycle through the context"; + EXPECT_TRUE(weak_node.expired()) << "node must not be kept alive by a reference cycle through the context"; + + // The context itself, and its now-dangling (weak-only, already-expired) + // callback registration, can be safely torn down too. + context->shutdown("test cleanup"); +} + +// Black-box test of the actual public API, using the process-wide default +// rclcpp context (the same one moveit::setNodeLoggerName() and +// moveit::getGlobalRootLogger() use internally via plain rclcpp::init()), +// rather than a private test context. This exercises the same +// init/use/shutdown sequence as the issue #3827 reporter's MWE, plus a +// second call afterwards to guard against a post-shutdown null dereference. +TEST(SetNodeLoggerNameTest, SafeAcrossExplicitShutdown) +{ + rclcpp::init(0, nullptr); + + moveit::setNodeLoggerName("logger_black_box_test"); + try + { + RCLCPP_INFO(moveit::getLogger("child"), "before shutdown"); + } + catch (const std::exception& ex) + { + FAIL() << "logging before shutdown must not throw: " << ex.what(); + } + + rclcpp::shutdown(); + + // A second call after shutdown, and continuing to log through the (now + // node-less) logger, must not crash: this is a regression guard for the + // "first call wins" static being reset out from under a later caller. + try + { + moveit::setNodeLoggerName("logger_black_box_test_after_shutdown"); + RCLCPP_INFO(moveit::getLogger("child"), "after shutdown"); + } + catch (const std::exception& ex) + { + FAIL() << "setNodeLoggerName()/logging after shutdown must not throw: " << ex.what(); + } +} + +} // namespace + +int main(int argc, char** argv) +{ + testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} From 03c1886cd9f2750244eac9286ea2c6c25cf57da8 Mon Sep 17 00:00:00 2001 From: Dennis Lanov Date: Wed, 26 Aug 2026 16:52:18 -0500 Subject: [PATCH 2/4] Address logger shutdown review feedback Signed-off-by: Dennis Lanov --- moveit_core/utils/src/logger.cpp | 24 ++--- moveit_core/utils/src/logger_detail.hpp | 87 +++++++----------- moveit_core/utils/test/CMakeLists.txt | 6 +- moveit_core/utils/test/test_logger.cpp | 114 +++++++++++++++--------- 4 files changed, 118 insertions(+), 113 deletions(-) diff --git a/moveit_core/utils/src/logger.cpp b/moveit_core/utils/src/logger.cpp index 2efdf3fc3e..bc4244a83f 100644 --- a/moveit_core/utils/src/logger.cpp +++ b/moveit_core/utils/src/logger.cpp @@ -36,7 +36,6 @@ #include #include -#include #include #include #include @@ -59,11 +58,11 @@ rclcpp::Logger& getGlobalRootLogger() { static rclcpp::Node::SharedPtr moveit_node = rclcpp::Node::make_shared(name); // See registerNodeResetOnPreShutdown()'s documentation for why the - // returned flag must exist even though it is not otherwise used here: + // returned mutex must exist even though it is not otherwise used here: // this call, immediately after constructing moveit_node, is what makes - // moveit_node's destruction ordering (relative to the flag) safe. - static std::shared_ptr flag = detail::registerNodeResetOnPreShutdown(moveit_node); - (void)flag; + // moveit_node's destruction ordering (relative to the mutex) safe. + static std::shared_ptr s_mutex = detail::registerNodeResetOnPreShutdown(moveit_node); + (void)s_mutex; return moveit_node->get_logger(); } catch (const std::exception& ex) @@ -80,17 +79,20 @@ rclcpp::Logger& getGlobalRootLogger() void setNodeLoggerName(const std::string& name) { - static rclcpp::Node::SharedPtr node = std::make_shared("moveit", name); - static std::shared_ptr flag = detail::registerNodeResetOnPreShutdown(node); + static rclcpp::Node::SharedPtr s_node = std::make_shared("moveit", name); + static std::shared_ptr s_mutex = detail::registerNodeResetOnPreShutdown(s_node); - std::lock_guard lock(flag->mutex); - if (node) + std::lock_guard lock(*s_mutex); + if (s_node) { - getGlobalRootLogger() = node->get_logger(); + getGlobalRootLogger() = s_node->get_logger(); } // If the node has already been reset by a pre-shutdown callback from an // earlier rclcpp::shutdown(), leave the global logger untouched rather - // than dereferencing a destroyed node. + // than dereferencing a destroyed node. The previously assigned Logger + // remains valid: rclcpp::Logger owns its logger-name state independently + // and does not retain a reference to the Node, so destroying the Node + // does not invalidate the Logger. } rclcpp::Logger getLogger(const std::string& name) diff --git a/moveit_core/utils/src/logger_detail.hpp b/moveit_core/utils/src/logger_detail.hpp index 8b5b88147b..e624f32615 100644 --- a/moveit_core/utils/src/logger_detail.hpp +++ b/moveit_core/utils/src/logger_detail.hpp @@ -46,17 +46,6 @@ namespace moveit namespace detail { -/// Out-of-band flag + mutex used to guard a caller-owned rclcpp::Node::SharedPtr -/// against a pre-shutdown callback resetting it concurrently, and to record -/// whether that reset has already happened. Deliberately its own, separately -/// allocated object -- NOT a wrapper that also stores the Node::SharedPtr -/// itself. See registerNodeResetOnPreShutdown() for why. -struct LoggerNodeFlag -{ - std::mutex mutex; - bool retired = false; -}; - /// Arranges for `node` -- a caller-owned rclcpp::Node::SharedPtr with static /// storage duration -- to be reset from an rclcpp pre-shutdown callback /// (which runs *before* rcl_shutdown() tears down the associated RMW @@ -67,64 +56,48 @@ struct LoggerNodeFlag /// the unspecified destruction order of unrelated function-local static /// objects. See moveit/moveit2#3827. /// -/// Returns a LoggerNodeFlag the caller must lock (its mutex) before reading -/// or writing `node` afterwards, and can check (`retired`) to know whether -/// the callback already reset it. -/// -/// Two deliberate design choices here, both required together and each -/// empirically verified while developing this fix: +/// Returns a mutex the caller must lock before reading or writing `node` +/// afterwards, so a concurrent caller and pre-shutdown callback can't race +/// on it. /// -/// 1. The callback captures `node` by *reference*, not by storing a -/// shared_ptr to it (or to a wrapper struct that also owns it). Even a -/// same-process, non-heap struct that bundles an rclcpp::Node::SharedPtr -/// together with any other member was observed to break ordinary Node -/// teardown at process exit when rclcpp::shutdown() is never called -/// explicitly -- independent of, and in addition to, the reference-cycle -/// concern below. Keeping `node` a fully standalone -/// rclcpp::Node::SharedPtr, exactly as in the pre-fix code, avoids that. -/// A plain reference cannot itself ever be part of a shared_ptr reference -/// cycle. +/// Two independent, deliberate design choices: /// -/// 2. The callback captures the returned LoggerNodeFlag by *weak_ptr*, not -/// shared_ptr. rclcpp::Node (via its NodeBase) strongly owns an -/// rclcpp::Context::SharedPtr, and Context strongly owns every -/// pre-shutdown callback registered on it. A callback holding a -/// shared_ptr to state that (transitively) owns the node would close a -/// strong reference cycle back to the context -/// (Context -> callback -> state -> node -> Context) that reference -/// counting alone could never break: if rclcpp::shutdown() were never -/// called, nothing would ever be destroyed at all, rather than merely -/// being destroyed later. A weak_ptr capture means the only edge from -/// Context back to this flag is non-owning, so there is no cycle. +/// A. Node capture: the callback references the caller-owned `node` slot +/// itself, rather than strongly capturing the Node. A strong Node +/// capture could create Context -> pre-shutdown callback -> Node -> +/// Context: Context strongly owns every pre-shutdown callback registered +/// on it, and Node (via its NodeBase) strongly owns its +/// rclcpp::Context::SharedPtr, so a callback holding a shared_ptr to the +/// Node would close that cycle. A plain reference cannot itself be part +/// of a shared_ptr reference cycle, so this can't happen. /// -/// Together, this also makes the callback's behavior when -/// rclcpp::shutdown() is never called explicitly well-defined: `node` is a -/// static, so by the time it and the returned flag (constructed -/// immediately afterwards, by the caller) reach static destruction, the -/// flag -- constructed later -- is destroyed first, by the standard's -/// reverse-order-of-completed-construction rule. That drops the callback's -/// only strong-refcounted reference *before* `node` is destroyed, so by the -/// time the callback could possibly fire afterwards (e.g. from within the -/// context's own destructor), locking the weak_ptr fails and the callback -/// safely does nothing, leaving `node` to be destroyed exactly as it would -/// have been before this fix -- not fixed, but not worsened either. -inline std::shared_ptr registerNodeResetOnPreShutdown(rclcpp::Node::SharedPtr& node) +/// B. Guard/mutex capture: the callback captures the returned mutex by +/// *weak_ptr*, not shared_ptr, so that in the "rclcpp::shutdown() is +/// never called" static-destruction path, the caller-owned mutex -- +/// constructed immediately after `node`, so by the standard's +/// reverse-order-of-completed-construction rule it is destroyed *before* +/// `node` -- is already gone by the time `node` itself is destroyed. The +/// weak_ptr lock then fails if the callback fires while `node` is being +/// (or has been) destroyed, so the callback never touches the `node` +/// slot while its own static shared_ptr is itself being torn down, +/// leaving `node` to be destroyed exactly as it would have been before +/// this fix. +inline std::shared_ptr registerNodeResetOnPreShutdown(rclcpp::Node::SharedPtr& node) { - auto flag = std::make_shared(); - std::weak_ptr weak_flag = flag; - node->get_node_base_interface()->get_context()->add_pre_shutdown_callback([weak_flag, &node] { - std::shared_ptr locked = weak_flag.lock(); + auto mutex = std::make_shared(); + std::weak_ptr weak_mutex = mutex; + node->get_node_base_interface()->get_context()->add_pre_shutdown_callback([weak_mutex, &node] { + std::shared_ptr locked = weak_mutex.lock(); if (!locked) { return; } - std::lock_guard lock(locked->mutex); + std::lock_guard lock(*locked); // Drop the reference so ~rclcpp::Node runs now, while the RMW context is // still alive, instead of racing against it at static destruction time. node.reset(); - locked->retired = true; }); - return flag; + return mutex; } } // namespace detail diff --git a/moveit_core/utils/test/CMakeLists.txt b/moveit_core/utils/test/CMakeLists.txt index 83a27929f1..0bbb3e8d9a 100644 --- a/moveit_core/utils/test/CMakeLists.txt +++ b/moveit_core/utils/test/CMakeLists.txt @@ -7,9 +7,9 @@ install(TARGETS logger_dut DESTINATION lib/${PROJECT_NAME}) # Unit test for the logger's node lifecycle (regression test for # moveit/moveit2#3827). Links against the real moveit_utils library; reaches -# registerNodeResetOnPreShutdown()/LoggerNodeFlag via the internal -# (non-installed) src/logger_detail.hpp shared with logger.cpp, without exposing -# them through the public moveit/utils/logger.hpp API. +# registerNodeResetOnPreShutdown() via the internal (non-installed) +# src/logger_detail.hpp shared with logger.cpp, without exposing it through the +# public moveit/utils/logger.hpp API. find_package(ament_cmake_gtest REQUIRED) ament_add_gtest(test_logger test_logger.cpp) target_link_libraries(test_logger moveit_utils) diff --git a/moveit_core/utils/test/test_logger.cpp b/moveit_core/utils/test/test_logger.cpp index 7bd7cd00d4..6ff4fe98f3 100644 --- a/moveit_core/utils/test/test_logger.cpp +++ b/moveit_core/utils/test/test_logger.cpp @@ -38,20 +38,22 @@ // a reference cycle that would leak instead if rclcpp::shutdown() is never // called. // -// registerNodeResetOnPreShutdown()/LoggerNodeFlag live in the internal -// (non-installed) logger_detail.hpp, included by both logger.cpp (compiled -// into the real moveit_utils library) and this test, so the white-box tests -// below exercise the exact same code the library uses. This test links -// against the real moveit_utils rather than recompiling logger.cpp, and does -// not expose anything through the public moveit/utils/logger.hpp API. +// registerNodeResetOnPreShutdown() lives in the internal (non-installed) +// logger_detail.hpp, included by both logger.cpp (compiled into the real +// moveit_utils library) and this test, so the white-box tests below +// exercise the exact same code the library uses. This test links against +// the real moveit_utils rather than recompiling logger.cpp, and does not +// expose anything through the public moveit/utils/logger.hpp API. // -// Test order matters in this file: GetGlobalRootLoggerTest must run before -// SetNodeLoggerNameTest, since the latter is the only test that touches the -// process-wide default rclcpp context (via plain rclcpp::init()), and -// getGlobalRootLogger()'s underlying logger is a function-local static that -// is only ever computed once for the life of the process. GoogleTest runs -// tests within one binary in the order they are defined (no shuffling is -// enabled here), so that ordering is preserved by construction. +// GetGlobalRootLoggerTest must run before SetNodeLoggerNameTest: the latter +// is the only test that touches the process-wide default rclcpp context +// (via plain rclcpp::init()), and getGlobalRootLogger()'s underlying logger +// is a function-local static that is only ever computed once for the life +// of the process. GoogleTest runs tests within one binary in the order they +// are defined (no shuffling is enabled here), so that ordering holds by +// construction; GetGlobalRootLoggerTest also asserts its own precondition +// (no context initialized yet) so a violation fails loudly instead of +// silently exercising the wrong code path. #include "../src/logger_detail.hpp" #include @@ -75,12 +77,16 @@ namespace TEST(GetGlobalRootLoggerTest, FallsBackToNonNodeLoggerBeforeInit) { + // Precondition for this test to mean anything: no rclcpp context has been + // initialized yet in this process. getGlobalRootLogger()'s underlying + // logger is a function-local static computed once per process, so if this + // ever runs after some other test has called rclcpp::init(), it would + // silently stop testing the "before init" fallback path. Fail loudly + // instead of passing for the wrong reason. + ASSERT_FALSE(rclcpp::ok()); + // getGlobalRootLogger() must not throw or crash when no rclcpp context has // ever been initialized; it should fall back to a plain, non-node logger. - // (getGlobalRootLogger()'s underlying logger is only ever computed once - // per process, so this only meaningfully verifies the "before init" path - // if it runs before anything else in this binary calls rclcpp::init() -- - // see the file comment above.) EXPECT_NO_THROW({ rclcpp::Logger logger = moveit::getGlobalRootLogger(); }); } @@ -106,10 +112,12 @@ TEST(RegisterNodeResetOnPreShutdownTest, ExplicitShutdownDestroysNode) rclcpp::Node::SharedPtr node = std::make_shared("logger_reset_test", options); std::weak_ptr weak_node = node; - std::shared_ptr flag = moveit::detail::registerNodeResetOnPreShutdown(node); + // Must stay in scope (not just be constructed) until after context->shutdown() + // below: it is what the pre-shutdown callback's weak_ptr needs to lock + // successfully in order to reset `node`. + std::shared_ptr mutex = moveit::detail::registerNodeResetOnPreShutdown(node); EXPECT_FALSE(weak_node.expired()); - EXPECT_FALSE(flag->retired); // rcl_shutdown() runs as part of this call. If the node were destroyed // only afterwards (e.g. at static destruction), an RMW implementation @@ -120,40 +128,42 @@ TEST(RegisterNodeResetOnPreShutdownTest, ExplicitShutdownDestroysNode) EXPECT_EQ(node, nullptr) << "the caller's own node slot must be reset by the callback"; EXPECT_TRUE(weak_node.expired()) << "node must be destroyed before rcl_shutdown(), not after"; - EXPECT_TRUE(flag->retired); } -// Proves there is no Context -> callback -> flag -> node -> Context -// reference cycle: if there were, dropping the caller's (only remaining -// strong) reference to the node without ever calling context->shutdown() -// would not be enough to free it, because the context's still-registered -// callback would still be keeping it alive. This tests actual object -// destruction via weak_ptr expiry, not merely a process exit code. -TEST(RegisterNodeResetOnPreShutdownTest, NoReferenceCycleWithoutExplicitShutdown) +// Proves the callback does not strongly capture either the node or the +// mutex, by exercising the "rclcpp::shutdown() is never called" path and +// checking actual object destruction via weak_ptr expiry (not merely a +// process exit code): +// +// - if the callback strongly captured the mutex, weak_mutex would not +// expire while the context (which owns the callback) remained alive; +// - if the callback strongly captured the Node, weak_node would not expire +// either, since the context owns the callback and the Node owns the +// context; +// - with the intended weak/non-owning captures, both expire once the +// caller's own `node` and `mutex` variables go out of scope, even though +// the (still-alive) context's registered callback references them. +TEST(RegisterNodeResetOnPreShutdownTest, DoesNotRetainNodeOrGuardWithoutExplicitShutdown) { std::shared_ptr context; rclcpp::NodeOptions options = makeOptionsWithFreshContext(context); std::weak_ptr weak_node; - std::weak_ptr weak_flag; + std::weak_ptr weak_mutex; { rclcpp::Node::SharedPtr node = std::make_shared("cycle_test_node", options); weak_node = node; - std::shared_ptr flag = moveit::detail::registerNodeResetOnPreShutdown(node); - weak_flag = flag; + std::shared_ptr mutex = moveit::detail::registerNodeResetOnPreShutdown(node); + weak_mutex = mutex; EXPECT_FALSE(weak_node.expired()); - EXPECT_FALSE(weak_flag.expired()); - // Both `node` and `flag` (the only strong owners of the node and the - // flag, respectively) go out of scope here, *without* ever calling + EXPECT_FALSE(weak_mutex.expired()); + // Both `node` and `mutex` (the only strong owners of the node and the + // mutex, respectively) go out of scope here, *without* ever calling // context->shutdown() -- this is the "no explicit shutdown" case. } - // If the pre-shutdown callback held a strong reference back to the flag - // (or the flag held one to the node) that the context kept alive, these - // would still report `false`: the context is still alive and would still - // be keeping them alive through that callback. - EXPECT_TRUE(weak_flag.expired()) << "flag must not be kept alive by a reference cycle through the context"; - EXPECT_TRUE(weak_node.expired()) << "node must not be kept alive by a reference cycle through the context"; + EXPECT_TRUE(weak_mutex.expired()) << "mutex must not be kept alive by the callback's capture of it"; + EXPECT_TRUE(weak_node.expired()) << "node must not be kept alive by the callback's capture of it"; // The context itself, and its now-dangling (weak-only, already-expired) // callback registration, can be safely torn down too. @@ -171,9 +181,12 @@ TEST(SetNodeLoggerNameTest, SafeAcrossExplicitShutdown) rclcpp::init(0, nullptr); moveit::setNodeLoggerName("logger_black_box_test"); + const rclcpp::Logger logger_before_shutdown = moveit::getLogger("child"); + ASSERT_NE(logger_before_shutdown.get_name(), nullptr); + const std::string name_before_shutdown = logger_before_shutdown.get_name(); try { - RCLCPP_INFO(moveit::getLogger("child"), "before shutdown"); + RCLCPP_INFO(logger_before_shutdown, "before shutdown"); } catch (const std::exception& ex) { @@ -182,13 +195,30 @@ TEST(SetNodeLoggerNameTest, SafeAcrossExplicitShutdown) rclcpp::shutdown(); + // The pre-shutdown callback has now reset setNodeLoggerName()'s node, but + // the rclcpp::Logger previously assigned into getGlobalRootLogger() is + // unaffected: rclcpp::Logger owns its logger-name state independently and + // does not retain a reference to the Node, so its name -- and every + // logger derived from it via get_child() -- is still valid and unchanged + // here. + const rclcpp::Logger logger_after_shutdown = moveit::getLogger("child"); + ASSERT_NE(logger_after_shutdown.get_name(), nullptr); + EXPECT_STREQ(logger_after_shutdown.get_name(), name_before_shutdown.c_str()); + // A second call after shutdown, and continuing to log through the (now // node-less) logger, must not crash: this is a regression guard for the - // "first call wins" static being reset out from under a later caller. + // "first call wins" static being reset out from under a later caller. The + // name is deliberately unchanged from before shutdown: with the node + // already reset, setNodeLoggerName() leaves getGlobalRootLogger() as-is + // rather than dereferencing the destroyed node. try { moveit::setNodeLoggerName("logger_black_box_test_after_shutdown"); - RCLCPP_INFO(moveit::getLogger("child"), "after shutdown"); + const rclcpp::Logger logger_after_second_call = moveit::getLogger("child"); + ASSERT_NE(logger_after_second_call.get_name(), nullptr); + EXPECT_STREQ(logger_after_second_call.get_name(), name_before_shutdown.c_str()) + << "setNodeLoggerName() must not change the logger after its node has already been reset"; + RCLCPP_INFO(logger_after_second_call, "after shutdown"); } catch (const std::exception& ex) { From 2c5284f53b6c0940ed435bc4137875568d77d8bb Mon Sep 17 00:00:00 2001 From: Dennis Lanov Date: Wed, 26 Aug 2026 17:13:10 -0500 Subject: [PATCH 3/4] Fix logger shutdown race and isolate fallback test Signed-off-by: Dennis Lanov --- moveit_core/utils/src/logger.cpp | 20 ++++-- moveit_core/utils/test/CMakeLists.txt | 7 ++ moveit_core/utils/test/test_logger.cpp | 64 ++++++++--------- .../utils/test/test_logger_before_init.cpp | 72 +++++++++++++++++++ 4 files changed, 123 insertions(+), 40 deletions(-) create mode 100644 moveit_core/utils/test/test_logger_before_init.cpp diff --git a/moveit_core/utils/src/logger.cpp b/moveit_core/utils/src/logger.cpp index bc4244a83f..4ad9afd579 100644 --- a/moveit_core/utils/src/logger.cpp +++ b/moveit_core/utils/src/logger.cpp @@ -57,13 +57,21 @@ rclcpp::Logger& getGlobalRootLogger() try { static rclcpp::Node::SharedPtr moveit_node = rclcpp::Node::make_shared(name); - // See registerNodeResetOnPreShutdown()'s documentation for why the - // returned mutex must exist even though it is not otherwise used here: - // this call, immediately after constructing moveit_node, is what makes - // moveit_node's destruction ordering (relative to the mutex) safe. static std::shared_ptr s_mutex = detail::registerNodeResetOnPreShutdown(moveit_node); - (void)s_mutex; - return moveit_node->get_logger(); + + // The pre-shutdown callback registered above can run concurrently on + // another thread as soon as it's registered (e.g. if rclcpp::shutdown() + // races with this, the very first, call to getGlobalRootLogger()), and + // may reset moveit_node to null. Lock the same mutex the callback locks + // before reading moveit_node, so the read and the reset can't race. + std::lock_guard lock(*s_mutex); + if (moveit_node) + { + return moveit_node->get_logger(); + } + // Shutdown's pre-shutdown callback already reset the node: fall back to + // a plain, non-node logger instead of dereferencing a destroyed node. + return rclcpp::get_logger(name); } catch (const std::exception& ex) { diff --git a/moveit_core/utils/test/CMakeLists.txt b/moveit_core/utils/test/CMakeLists.txt index 0bbb3e8d9a..44467e6564 100644 --- a/moveit_core/utils/test/CMakeLists.txt +++ b/moveit_core/utils/test/CMakeLists.txt @@ -14,6 +14,13 @@ find_package(ament_cmake_gtest REQUIRED) ament_add_gtest(test_logger test_logger.cpp) target_link_libraries(test_logger moveit_utils) +# Regression test for getGlobalRootLogger()'s before-rclcpp::init() fallback. +# Deliberately its own single-test executable/process -- see the file comment in +# test_logger_before_init.cpp for why this can't be a test case inside +# test_logger.cpp above. +ament_add_gtest(test_logger_before_init test_logger_before_init.cpp) +target_link_libraries(test_logger_before_init moveit_utils) + find_package(launch_testing_ament_cmake) # These tests do not work on Humble as /rosout logging from child loggers does diff --git a/moveit_core/utils/test/test_logger.cpp b/moveit_core/utils/test/test_logger.cpp index 6ff4fe98f3..c05d7fb34c 100644 --- a/moveit_core/utils/test/test_logger.cpp +++ b/moveit_core/utils/test/test_logger.cpp @@ -45,15 +45,10 @@ // the real moveit_utils rather than recompiling logger.cpp, and does not // expose anything through the public moveit/utils/logger.hpp API. // -// GetGlobalRootLoggerTest must run before SetNodeLoggerNameTest: the latter -// is the only test that touches the process-wide default rclcpp context -// (via plain rclcpp::init()), and getGlobalRootLogger()'s underlying logger -// is a function-local static that is only ever computed once for the life -// of the process. GoogleTest runs tests within one binary in the order they -// are defined (no shuffling is enabled here), so that ordering holds by -// construction; GetGlobalRootLoggerTest also asserts its own precondition -// (no context initialized yet) so a violation fails loudly instead of -// silently exercising the wrong code path. +// getGlobalRootLogger()'s before-rclcpp::init() fallback is covered +// separately, by its own single-test executable/process +// (test_logger_before_init.cpp) -- not here, since that behavior can only +// be observed by the first thing in a process to touch it. #include "../src/logger_detail.hpp" #include @@ -62,34 +57,9 @@ #include #include -namespace moveit -{ -// getGlobalRootLogger() has external linkage (it is not static/anonymous), -// but is intentionally not declared in the public logger.hpp -- it is an -// implementation detail of setNodeLoggerName()/getLogger(). Forward-declare -// it here to exercise its no-init fallback path directly, without adding it -// to the public header. -rclcpp::Logger& getGlobalRootLogger(); -} // namespace moveit - namespace { -TEST(GetGlobalRootLoggerTest, FallsBackToNonNodeLoggerBeforeInit) -{ - // Precondition for this test to mean anything: no rclcpp context has been - // initialized yet in this process. getGlobalRootLogger()'s underlying - // logger is a function-local static computed once per process, so if this - // ever runs after some other test has called rclcpp::init(), it would - // silently stop testing the "before init" fallback path. Fail loudly - // instead of passing for the wrong reason. - ASSERT_FALSE(rclcpp::ok()); - - // getGlobalRootLogger() must not throw or crash when no rclcpp context has - // ever been initialized; it should fall back to a plain, non-node logger. - EXPECT_NO_THROW({ rclcpp::Logger logger = moveit::getGlobalRootLogger(); }); -} - // Each white-box test below uses its own private rclcpp::Context (rather // than the process default one) so tests are fully isolated from one // another: rclcpp pre/on-shutdown callbacks are never removed from a @@ -130,6 +100,32 @@ TEST(RegisterNodeResetOnPreShutdownTest, ExplicitShutdownDestroysNode) EXPECT_TRUE(weak_node.expired()) << "node must be destroyed before rcl_shutdown(), not after"; } +// Regression test for a race CodeRabbit flagged in getGlobalRootLogger(): +// once registerNodeResetOnPreShutdown() returns, a concurrent +// rclcpp::shutdown() can reset the node at any point afterwards, including +// before the caller's first read of it. This deterministically exercises +// the worst case of that race -- the reset having already happened by the +// time the read takes its lock -- without needing actual concurrent +// threads (which would make the test flaky). It proves the lock-then-check +// pattern getGlobalRootLogger() and setNodeLoggerName() both use is safe: +// once the same mutex the callback locks is held, the node is never +// dereferenced without first being checked for null. +TEST(RegisterNodeResetOnPreShutdownTest, LockedReadAfterResetDoesNotDereferenceNull) +{ + std::shared_ptr context; + rclcpp::NodeOptions options = makeOptionsWithFreshContext(context); + + rclcpp::Node::SharedPtr node = std::make_shared("race_test_node", options); + std::shared_ptr mutex = moveit::detail::registerNodeResetOnPreShutdown(node); + + // Simulate a shutdown racing ahead of the first locked read. + context->shutdown("simulate a shutdown racing ahead of the first read"); + ASSERT_EQ(node, nullptr); + + std::lock_guard lock(*mutex); + EXPECT_FALSE(static_cast(node)) << "node must be safely observed as reset while holding the lock"; +} + // Proves the callback does not strongly capture either the node or the // mutex, by exercising the "rclcpp::shutdown() is never called" path and // checking actual object destruction via weak_ptr expiry (not merely a diff --git a/moveit_core/utils/test/test_logger_before_init.cpp b/moveit_core/utils/test/test_logger_before_init.cpp new file mode 100644 index 0000000000..6f156fe4cd --- /dev/null +++ b/moveit_core/utils/test/test_logger_before_init.cpp @@ -0,0 +1,72 @@ +/********************************************************************* + * Software License Agreement (BSD License) + * + * Copyright (c) 2026, PickNik Inc. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following + * disclaimer in the documentation and/or other materials provided + * with the distribution. + * * Neither the name of PickNik Inc. nor the names of its + * contributors may be used to endorse or promote products derived + * from this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE + * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, + * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER + * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN + * ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE + * POSSIBILITY OF SUCH DAMAGE. + *********************************************************************/ + +// Regression test for moveit/moveit2#3827's before-rclcpp::init() fallback +// path. getGlobalRootLogger()'s underlying rclcpp::Logger is a +// function-local static computed exactly once per process, so this test +// must be the first thing in its process to touch it -- it is deliberately +// its own single-test executable (its own OS process under ctest), rather +// than a test case inside test_logger.cpp, so nothing else can call +// rclcpp::init() first regardless of gtest execution order. +// +// Uses the public API (moveit::getLogger()) rather than the private +// getGlobalRootLogger(), since getLogger() calls getGlobalRootLogger() +// internally (see logger.cpp) and so exercises the same fallback without +// needing to forward-declare an implementation-detail symbol. +#include +#include +#include + +namespace +{ + +TEST(LoggerBeforeInitTest, GetLoggerFallsBackWithoutNode) +{ + ASSERT_FALSE(rclcpp::ok()) << "this test must run alone, in a fresh process, before rclcpp::init()"; + + // moveit::getLogger() must not throw or crash when no rclcpp context has + // ever been initialized; it should fall back to a plain, non-node logger. + EXPECT_NO_THROW({ moveit::getLogger("child"); }); + + const rclcpp::Logger logger = moveit::getLogger("child"); + EXPECT_NE(logger.get_name(), nullptr); +} + +} // namespace + +int main(int argc, char** argv) +{ + testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} From 5e70ed9fb7af7e943e198e1148e8a711218479c5 Mon Sep 17 00:00:00 2001 From: Dennis Lanov Date: Thu, 27 Aug 2026 11:31:11 -0500 Subject: [PATCH 4/4] Fix custom gtest main linkage --- moveit_core/utils/test/CMakeLists.txt | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/moveit_core/utils/test/CMakeLists.txt b/moveit_core/utils/test/CMakeLists.txt index 44467e6564..9b1e86549e 100644 --- a/moveit_core/utils/test/CMakeLists.txt +++ b/moveit_core/utils/test/CMakeLists.txt @@ -18,7 +18,11 @@ target_link_libraries(test_logger moveit_utils) # Deliberately its own single-test executable/process -- see the file comment in # test_logger_before_init.cpp for why this can't be a test case inside # test_logger.cpp above. -ament_add_gtest(test_logger_before_init test_logger_before_init.cpp) +ament_add_gtest( + test_logger_before_init + test_logger_before_init.cpp + SKIP_LINKING_MAIN_LIBRARIES +) target_link_libraries(test_logger_before_init moveit_utils) find_package(launch_testing_ament_cmake)