From 05f6781c0ec9658f5aa1189f31d9ae223b9d1b2f Mon Sep 17 00:00:00 2001
From: yadunund <yadunund@gmail.com>
Date: Thu, 23 Jul 2026 09:25:00 -0700
Subject: [PATCH] Run RMW tests in isolation and call shutdown on error (#2460)

Signed-off-by: Yadunund Vijay <yadunund@gmail.com>
(cherry picked from commit 2777970aabbd33d817a93b8c905f4fea08b64e71)

# Conflicts:
#	rosbag2_py/src/rosbag2_py/_transport.cpp
---
 rosbag2_py/CMakeLists.txt                |  2 +-
 rosbag2_py/src/rosbag2_py/_transport.cpp | 42 ++++++++++++++++++++++--
 rosbag2_tests/CMakeLists.txt             |  7 ++--
 rosbag2_tests/package.xml                |  1 +
 4 files changed, 45 insertions(+), 7 deletions(-)

diff --git a/rosbag2_py/CMakeLists.txt b/rosbag2_py/CMakeLists.txt
index 15bdb9463a..ad74a5753d 100644
--- a/rosbag2_py/CMakeLists.txt
+++ b/rosbag2_py/CMakeLists.txt
@@ -163,7 +163,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 fb2d737707..af98a69233 100644
--- a/rosbag2_py/src/rosbag2_py/_transport.cpp
+++ b/rosbag2_py/src/rosbag2_py/_transport.cpp
@@ -194,7 +194,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()
@@ -579,13 +591,37 @@ class Recorder
     rclcpp::init(arguments.argc(), arguments.argv(),
                  rclcpp::InitOptions(), rclcpp::SignalHandlerOptions::None);
 
+<<<<<<< HEAD
     if (record_options.rmw_serialization_format.empty()) {
       record_options.rmw_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);
+>>>>>>> 2777970 (Run RMW tests in isolation and call shutdown on error (#2460))
 
-    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()
diff --git a/rosbag2_tests/CMakeLists.txt b/rosbag2_tests/CMakeLists.txt
index 412fd52ba1..21e823ef2d 100644
--- a/rosbag2_tests/CMakeLists.txt
+++ b/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)
diff --git a/rosbag2_tests/package.xml b/rosbag2_tests/package.xml
index ff150670cf..963d8c0f95 100644
--- a/rosbag2_tests/package.xml
+++ b/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>