diff --git a/rosbag2_py/CMakeLists.txt b/rosbag2_py/CMakeLists.txt index 2a75ae335..3c46c62d9 100644 --- a/rosbag2_py/CMakeLists.txt +++ b/rosbag2_py/CMakeLists.txt @@ -176,7 +176,7 @@ if(BUILD_TESTING) APPEND_ENV "${append_env_vars}" ENV "${set_env_vars}" ) - ament_add_pytest_test(test_transport_py + ament_add_ros_isolated_pytest_test(test_transport_py "test/test_transport.py" APPEND_ENV "${append_env_vars}" ENV "${set_env_vars}" diff --git a/rosbag2_py/src/rosbag2_py/_transport.cpp b/rosbag2_py/src/rosbag2_py/_transport.cpp index 51ac2763e..c63123e61 100644 --- a/rosbag2_py/src/rosbag2_py/_transport.cpp +++ b/rosbag2_py/src/rosbag2_py/_transport.cpp @@ -195,7 +195,19 @@ class Player // Don't install signal handlers to keep signal handling simple in the Python layer rclcpp::init(arguments.argc(), arguments.argv(), rclcpp::InitOptions(), rclcpp::SignalHandlerOptions::None); - player_ = std::make_shared(storage_options, play_options, node_name); + // If constructing the Player fails (e.g. the bag needs a converter plugin + // that is not installed), the destructor never runs, so rclcpp::shutdown() + // would otherwise only be called when the global Context is destroyed at + // process exit. + // Ensure the Context is shut down on the failure path so the program can exit + // cleanly with the expected error code. + try { + player_ = + std::make_shared(storage_options, play_options, node_name); + } catch (...) { + rclcpp::shutdown(); + throw; + } } virtual ~Player() @@ -580,23 +592,30 @@ class Recorder rclcpp::init(arguments.argc(), arguments.argv(), rclcpp::InitOptions(), rclcpp::SignalHandlerOptions::None); - if (!record_options.rmw_serialization_format.empty() && - record_options.output_serialization_format.empty()) - { - record_options.output_serialization_format = record_options.rmw_serialization_format; - PyErr_WarnEx(PyExc_DeprecationWarning, - "The rmw_serialization_format option is deprecated and will be removed in a " - "future release.\nPlease use output_serialization_format instead.", - 1 - ); - } - if (record_options.output_serialization_format.empty()) { - record_options.output_serialization_format = std::string(rmw_get_serialization_format()); - } - auto writer = rosbag2_transport::ReaderWriterFactory::make_writer(record_options); + // See the Player constructor: if constructing the Recorder throws, shut the + // Context down here so it is not left to be destroyed at process exit. + try { + if (!record_options.rmw_serialization_format.empty() && + record_options.output_serialization_format.empty()) + { + record_options.output_serialization_format = record_options.rmw_serialization_format; + PyErr_WarnEx(PyExc_DeprecationWarning, + "The rmw_serialization_format option is deprecated and will be removed in a " + "future release.\nPlease use output_serialization_format instead.", + 1 + ); + } + if (record_options.output_serialization_format.empty()) { + record_options.output_serialization_format = std::string(rmw_get_serialization_format()); + } + auto writer = rosbag2_transport::ReaderWriterFactory::make_writer(record_options); - recorder_ = std::make_shared( - std::move(writer), storage_options, record_options, node_name); + recorder_ = std::make_shared( + std::move(writer), storage_options, record_options, node_name); + } catch (...) { + rclcpp::shutdown(); + throw; + } } virtual ~Recorder() diff --git a/rosbag2_tests/CMakeLists.txt b/rosbag2_tests/CMakeLists.txt index 528957d90..e17f66a33 100644 --- a/rosbag2_tests/CMakeLists.txt +++ b/rosbag2_tests/CMakeLists.txt @@ -18,6 +18,7 @@ find_package(ament_index_cpp REQUIRED) if(BUILD_TESTING) find_package(ament_cmake_gmock REQUIRED) + find_package(ament_cmake_ros REQUIRED) find_package(ament_lint_auto REQUIRED) ament_lint_auto_find_test_dependencies() @@ -32,7 +33,7 @@ if(BUILD_TESTING) find_package(std_msgs REQUIRED) find_package(test_msgs REQUIRED) - ament_add_gmock(test_rosbag2_record_end_to_end + ament_add_ros_isolated_gmock(test_rosbag2_record_end_to_end test/rosbag2_tests/test_rosbag2_record_end_to_end.cpp WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} TIMEOUT 180) @@ -49,7 +50,7 @@ if(BUILD_TESTING) ament_add_test_label(test_rosbag2_record_end_to_end xfail) endif() - ament_add_gmock(test_rosbag2_play_end_to_end + ament_add_ros_isolated_gmock(test_rosbag2_play_end_to_end test/rosbag2_tests/test_rosbag2_play_end_to_end.cpp WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}) if(TARGET test_rosbag2_play_end_to_end) @@ -116,7 +117,7 @@ if(BUILD_TESTING) ) endif() - ament_add_gmock(test_rosbag2_cpp_get_service_info + ament_add_ros_isolated_gmock(test_rosbag2_cpp_get_service_info test/rosbag2_tests/test_rosbag2_cpp_get_service_info.cpp WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}) if(TARGET test_rosbag2_cpp_get_service_info) diff --git a/rosbag2_tests/package.xml b/rosbag2_tests/package.xml index 05590c3d5..c9f661d87 100644 --- a/rosbag2_tests/package.xml +++ b/rosbag2_tests/package.xml @@ -17,6 +17,7 @@ ament_index_cpp ament_cmake_gmock + ament_cmake_ros ament_lint_auto ament_lint_common rclcpp