From 4401542724e962d38291d2f5bf8a3e063784ba6e Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Tue, 15 Dec 2020 13:16:57 +0000 Subject: [PATCH] Revert "Ensure typesupport handle functions do not throw. (#99)" This reverts commit e513ffb524b2e89869188cb3b527fcb093cec5ce. --- .../src/type_support_dispatch.hpp | 22 +++---- .../message_type_support_dispatch.hpp | 2 +- .../service_type_support_dispatch.hpp | 2 +- .../src/message_type_support_dispatch.cpp | 2 +- .../src/service_type_support_dispatch.cpp | 2 +- .../src/type_support_dispatch.hpp | 58 ++++++------------- .../test_message_type_support_dispatch.cpp | 27 ++++----- .../test_service_type_support_dispatch.cpp | 26 +++------ 8 files changed, 50 insertions(+), 91 deletions(-) diff --git a/rosidl_typesupport_c/src/type_support_dispatch.hpp b/rosidl_typesupport_c/src/type_support_dispatch.hpp index 106b82ce..c7839c2a 100644 --- a/rosidl_typesupport_c/src/type_support_dispatch.hpp +++ b/rosidl_typesupport_c/src/type_support_dispatch.hpp @@ -21,12 +21,12 @@ #include #include +#include #include #include "rcpputils/find_library.hpp" #include "rcpputils/shared_library.hpp" #include "rcutils/error_handling.h" -#include "rcutils/snprintf.h" #include "rosidl_typesupport_c/identifier.h" #include "rosidl_typesupport_c/type_support_map.h" @@ -55,27 +55,23 @@ get_typesupport_handle_function( if (!map->data[i]) { char library_name[1024]; - int ret = rcutils_snprintf( + snprintf( library_name, 1023, "%s__%s", map->package_name, identifier); - if (ret < 0) { - RCUTILS_SET_ERROR_MSG("Failed to format library name"); - return nullptr; - } std::string library_path; try { library_path = rcpputils::find_library_path(library_name); } catch (const std::exception & e) { RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING( - "Failed to find library '%s' due to %s", + "Failed to find library '%s' due to %s\n", library_name, e.what()); return nullptr; } if (library_path.empty()) { RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING( - "Failed to find library '%s'", library_name); + "Failed to find library '%s'\n", library_name); return nullptr; } @@ -83,11 +79,11 @@ get_typesupport_handle_function( lib = new rcpputils::SharedLibrary(library_path.c_str()); } catch (const std::runtime_error & e) { RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING( - "Could not load library %s: %s", library_path.c_str(), e.what()); + "Could not load library %s: %s\n", library_path.c_str(), e.what()); return nullptr; } catch (const std::bad_alloc & e) { RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING( - "Could not load library %s: %s", library_path.c_str(), e.what()); + "Could not load library %s: %s\n", library_path.c_str(), e.what()); return nullptr; } map->data[i] = lib; @@ -100,13 +96,13 @@ get_typesupport_handle_function( try { if (!lib->has_symbol(map->symbol_name[i])) { RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING( - "Failed to find symbol '%s' in library", map->symbol_name[i]); + "Failed to find symbol '%s' in library\n", map->symbol_name[i]); return nullptr; } sym = lib->get_symbol(map->symbol_name[i]); } catch (const std::exception & e) { RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING( - "Failed to get symbol '%s' in library: %s", + "Failed to get symbol '%s' in library: %s\n", map->symbol_name[i], e.what()); return nullptr; } @@ -118,7 +114,7 @@ get_typesupport_handle_function( } } RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING( - "Handle's typesupport identifier (%s) is not supported by this library", + "Handle's typesupport identifier (%s) is not supported by this library\n", handle->typesupport_identifier); return nullptr; } diff --git a/rosidl_typesupport_cpp/include/rosidl_typesupport_cpp/message_type_support_dispatch.hpp b/rosidl_typesupport_cpp/include/rosidl_typesupport_cpp/message_type_support_dispatch.hpp index 00c9cc98..14020501 100644 --- a/rosidl_typesupport_cpp/include/rosidl_typesupport_cpp/message_type_support_dispatch.hpp +++ b/rosidl_typesupport_cpp/include/rosidl_typesupport_cpp/message_type_support_dispatch.hpp @@ -35,7 +35,7 @@ namespace rosidl_typesupport_cpp ROSIDL_TYPESUPPORT_CPP_PUBLIC const rosidl_message_type_support_t * get_message_typesupport_handle_function( - const rosidl_message_type_support_t * handle, const char * identifier) noexcept; + const rosidl_message_type_support_t * handle, const char * identifier); } // namespace rosidl_typesupport_cpp diff --git a/rosidl_typesupport_cpp/include/rosidl_typesupport_cpp/service_type_support_dispatch.hpp b/rosidl_typesupport_cpp/include/rosidl_typesupport_cpp/service_type_support_dispatch.hpp index ff4f1132..73d94910 100644 --- a/rosidl_typesupport_cpp/include/rosidl_typesupport_cpp/service_type_support_dispatch.hpp +++ b/rosidl_typesupport_cpp/include/rosidl_typesupport_cpp/service_type_support_dispatch.hpp @@ -35,7 +35,7 @@ namespace rosidl_typesupport_cpp ROSIDL_TYPESUPPORT_CPP_PUBLIC const rosidl_service_type_support_t * get_service_typesupport_handle_function( - const rosidl_service_type_support_t * handle, const char * identifier) noexcept; + const rosidl_service_type_support_t * handle, const char * identifier); } // namespace rosidl_typesupport_cpp diff --git a/rosidl_typesupport_cpp/src/message_type_support_dispatch.cpp b/rosidl_typesupport_cpp/src/message_type_support_dispatch.cpp index 8ea4239c..84448cec 100644 --- a/rosidl_typesupport_cpp/src/message_type_support_dispatch.cpp +++ b/rosidl_typesupport_cpp/src/message_type_support_dispatch.cpp @@ -21,7 +21,7 @@ namespace rosidl_typesupport_cpp const rosidl_message_type_support_t * get_message_typesupport_handle_function( - const rosidl_message_type_support_t * handle, const char * identifier) noexcept + const rosidl_message_type_support_t * handle, const char * identifier) { return rosidl_typesupport_cpp::get_typesupport_handle_function< rosidl_message_type_support_t>(handle, identifier); diff --git a/rosidl_typesupport_cpp/src/service_type_support_dispatch.cpp b/rosidl_typesupport_cpp/src/service_type_support_dispatch.cpp index 3b725297..5ccb2ae0 100644 --- a/rosidl_typesupport_cpp/src/service_type_support_dispatch.cpp +++ b/rosidl_typesupport_cpp/src/service_type_support_dispatch.cpp @@ -21,7 +21,7 @@ namespace rosidl_typesupport_cpp const rosidl_service_type_support_t * get_service_typesupport_handle_function( - const rosidl_service_type_support_t * handle, const char * identifier) noexcept + const rosidl_service_type_support_t * handle, const char * identifier) { return rosidl_typesupport_cpp::get_typesupport_handle_function< rosidl_service_type_support_t>(handle, identifier); diff --git a/rosidl_typesupport_cpp/src/type_support_dispatch.hpp b/rosidl_typesupport_cpp/src/type_support_dispatch.hpp index 184017e6..3162d274 100644 --- a/rosidl_typesupport_cpp/src/type_support_dispatch.hpp +++ b/rosidl_typesupport_cpp/src/type_support_dispatch.hpp @@ -20,12 +20,11 @@ #include #include #include + #include #include "rcpputils/find_library.hpp" #include "rcpputils/shared_library.hpp" -#include "rcutils/error_handling.h" -#include "rcutils/snprintf.h" #include "rosidl_typesupport_c/type_support_map.h" namespace rosidl_typesupport_cpp @@ -36,7 +35,7 @@ extern const char * typesupport_identifier; template const TypeSupport * get_typesupport_handle_function( - const TypeSupport * handle, const char * identifier) noexcept + const TypeSupport * handle, const char * identifier) { if (strcmp(handle->typesupport_identifier, identifier) == 0) { return handle; @@ -52,62 +51,44 @@ get_typesupport_handle_function( rcpputils::SharedLibrary * lib = nullptr; if (!map->data[i]) { - char library_name[1024]; - int ret = rcutils_snprintf( - library_name, 1023, "%s__%s", - map->package_name, identifier); - if (ret < 0) { - RCUTILS_SET_ERROR_MSG("Failed to format library name"); - return nullptr; - } + std::string library_name{map->package_name}; + library_name += "__"; + library_name += identifier; std::string library_path; try { library_path = rcpputils::find_library_path(library_name); } catch (const std::runtime_error & e) { - RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING( - "Failed to find library '%s' due to %s", - library_name, e.what()); - return nullptr; + const std::string message = + "Failed to find library '" + library_name + "' due to " + e.what(); + throw std::runtime_error(message); } if (library_path.empty()) { - RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING( - "Failed to find library '%s'", library_name); + fprintf(stderr, "Failed to find library '%s'\n", library_name.c_str()); return nullptr; } try { lib = new rcpputils::SharedLibrary(library_path.c_str()); } catch (const std::runtime_error & e) { - RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING( - "Could not load library %s: %s", library_name, e.what()); - return nullptr; + throw std::runtime_error( + "Could not load library " + library_path + ": " + + std::string(e.what())); } catch (const std::bad_alloc & e) { - RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING( - "Could not load library %s: %s", library_name, e.what()); - return nullptr; + throw std::runtime_error( + "Could not load library " + library_path + ": " + + std::string(e.what())); } map->data[i] = lib; } auto clib = static_cast(map->data[i]); lib = const_cast(clib); - - void * sym = nullptr; - - try { - if (!lib->has_symbol(map->symbol_name[i])) { - RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING( - "Failed to find symbol '%s' in library", map->symbol_name[i]); - return nullptr; - } - sym = lib->get_symbol(map->symbol_name[i]); - } catch (const std::exception & e) { - RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING( - "Failed to get symbol '%s' in library: %s", - map->symbol_name[i], e.what()); + if (!lib->has_symbol(map->symbol_name[i])) { + fprintf(stderr, "Failed to find symbol '%s' in library\n", map->symbol_name[i]); return nullptr; } + void * sym = lib->get_symbol(map->symbol_name[i]); typedef const TypeSupport * (* funcSignature)(void); funcSignature func = reinterpret_cast(sym); @@ -115,9 +96,6 @@ get_typesupport_handle_function( return ts; } } - RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING( - "Handle's typesupport identifier (%s) is not supported by this library", - handle->typesupport_identifier); return nullptr; } diff --git a/rosidl_typesupport_cpp/test/test_message_type_support_dispatch.cpp b/rosidl_typesupport_cpp/test/test_message_type_support_dispatch.cpp index e41b1862..ef9c71c0 100644 --- a/rosidl_typesupport_cpp/test/test_message_type_support_dispatch.cpp +++ b/rosidl_typesupport_cpp/test/test_message_type_support_dispatch.cpp @@ -13,7 +13,6 @@ // limitations under the License. #include -#include "rcutils/error_handling.h" #include "rcutils/testing/fault_injection.h" #include "rcpputils/shared_library.hpp" #include "rosidl_typesupport_c/type_support_map.h" @@ -85,8 +84,6 @@ TEST(TestMessageTypeSupportDispatch, get_handle_function) { rosidl_typesupport_cpp::get_message_typesupport_handle_function( &type_support, "different_identifier"), nullptr); - EXPECT_TRUE(rcutils_error_is_set()); - rcutils_reset_error(); rosidl_message_type_support_t type_support_cpp_identifier = get_rosidl_message_type_support(rosidl_typesupport_cpp::typesupport_identifier); @@ -116,24 +113,18 @@ TEST(TestMessageTypeSupportDispatch, get_handle_function) { rosidl_typesupport_cpp::get_message_typesupport_handle_function( &type_support_cpp_identifier, "test_type_support2"), nullptr); - EXPECT_TRUE(rcutils_error_is_set()); - rcutils_reset_error(); // Library file exists, but loading shared library fails - EXPECT_EQ( + EXPECT_THROW( rosidl_typesupport_cpp::get_message_typesupport_handle_function( &type_support_cpp_identifier, - "test_type_support3"), nullptr); - EXPECT_TRUE(rcutils_error_is_set()); - rcutils_reset_error(); + "test_type_support3"), std::runtime_error); // Library doesn't exist EXPECT_EQ( rosidl_typesupport_cpp::get_message_typesupport_handle_function( &type_support_cpp_identifier, "test_type_support4"), nullptr); - EXPECT_TRUE(rcutils_error_is_set()); - rcutils_reset_error(); } TEST(TestMessageTypeSupportDispatch, get_message_typesupport_maybe_fail_test) @@ -147,12 +138,14 @@ TEST(TestMessageTypeSupportDispatch, get_message_typesupport_maybe_fail_test) RCUTILS_FAULT_INJECTION_TEST( { // load library and find symbols - auto * result = rosidl_typesupport_cpp::get_message_typesupport_handle_function( - &type_support_cpp_identifier, - "test_type_support1"); - if (nullptr == result) { - EXPECT_TRUE(rcutils_error_is_set()); - rcutils_reset_error(); + try { + auto * result = rosidl_typesupport_cpp::get_message_typesupport_handle_function( + &type_support_cpp_identifier, + "test_type_support1"); + EXPECT_NE(result, nullptr); + } catch (const std::runtime_error &) { + } catch (...) { + ADD_FAILURE() << "Unexpected exception type in fault injection test"; } }); } diff --git a/rosidl_typesupport_cpp/test/test_service_type_support_dispatch.cpp b/rosidl_typesupport_cpp/test/test_service_type_support_dispatch.cpp index e5ddeeee..faedd41a 100644 --- a/rosidl_typesupport_cpp/test/test_service_type_support_dispatch.cpp +++ b/rosidl_typesupport_cpp/test/test_service_type_support_dispatch.cpp @@ -13,7 +13,6 @@ // limitations under the License. #include -#include "rcutils/error_handling.h" #include "rcutils/testing/fault_injection.h" #include "rcpputils/shared_library.hpp" #include "rosidl_typesupport_c/type_support_map.h" @@ -85,8 +84,6 @@ TEST(TestServiceTypeSupportDispatch, get_handle_function) { rosidl_typesupport_cpp::get_service_typesupport_handle_function( &type_support, "different_identifier"), nullptr); - EXPECT_TRUE(rcutils_error_is_set()); - rcutils_reset_error(); rosidl_service_type_support_t type_support_cpp_identifier = get_rosidl_service_type_support(rosidl_typesupport_cpp::typesupport_identifier); @@ -113,24 +110,18 @@ TEST(TestServiceTypeSupportDispatch, get_handle_function) { rosidl_typesupport_cpp::get_service_typesupport_handle_function( &type_support_cpp_identifier, "test_type_support2"), nullptr); - EXPECT_TRUE(rcutils_error_is_set()); - rcutils_reset_error(); // Library file exists, but loading shared library fails - EXPECT_EQ( + EXPECT_THROW( rosidl_typesupport_cpp::get_service_typesupport_handle_function( &type_support_cpp_identifier, - "test_type_support3"), nullptr); - EXPECT_TRUE(rcutils_error_is_set()); - rcutils_reset_error(); + "test_type_support3"), std::runtime_error); // Library doesn't exist EXPECT_EQ( rosidl_typesupport_cpp::get_service_typesupport_handle_function( &type_support_cpp_identifier, "test_type_support4"), nullptr); - EXPECT_TRUE(rcutils_error_is_set()); - rcutils_reset_error(); } TEST(TestServiceTypeSupportDispatch, get_service_typesupport_maybe_fail_test) @@ -144,12 +135,13 @@ TEST(TestServiceTypeSupportDispatch, get_service_typesupport_maybe_fail_test) RCUTILS_FAULT_INJECTION_TEST( { // load library and find symbols - auto * result = rosidl_typesupport_cpp::get_service_typesupport_handle_function( - &type_support_cpp_identifier, - "test_type_support1"); - if (nullptr == result) { - EXPECT_TRUE(rcutils_error_is_set()); - rcutils_reset_error(); + try { + auto * result = rosidl_typesupport_cpp::get_service_typesupport_handle_function( + &type_support_cpp_identifier, + "test_type_support1"); + EXPECT_NE(result, nullptr); + } catch (const std::runtime_error &) { + } catch (...) { } }); }