fix a case that not throw ParameterUninitializedException (ros2#2036)

* fix a case that not throw ParameterUninitializedException Signed-off-by: Chen Lihui <lihui.chen@sony.com> * catch ParameterUninitializedException exception while calling get_parameters in service callback Signed-off-by: Chen Lihui <lihui.chen@sony.com> * update doc Signed-off-by: Chen Lihui <lihui.chen@sony.com> * add one more test Signed-off-by: Chen Lihui <lihui.chen@sony.com> * explicitly use this to call a method inside the class itself Co-authored-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com> Signed-off-by: Chen Lihui <lihui.chen@sony.com> Signed-off-by: Chen Lihui <lihui.chen@sony.com> Co-authored-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
irobot-ros · Apr 29, 2023 · d011f6b · d011f6b
1 parent 41ccb78
commit d011f6b
Show file tree

Hide file tree

Showing 5 changed files with 77 additions and 27 deletions.
diff --git a/rclcpp/include/rclcpp/node.hpp b/rclcpp/include/rclcpp/node.hpp
@@ -634,17 +634,21 @@ class Node : public std::enable_shared_from_this<Node>
   /**
    * If the parameter has not been declared, then this method may throw the
    * rclcpp::exceptions::ParameterNotDeclaredException exception.
+   * If the parameter has not been initialized, then this method may throw the
+   * rclcpp::exceptions::ParameterUninitializedException exception.
    *
    * If undeclared parameters are allowed, see the node option
    * rclcpp::NodeOptions::allow_undeclared_parameters, then this method will
-   * not throw an exception, and instead return a default initialized
-   * rclcpp::Parameter, which has a type of
+   * not throw the rclcpp::exceptions::ParameterNotDeclaredException exception,
+   * and instead return a default initialized rclcpp::Parameter, which has a type of
    * rclcpp::ParameterType::PARAMETER_NOT_SET.
    *
    * \param[in] name The name of the parameter to get.
    * \return The requested parameter inside of a rclcpp parameter object.
    * \throws rclcpp::exceptions::ParameterNotDeclaredException if the parameter
    *   has not been declared and undeclared parameters are not allowed.
+   * \throws rclcpp::exceptions::ParameterUninitializedException if the parameter
+   *   has not been initialized.
    */
   RCLCPP_PUBLIC
   rclcpp::Parameter
@@ -728,12 +732,12 @@ class Node : public std::enable_shared_from_this<Node>
 
   /// Return the parameters by the given parameter names.
   /**
-   * Like get_parameters(), this method may throw the
+   * Like get_parameter(const std::string &), this method may throw the
    * rclcpp::exceptions::ParameterNotDeclaredException exception if the
    * requested parameter has not been declared and undeclared parameters are
-   * not allowed.
+   * not allowed, and may throw the rclcpp::exceptions::ParameterUninitializedException exception.
    *
-   * Also like get_parameters(), if undeclared parameters are allowed and the
+   * Also like get_parameter(const std::string &), if undeclared parameters are allowed and the
    * parameter has not been declared, then the corresponding rclcpp::Parameter
    * will be default initialized and therefore have the type
    * rclcpp::ParameterType::PARAMETER_NOT_SET.
@@ -743,6 +747,8 @@ class Node : public std::enable_shared_from_this<Node>
    * \throws rclcpp::exceptions::ParameterNotDeclaredException if any of the
    *   parameters have not been declared and undeclared parameters are not
    *   allowed.
+   * \throws rclcpp::exceptions::ParameterUninitializedException if any of the
+   *   parameters have not been initialized.
    */
   RCLCPP_PUBLIC
   std::vector<rclcpp::Parameter>

diff --git a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp
@@ -784,22 +784,12 @@ NodeParameters::set_parameters_atomically(const std::vector<rclcpp::Parameter> &
 std::vector<rclcpp::Parameter>
 NodeParameters::get_parameters(const std::vector<std::string> & names) const
 {
-  std::lock_guard<std::recursive_mutex> lock(mutex_);
   std::vector<rclcpp::Parameter> results;
   results.reserve(names.size());
 
+  std::lock_guard<std::recursive_mutex> lock(mutex_);
   for (auto & name : names) {
-    auto found_parameter = parameters_.find(name);
-    if (found_parameter != parameters_.cend()) {
-      // found
-      results.emplace_back(name, found_parameter->second.value);
-    } else if (this->allow_undeclared_) {
-      // not found, but undeclared allowed
-      results.emplace_back(name, rclcpp::ParameterValue());
-    } else {
-      // not found, and undeclared are not allowed
-      throw rclcpp::exceptions::ParameterNotDeclaredException(name);
-    }
+    results.emplace_back(this->get_parameter(name));
   }
   return results;
 }
@@ -810,18 +800,18 @@ NodeParameters::get_parameter(const std::string & name) const
   std::lock_guard<std::recursive_mutex> lock(mutex_);
 
   auto param_iter = parameters_.find(name);
-  if (
-    parameters_.end() != param_iter &&
-    (param_iter->second.value.get_type() != rclcpp::ParameterType::PARAMETER_NOT_SET ||
-    param_iter->second.descriptor.dynamic_typing))
-  {
-    return rclcpp::Parameter{name, param_iter->second.value};
+  if (parameters_.end() != param_iter) {
+    if (
+      param_iter->second.value.get_type() != rclcpp::ParameterType::PARAMETER_NOT_SET ||
+      param_iter->second.descriptor.dynamic_typing)
+    {
+      return rclcpp::Parameter{name, param_iter->second.value};
+    }
+    throw rclcpp::exceptions::ParameterUninitializedException(name);
   } else if (this->allow_undeclared_) {
-    return rclcpp::Parameter{};
-  } else if (parameters_.end() == param_iter) {
-    throw rclcpp::exceptions::ParameterNotDeclaredException(name);
+    return rclcpp::Parameter{name};
   } else {
-    throw rclcpp::exceptions::ParameterUninitializedException(name);
+    throw rclcpp::exceptions::ParameterNotDeclaredException(name);
   }
 }
 

diff --git a/rclcpp/src/rclcpp/parameter_service.cpp b/rclcpp/src/rclcpp/parameter_service.cpp
@@ -48,6 +48,8 @@ ParameterService::ParameterService(
         }
       } catch (const rclcpp::exceptions::ParameterNotDeclaredException & ex) {
         RCLCPP_DEBUG(rclcpp::get_logger("rclcpp"), "Failed to get parameters: %s", ex.what());
+      } catch (const rclcpp::exceptions::ParameterUninitializedException & ex) {
+        RCLCPP_DEBUG(rclcpp::get_logger("rclcpp"), "Failed to get parameters: %s", ex.what());
       }
     },
     qos_profile, nullptr);

