From 4d95b7b00dec746e75d3efd6eb96891cfa59ad7b Mon Sep 17 00:00:00 2001 From: Emerson Knapp Date: Thu, 24 Jun 2021 16:33:50 -0700 Subject: [PATCH 1/5] Add SIGTERM regression test, with fix commented out Signed-off-by: Emerson Knapp --- rosbag2_py/src/rosbag2_py/_transport.cpp | 4 ++++ .../process_execution_helpers_unix.hpp | 4 ++-- .../process_execution_helpers_windows.hpp | 4 +++- .../test_rosbag2_record_end_to_end.cpp | 14 ++++++++++++++ 4 files changed, 23 insertions(+), 3 deletions(-) diff --git a/rosbag2_py/src/rosbag2_py/_transport.cpp b/rosbag2_py/src/rosbag2_py/_transport.cpp index 01fb7ae299..45b7d5f58e 100644 --- a/rosbag2_py/src/rosbag2_py/_transport.cpp +++ b/rosbag2_py/src/rosbag2_py/_transport.cpp @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include #include #include #include @@ -145,6 +146,9 @@ class Recorder Recorder() { rclcpp::init(0, nullptr); + // std::signal(SIGTERM, [](int/* signal */) { + // rclcpp::shutdown(); + // }); } virtual ~Recorder() diff --git a/rosbag2_test_common/include/rosbag2_test_common/process_execution_helpers_unix.hpp b/rosbag2_test_common/include/rosbag2_test_common/process_execution_helpers_unix.hpp index f03e1c07f2..a1eb170d9f 100644 --- a/rosbag2_test_common/include/rosbag2_test_common/process_execution_helpers_unix.hpp +++ b/rosbag2_test_common/include/rosbag2_test_common/process_execution_helpers_unix.hpp @@ -59,9 +59,9 @@ ProcessHandle start_execution(const std::string & command) return process_id; } -void stop_execution(const ProcessHandle & handle) +void stop_execution(const ProcessHandle & handle, int signum = SIGINT) { - killpg(handle, SIGINT); + killpg(handle, signum); int child_return_code; waitpid(handle, &child_return_code, 0); // this call will make sure that the process does execute without issues before it is killed by diff --git a/rosbag2_test_common/include/rosbag2_test_common/process_execution_helpers_windows.hpp b/rosbag2_test_common/include/rosbag2_test_common/process_execution_helpers_windows.hpp index 98b4eabb34..3b3347b13b 100644 --- a/rosbag2_test_common/include/rosbag2_test_common/process_execution_helpers_windows.hpp +++ b/rosbag2_test_common/include/rosbag2_test_common/process_execution_helpers_windows.hpp @@ -104,8 +104,10 @@ ProcessHandle start_execution(const std::string & command) return process; } -void stop_execution(const ProcessHandle & handle) +void stop_execution(const ProcessHandle & handle, int signum = SIGINT) { + // Match the Unix version by allowing for int signum argument - however Windows does not have + // Linux signals in the same way, so there isn't a 1:1 mapping to dispatch e.g. SIGTERM DWORD exit_code; GetExitCodeProcess(handle.process_info.hProcess, &exit_code); // 259 indicates that the process is still active: we want to make sure that the process is diff --git a/rosbag2_tests/test/rosbag2_tests/test_rosbag2_record_end_to_end.cpp b/rosbag2_tests/test/rosbag2_tests/test_rosbag2_record_end_to_end.cpp index 1a039a41ab..763c5b826e 100644 --- a/rosbag2_tests/test/rosbag2_tests/test_rosbag2_record_end_to_end.cpp +++ b/rosbag2_tests/test/rosbag2_tests/test_rosbag2_record_end_to_end.cpp @@ -180,6 +180,20 @@ TEST_F(RecordFixture, record_end_to_end_test) { EXPECT_THAT(wrong_topic_messages, IsEmpty()); } +TEST_F(RecordFixture, record_end_to_end_exits_gracefully_on_sigterm) { + const std::string topic_name = "/test_sigterm"; + auto message = get_messages_strings()[0]; + message->string_value = "test"; + rosbag2_test_common::PublicationManager pub_manager; + pub_manager.setup_publisher(topic_name, message, 10); + auto process_handle = start_execution( + "ros2 bag record --output " + root_bag_path_.string() + " " + topic_name); + wait_for_db(); + pub_manager.run_publishers(); + stop_execution(process_handle, SIGTERM); + wait_for_metadata(); +} + // TODO(zmichaels11): Fix and enable this test on Windows. // This tests depends on the ability to read the metadata file. // Stopping the process on Windows does a hard kill and the metadata file is not written. From 287dd2c3820ab19b07dd97e63dd136571536bfa2 Mon Sep 17 00:00:00 2001 From: Emerson Knapp Date: Thu, 24 Jun 2021 18:25:13 -0700 Subject: [PATCH 2/5] Disable xfail disabling - it's unfortunate we can't run it but just not mark as failure - gotta fix Signed-off-by: Emerson Knapp --- .github/workflows/test.yml | 2 -- 1 file changed, 2 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 1019ebe888..18bd8221fc 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -45,8 +45,6 @@ jobs: ] }, "test": { - "ctest-args": ["-LE", "xfail"], - "pytest-args": ["-m", "not xfail"] } } - name: Run xfail tests (not required to succeed) From 79aeb53dc1e8155146956b99e3e70b698480b632 Mon Sep 17 00:00:00 2001 From: Emerson Knapp Date: Wed, 30 Jun 2021 15:55:59 -0700 Subject: [PATCH 3/5] Enable the testfix Signed-off-by: Emerson Knapp --- rosbag2_py/src/rosbag2_py/_transport.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/rosbag2_py/src/rosbag2_py/_transport.cpp b/rosbag2_py/src/rosbag2_py/_transport.cpp index 45b7d5f58e..85013b62fc 100644 --- a/rosbag2_py/src/rosbag2_py/_transport.cpp +++ b/rosbag2_py/src/rosbag2_py/_transport.cpp @@ -146,9 +146,10 @@ class Recorder Recorder() { rclcpp::init(0, nullptr); - // std::signal(SIGTERM, [](int/* signal */) { - // rclcpp::shutdown(); - // }); + std::signal( + SIGTERM, [](int /* signal */) { + rclcpp::shutdown(); + }); } virtual ~Recorder() From fb580745a36aa231163ef19324e063aa1e5b7639 Mon Sep 17 00:00:00 2001 From: Emerson Knapp Date: Thu, 1 Jul 2021 17:04:27 -0700 Subject: [PATCH 4/5] Re-disable xfail tests Signed-off-by: Emerson Knapp --- .github/workflows/test.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 18bd8221fc..1019ebe888 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -45,6 +45,8 @@ jobs: ] }, "test": { + "ctest-args": ["-LE", "xfail"], + "pytest-args": ["-m", "not xfail"] } } - name: Run xfail tests (not required to succeed) From fb3e3d170f68fbf9ff26975d3e9a87816bc86522 Mon Sep 17 00:00:00 2001 From: Emerson Knapp Date: Fri, 2 Jul 2021 15:45:24 -0700 Subject: [PATCH 5/5] Include csignal header for windows Signed-off-by: Emerson Knapp --- .../rosbag2_test_common/process_execution_helpers_windows.hpp | 1 + 1 file changed, 1 insertion(+) diff --git a/rosbag2_test_common/include/rosbag2_test_common/process_execution_helpers_windows.hpp b/rosbag2_test_common/include/rosbag2_test_common/process_execution_helpers_windows.hpp index 3b3347b13b..ec21d01e31 100644 --- a/rosbag2_test_common/include/rosbag2_test_common/process_execution_helpers_windows.hpp +++ b/rosbag2_test_common/include/rosbag2_test_common/process_execution_helpers_windows.hpp @@ -21,6 +21,7 @@ #include #include +#include #include #include #include