Fix static destruction ordering issue on macOS #16
Conversation
|
Thank you for the detailed description. I completely agree with the rational to move the restoring into the destructor of
Does catkin_run_tests_target satisfy this? That function is being used when registering a |
In principle yes, but it would still require some glue like in catkin/gtest.cmake:81-89 that has to be repeated for each test executable and that could be wrapped in its own catkin function: function(catkin_add_test name_or_target
cmake_parse_arguments(_testing "" "" "COMMAND" ${ARGN})
set(target)
set(name "${name_or_target}")
# unparsed arguments are forwarded to catkin_run_tests_target()
# for compatibility with add_test(), which accepts a target name after COMMAND
if(_testing_COMMAND AND TARGET "${_testing_COMMAND}")
set(target "${_testing_COMMAND}")
set(_testing_COMMAND)
elseif(TARGET "${name_or_target}")
set(target "${name_or_target}")
endif()
if(TARGET ${target})
# make sure the target is built before running tests
add_dependencies(tests ${target})
# get command from executable output path
if(NOT _testing_COMMAND)
# get_target_property() may be replaced by a generator expression in modern CMake?
get_target_property(_target_path ${target} RUNTIME_OUTPUT_DIRECTORY)
get_target_property(_target_executable ${target} RUNTIME_OUTPUT_NAME)
set(_testing_COMMAND "${_target_path}/${_target_executable }")
endif()
endif()
# The test executable does not produce a junit result file plain-${name}.xml, but
# we have to add one in the catkin_run_tests_target() call...
catkin_run_tests_target("plain" ${name} "plain-${name}.xml"
COMMAND "${_testing_COMMAND}"
DEPENDENCIES ${target}
${_testing_UNPARSED_ARGUMENTS})
endfunction()
# in test/CMakeLists.txt
add_executable(cleanup cleanup.cpp)
catkin_add_test(cleanup)This function would create a custom An alternative would be to make |
|
just for note, and I do not know much about I have environment with which we have installed rosconsole-bridge a few month ago, and just install ros-kinetic-urdf yesterday. And this fail to compile our packages . Ofcourse if we install https://api.travis-ci.org/v3/job/412049716/log.txt |
This is a follow-up of #10 (fix: on destruction OutputHandlerROS should restore the previous handler).
Problem Description
While for Linux the proposed fix works nicely and avoids the pure virtual method call demonstrated by the
cleanupunit test, on macOS the added call toconsole_bridge::restorePreviousOutputHandler()in the destructor ofOutputHandlerROStriggers an exception because this mutex inconsole_bridge'sstatic DefaultHandler DOHstruct has already been destructed when the call toconsole_bridge::restorePreviousOutputHandler()tries to lock it during the static destruction of RegisterOutputHandlerProxy.(after quitting rviz with ROS kinetic built from source on macOS Sierra 10.12, with system dependencies installed from HomeBrew, with console_bridge 0.4.0 and rosconsole_bridge version 0.5.1)
I could not reproduce this problem for "normal" executables that only link statically to dynamic libraries, but the exception is triggered consistently if the application loaded libraries dynamically at run-time (
dlopen(), e.g. through pluginlib) that directly or indirectly link tolibrosconsole_bridge.dylib.The root cause is likely the different ordering of Linux and Darwin during the static initialization and destruction phase. While Linux initializes all global symbols right after a dynamic library has been loaded, Darwin only initializes them on first use. Apparently that is what is called "deferred dynamic initialization" in [1], which is allowed by the C++ standard [2]. The destruction happens in strictly reverse order of initialization in both cases, where the completion of the constructor or dynamic initialization is the synchronization point [3].
If the construction of OutputHandlerROS called during static initialization of oh_ros from the constructor of RegisterOutputHandlerProxy finishes before the first call to getDOH(), which in turn initializes the mutex as part of the static initialization of DefaultOutputHandler DOH, the mutex also gets destroyed before the destruction of oh_ros, which calls into console_bridge::restorePreviousOutputHandler and tries to lock it.
Proposed patch
The solution is to make sure that the output handler is restored during the destruction of exactly the same object that installed a new one, namely
RegisterOutputHandlerProxy. If it's not the constructor ofOutputHandlerROSthat callsconsole_bridge::useOutputHandler(this), it should also not be the destructors responsibility to callconsole_bridge::restorePreviousOutputHandler().Note that there might be more than one instance of
RegisterOutputHandlerProxyin a single process because it can be instantiated once for every compilation unit that included the REGISTER_ROSCONSOLE_BRIDGE macro that defines the symbolrosconsole_bridge::RegisterOutputHandlerProxy __register_rosconsole_output_handler_proxy, depending on whether it is loaded with RTLD_GLOBAL or not, and they refer to either the same or a different instance of static OutputHandlerROS oh_ros.ABI compatibility (tested on Xenial only)
I think the same reasoning about ABI-compatibility as in #10 (comment) applies to this patch, too:
But as the definition of
RegisterOutputHandlerProxyhas been changed, which is actually instantiated in another compilation unit (e.g.liburdf.so), we also have to test this one:That being said, even if ABI compatible, if only rosconsole_bridge would be recompiled/upgraded, but not the compilation unit that expands the REGISTER_ROSCONSOLE_BRIDGE macro and instantiates
RegisterOutputHandlerProxy, none of the two destructors would be called which might result in the pure virtual method call during static destruction again.Unit test
The add_test() call in test/CMakeLists.txt:4 was missing a
NAMEkeyword, which is why it always failed for me when running through the targettestgenerated by cmake, because it tried to execute a command calledCOMMAND. Also note that the unit test is not using catkin's test infrastructure which defines the targetstestsandrun_testsand only provides macros for nosetests and gtest. It will not run automatically onmake run_testsor on the ROS build farm. In order to fix that catkin should provide a macro likecatkin_add_test()that is not specific to a certain testing library and can execute arbitrary commands, but which integrates with itsrun_teststarget.References
[1] https://en.cppreference.com/w/cpp/language/initialization#Deferred_dynamic_initialization
[2] http://eel.is/c++draft/basic.start.dynamic#4
[3] http://eel.is/c++draft/basic.start#term-3