diff --git a/rclcpp/test/rclcpp/test_node.cpp b/rclcpp/test/rclcpp/test_node.cpp
@@ -3186,6 +3186,9 @@ TEST_F(TestNode, static_and_dynamic_typing) {
     EXPECT_THROW(
       node->get_parameter("integer_override_not_given"),
       rclcpp::exceptions::ParameterUninitializedException);
+    EXPECT_THROW(
+      node->get_parameters({"integer_override_not_given"}),
+      rclcpp::exceptions::ParameterUninitializedException);
   }
   {
     auto param = node->declare_parameter("integer_set_after_declare", rclcpp::PARAMETER_INTEGER);
@@ -3207,3 +3210,34 @@ TEST_F(TestNode, static_and_dynamic_typing) {
       rclcpp::exceptions::InvalidParameterTypeException);
   }
 }
+
+TEST_F(TestNode, parameter_uninitialized_exception_even_if_allow_undeclared) {
+  rclcpp::NodeOptions no;
+  no.allow_undeclared_parameters(true);
+  auto node = std::make_shared<rclcpp::Node>("node", "ns", no);
+  {
+    const std::string param_name = "integer_override_not_given";
+    auto param_value = node->declare_parameter(param_name, rclcpp::PARAMETER_INTEGER);
+    EXPECT_EQ(rclcpp::PARAMETER_NOT_SET, param_value.get_type());
+    // Throws if not set before access
+    EXPECT_THROW(
+      node->get_parameter(param_name),
+      rclcpp::exceptions::ParameterUninitializedException);
+    EXPECT_THROW(
+      node->get_parameters({param_name}),
+      rclcpp::exceptions::ParameterUninitializedException);
+  }
+}
+
+TEST_F(TestNode, get_parameter_with_node_allow_undeclared) {
+  rclcpp::NodeOptions no;
+  no.allow_undeclared_parameters(true);
+  auto node = std::make_shared<rclcpp::Node>("node", "ns", no);
+  {
+    const std::string param_name = "allow_undeclared_param";
+    auto param = node->get_parameter(param_name);
+    EXPECT_EQ(param_name, param.get_name());
+    EXPECT_EQ(rclcpp::PARAMETER_NOT_SET, param.get_type());
+    EXPECT_EQ(rclcpp::ParameterValue{}, param.get_parameter_value());
+  }
+}
diff --git a/rclcpp/test/rclcpp/test_parameter_client.cpp b/rclcpp/test/rclcpp/test_parameter_client.cpp
@@ -440,6 +440,7 @@ TEST_F(TestParameterClient, sync_parameter_get_parameter_types_allow_undeclared)
 TEST_F(TestParameterClient, sync_parameter_get_parameters) {
   node->declare_parameter("foo", 4);
   node->declare_parameter("bar", "this is bar");
+  node->declare_parameter("baz", rclcpp::PARAMETER_INTEGER);
   auto synchronous_client = std::make_shared<rclcpp::SyncParametersClient>(node);
 
   {
@@ -448,6 +449,14 @@ TEST_F(TestParameterClient, sync_parameter_get_parameters) {
     ASSERT_EQ(0u, parameters.size());
   }
 
+  {
+    // not throw ParameterUninitializedException while getting parameter from service
+    // even if the parameter is not initialized in the node
+    std::vector<std::string> names{"baz"};
+    std::vector<rclcpp::Parameter> parameters = synchronous_client->get_parameters(names, 10s);
+    ASSERT_EQ(0u, parameters.size());
+  }
+
   {
     std::vector<std::string> names{"none", "foo", "bar"};
     std::vector<rclcpp::Parameter> parameters = synchronous_client->get_parameters(names, 10s);
@@ -487,6 +496,7 @@ TEST_F(TestParameterClient, sync_parameter_get_parameters) {
 TEST_F(TestParameterClient, sync_parameter_get_parameters_allow_undeclared) {
   node_with_option->declare_parameter("foo", 4);
   node_with_option->declare_parameter("bar", "this is bar");
+  node_with_option->declare_parameter("baz", rclcpp::PARAMETER_INTEGER);
   auto synchronous_client = std::make_shared<rclcpp::SyncParametersClient>(node_with_option);
 
   {
@@ -495,6 +505,14 @@ TEST_F(TestParameterClient, sync_parameter_get_parameters_allow_undeclared) {
     ASSERT_EQ(1u, parameters.size());
   }
 
+  {
+    // not throw ParameterUninitializedException while getting parameter from service
+    // even if the parameter is not initialized in the node
+    std::vector<std::string> names{"baz"};
+    std::vector<rclcpp::Parameter> parameters = synchronous_client->get_parameters(names, 10s);
+    ASSERT_EQ(0u, parameters.size());
+  }
+
   {
     std::vector<std::string> names{"none", "foo", "bar"};
     std::vector<rclcpp::Parameter> parameters = synchronous_client->get_parameters(names, 10s);