Run RMW tests in isolation and call shutdown on error (backport #2460)

rosbag2_py/CMakeLists.txt

@@ -162,7 +162,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}"

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<rosbag2_transport::Player>(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<rosbag2_transport::Player>(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<rosbag2_transport::Recorder>(
-      std::move(writer), storage_options, record_options, node_name);
+      recorder_ = std::make_shared<rosbag2_transport::Recorder>(
+        std::move(writer), storage_options, record_options, node_name);
+    } catch (...) {
+      rclcpp::shutdown();
+      throw;
+    }
   }
 
   virtual ~Recorder()

rosbag2_tests/CMakeLists.txt

@@ -28,6 +28,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()
 
@@ -42,7 +43,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)
@@ -58,7 +59,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)
@@ -120,7 +121,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)

rosbag2_tests/package.xml

@@ -16,6 +16,7 @@
   <depend>ament_index_cpp</depend>
 
   <test_depend>ament_cmake_gmock</test_depend>
+  <test_depend>ament_cmake_ros</test_depend>
   <test_depend>ament_lint_auto</test_depend>
   <test_depend>ament_lint_common</test_depend>
   <test_depend>rclcpp</test_depend>