diff --git a/rclcpp/src/rclcpp/parameter_service.cpp b/rclcpp/src/rclcpp/parameter_service.cpp index 1cc08910d0..435f175e1d 100644 --- a/rclcpp/src/rclcpp/parameter_service.cpp +++ b/rclcpp/src/rclcpp/parameter_service.cpp @@ -15,6 +15,7 @@ #include "rclcpp/parameter_service.hpp" #include +#include #include #include #include @@ -90,7 +91,7 @@ ParameterService::ParameterService( try { result = node_params->set_parameters_atomically( {rclcpp::Parameter::from_parameter_msg(p)}); - } catch (const rclcpp::exceptions::ParameterNotDeclaredException & ex) { + } catch (const std::exception & ex) { RCLCPP_WARN(rclcpp::get_logger("rclcpp"), "Failed to set parameter: %s", ex.what()); result.successful = false; result.reason = ex.what(); @@ -108,14 +109,14 @@ ParameterService::ParameterService( const std::shared_ptr & request, std::shared_ptr response) { - std::vector pvariants; - std::transform( - request->parameters.cbegin(), request->parameters.cend(), - std::back_inserter(pvariants), - [](const rcl_interfaces::msg::Parameter & p) { - return rclcpp::Parameter::from_parameter_msg(p); - }); try { + std::vector pvariants; + std::transform( + request->parameters.cbegin(), request->parameters.cend(), + std::back_inserter(pvariants), + [](const rcl_interfaces::msg::Parameter & p) { + return rclcpp::Parameter::from_parameter_msg(p); + }); auto result = node_params->set_parameters_atomically(pvariants); response->result = result; } catch (const rclcpp::exceptions::ParameterNotDeclaredException & ex) { @@ -123,6 +124,11 @@ ParameterService::ParameterService( rclcpp::get_logger("rclcpp"), "Failed to set parameters atomically: %s", ex.what()); response->result.successful = false; response->result.reason = "One or more parameters were not declared before setting"; + } catch (const std::exception & ex) { + RCLCPP_WARN( + rclcpp::get_logger("rclcpp"), "Failed to set parameters atomically: %s", ex.what()); + response->result.successful = false; + response->result.reason = ex.what(); } }, qos_profile, nullptr); diff --git a/rclcpp/test/rclcpp/test_parameter_service.cpp b/rclcpp/test/rclcpp/test_parameter_service.cpp index 2aff8ecd0d..9abe22fcf9 100644 --- a/rclcpp/test/rclcpp/test_parameter_service.cpp +++ b/rclcpp/test/rclcpp/test_parameter_service.cpp @@ -20,6 +20,10 @@ #include #include +#include "rcl_interfaces/msg/parameter.hpp" +#include "rcl_interfaces/srv/set_parameters.hpp" +#include "rcl_interfaces/srv/set_parameters_atomically.hpp" + #include "../../src/rclcpp/parameter_service_names.hpp" #include "rclcpp/node.hpp" #include "rclcpp/parameter.hpp" @@ -98,6 +102,64 @@ TEST_F(TestParameterService, set_parameters_atomically) { EXPECT_EQ(0, client->get_parameter("parameter1", 100)); } +// Regression: an empty name must fail the request, not crash the node. +TEST_F(TestParameterService, set_parameters_empty_name_returns_failure) { + const std::vector parameters = { + rclcpp::Parameter("", 0), + }; + const auto results = client->set_parameters(parameters, 10s); + ASSERT_EQ(1u, results.size()); + EXPECT_FALSE(results[0].successful); +} + +TEST_F(TestParameterService, set_parameters_atomically_empty_name_returns_failure) { + const std::vector parameters = { + rclcpp::Parameter("", 0), + }; + const auto result = client->set_parameters_atomically(parameters, 10s); + EXPECT_FALSE(result.successful); +} + +// Unknown value type; the typed clients can't build one, so use a raw client. +TEST_F(TestParameterService, set_parameters_unknown_type_returns_failure) { + auto raw_client = node->create_client( + std::string(node->get_name()) + "/" + rclcpp::parameter_service_names::set_parameters); + ASSERT_TRUE(raw_client->wait_for_service(10s)); + + auto request = std::make_shared(); + rcl_interfaces::msg::Parameter parameter; + parameter.name = "parameter1"; + parameter.value.type = 42; // not a valid rclcpp::ParameterType + request->parameters.push_back(parameter); + + auto future = raw_client->async_send_request(request); + ASSERT_EQ( + rclcpp::spin_until_future_complete(node, future, 10s), + rclcpp::FutureReturnCode::SUCCESS); + const auto response = future.get(); + ASSERT_EQ(1u, response->results.size()); + EXPECT_FALSE(response->results[0].successful); +} + +TEST_F(TestParameterService, set_parameters_atomically_unknown_type_returns_failure) { + auto raw_client = node->create_client( + std::string(node->get_name()) + "/" + rclcpp::parameter_service_names::set_parameters_atomically); + ASSERT_TRUE(raw_client->wait_for_service(10s)); + + auto request = std::make_shared(); + rcl_interfaces::msg::Parameter parameter; + parameter.name = "parameter1"; + parameter.value.type = 42; // not a valid rclcpp::ParameterType + request->parameters.push_back(parameter); + + auto future = raw_client->async_send_request(request); + ASSERT_EQ( + rclcpp::spin_until_future_complete(node, future, 10s), + rclcpp::FutureReturnCode::SUCCESS); + const auto response = future.get(); + EXPECT_FALSE(response->result.successful); +} + TEST_F(TestParameterService, list_parameters) { const size_t number_parameters_in_basic_node = client->list_parameters({}, 1, 10s).names.size(); node->declare_parameter("parameter1", rclcpp::ParameterValue(42));