From 23fccd354fb0631bd8953a74e50b557c54287dd3 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Mon, 6 Jan 2020 09:29:22 -0300 Subject: [PATCH 1/4] Avoid reference cycle between Node and ParameterService Signed-off-by: Ivan Santiago Paunovic --- rclpy/rclpy/parameter_service.py | 200 ++++++++++++++++--------------- 1 file changed, 105 insertions(+), 95 deletions(-) diff --git a/rclpy/rclpy/parameter_service.py b/rclpy/rclpy/parameter_service.py index 188105b7d..f549dd077 100644 --- a/rclpy/rclpy/parameter_service.py +++ b/rclpy/rclpy/parameter_service.py @@ -25,125 +25,135 @@ class ParameterService: def __init__(self, node): - self._node = node + def _describe_parameters_callback(self, request, response): + for name in request.names: + try: + descriptor = node.describe_parameter(name) + except ParameterNotDeclaredException: + descriptor = ParameterDescriptor() + response.descriptors.append(descriptor) + return response + + def _get_parameters_callback(self, request, response): + for name in request.names: + p = node.get_parameter_or(name) + response.values.append(p.get_parameter_value()) + return response + + def _get_parameter_types_callback(self, request, response): + for name in request.names: + response.types.append(node.get_parameter_or(name).type_) + return response + + def _list_parameters_callback(self, request, response): + names_with_prefixes = [] + for name in node._parameters.keys(): + if PARAMETER_SEPARATOR_STRING in name: + names_with_prefixes.append(name) + continue + elif request.prefixes: + for prefix in request.prefixes: + if name.startswith(prefix): + response.result.names.append(name) + continue + else: + response.result.names.append(name) + if 1 == request.depth: + return response + + if not request.DEPTH_RECURSIVE == request.depth: + names_with_prefixes = filter( + lambda name: + name.count(PARAMETER_SEPARATOR_STRING) < request.depth, names_with_prefixes + ) + for name in names_with_prefixes: + if request.prefixes: + for prefix in request.prefixes: + if name.startswith(prefix + PARAMETER_SEPARATOR_STRING): + response.result.names.append(name) + full_prefix = PARAMETER_SEPARATOR_STRING.join( + name.split(PARAMETER_SEPARATOR_STRING)[0:-1]) + if full_prefix not in response.result.prefixes: + response.result.prefixes.append(full_prefix) + if prefix not in response.result.prefixes: + response.result.prefixes.append(prefix) + else: + prefix = PARAMETER_SEPARATOR_STRING.join( + name.split(PARAMETER_SEPARATOR_STRING)[0:-1]) + if prefix not in response.result.prefixes: + response.result.prefixes.append(prefix) + response.result.names.append(name) + + return response + + def _set_parameters_callback(self, request, response): + for p in request.parameters: + param = Parameter.from_parameter_msg(p) + try: + result = node.set_parameters_atomically([param]) + except ParameterNotDeclaredException as e: + result = SetParametersResult( + successful=False, + reason=str(e) + ) + response.results.append(result) + return response + + def _set_parameters_atomically_callback(self, request, response): + try: + response.result = node.set_parameters_atomically([ + Parameter.from_parameter_msg(p) for p in request.parameters]) + except ParameterNotDeclaredException as e: + response.result = SetParametersResult( + successful=False, + reason=str(e) + ) + return response + nodename = node.get_name() describe_parameters_service_name = \ TOPIC_SEPARATOR_STRING.join((nodename, 'describe_parameters')) node.create_service( - DescribeParameters, describe_parameters_service_name, - self._describe_parameters_callback, qos_profile=qos_profile_parameters + DescribeParameters, + describe_parameters_service_name, + _describe_parameters_callback, + qos_profile=qos_profile_parameters ) get_parameters_service_name = TOPIC_SEPARATOR_STRING.join((nodename, 'get_parameters')) node.create_service( - GetParameters, get_parameters_service_name, self._get_parameters_callback, + GetParameters, + get_parameters_service_name, + _get_parameters_callback, qos_profile=qos_profile_parameters ) get_parameter_types_service_name = \ TOPIC_SEPARATOR_STRING.join((nodename, 'get_parameter_types')) node.create_service( - GetParameterTypes, get_parameter_types_service_name, - self._get_parameter_types_callback, qos_profile=qos_profile_parameters + GetParameterTypes, + get_parameter_types_service_name, + _get_parameter_types_callback, + qos_profile=qos_profile_parameters ) list_parameters_service_name = TOPIC_SEPARATOR_STRING.join((nodename, 'list_parameters')) node.create_service( - ListParameters, list_parameters_service_name, self._list_parameters_callback, + ListParameters, + list_parameters_service_name, + _list_parameters_callback, qos_profile=qos_profile_parameters ) set_parameters_service_name = TOPIC_SEPARATOR_STRING.join((nodename, 'set_parameters')) node.create_service( - SetParameters, set_parameters_service_name, self._set_parameters_callback, + SetParameters, + set_parameters_service_name, + _set_parameters_callback, qos_profile=qos_profile_parameters ) set_parameters_atomically_service_name = \ TOPIC_SEPARATOR_STRING.join((nodename, 'set_parameters_atomically')) node.create_service( - SetParametersAtomically, set_parameters_atomically_service_name, - self._set_parameters_atomically_callback, + SetParametersAtomically, + set_parameters_atomically_service_name, + _set_parameters_atomically_callback, qos_profile=qos_profile_parameters ) - - def _describe_parameters_callback(self, request, response): - for name in request.names: - try: - descriptor = self._node.describe_parameter(name) - except ParameterNotDeclaredException: - descriptor = ParameterDescriptor() - response.descriptors.append(descriptor) - return response - - def _get_parameters_callback(self, request, response): - for name in request.names: - p = self._node.get_parameter_or(name) - response.values.append(p.get_parameter_value()) - return response - - def _get_parameter_types_callback(self, request, response): - for name in request.names: - response.types.append(self._node.get_parameter_or(name).type_) - return response - - def _list_parameters_callback(self, request, response): - names_with_prefixes = [] - for name in self._node._parameters.keys(): - if PARAMETER_SEPARATOR_STRING in name: - names_with_prefixes.append(name) - continue - elif request.prefixes: - for prefix in request.prefixes: - if name.startswith(prefix): - response.result.names.append(name) - continue - else: - response.result.names.append(name) - if 1 == request.depth: - return response - - if not request.DEPTH_RECURSIVE == request.depth: - names_with_prefixes = filter( - lambda name: - name.count(PARAMETER_SEPARATOR_STRING) < request.depth, names_with_prefixes - ) - for name in names_with_prefixes: - if request.prefixes: - for prefix in request.prefixes: - if name.startswith(prefix + PARAMETER_SEPARATOR_STRING): - response.result.names.append(name) - full_prefix = PARAMETER_SEPARATOR_STRING.join( - name.split(PARAMETER_SEPARATOR_STRING)[0:-1]) - if full_prefix not in response.result.prefixes: - response.result.prefixes.append(full_prefix) - if prefix not in response.result.prefixes: - response.result.prefixes.append(prefix) - else: - prefix = PARAMETER_SEPARATOR_STRING.join( - name.split(PARAMETER_SEPARATOR_STRING)[0:-1]) - if prefix not in response.result.prefixes: - response.result.prefixes.append(prefix) - response.result.names.append(name) - - return response - - def _set_parameters_callback(self, request, response): - for p in request.parameters: - param = Parameter.from_parameter_msg(p) - try: - result = self._node.set_parameters_atomically([param]) - except ParameterNotDeclaredException as e: - result = SetParametersResult( - successful=False, - reason=str(e) - ) - response.results.append(result) - return response - - def _set_parameters_atomically_callback(self, request, response): - try: - response.result = self._node.set_parameters_atomically([ - Parameter.from_parameter_msg(p) for p in request.parameters]) - except ParameterNotDeclaredException as e: - response.result = SetParametersResult( - successful=False, - reason=str(e) - ) - return response From 69d3a62e1e52fe84245bc710d4c63a4607286fcd Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Mon, 6 Jan 2020 12:20:12 -0300 Subject: [PATCH 2/4] Address peer review comments Signed-off-by: Ivan Santiago Paunovic --- rclpy/rclpy/parameter_service.py | 197 ++++++++++++++++--------------- 1 file changed, 101 insertions(+), 96 deletions(-) diff --git a/rclpy/rclpy/parameter_service.py b/rclpy/rclpy/parameter_service.py index f549dd077..74d13a34a 100644 --- a/rclpy/rclpy/parameter_service.py +++ b/rclpy/rclpy/parameter_service.py @@ -12,6 +12,8 @@ # See the License for the specific language governing permissions and # limitations under the License. +import weakref + from rcl_interfaces.msg import ParameterDescriptor from rcl_interfaces.msg import SetParametersResult from rcl_interfaces.srv import DescribeParameters, GetParameters, GetParameterTypes @@ -25,68 +27,117 @@ class ParameterService: def __init__(self, node): - def _describe_parameters_callback(self, request, response): + self._node_weak_ref = weakref.ref(node) + nodename = node.get_name() + + describe_parameters_service_name = \ + TOPIC_SEPARATOR_STRING.join((nodename, 'describe_parameters')) + node.create_service( + DescribeParameters, describe_parameters_service_name, + self._describe_parameters_callback, qos_profile=qos_profile_parameters + ) + get_parameters_service_name = TOPIC_SEPARATOR_STRING.join((nodename, 'get_parameters')) + node.create_service( + GetParameters, get_parameters_service_name, self._get_parameters_callback, + qos_profile=qos_profile_parameters + ) + get_parameter_types_service_name = \ + TOPIC_SEPARATOR_STRING.join((nodename, 'get_parameter_types')) + node.create_service( + GetParameterTypes, get_parameter_types_service_name, + self._get_parameter_types_callback, qos_profile=qos_profile_parameters + ) + list_parameters_service_name = TOPIC_SEPARATOR_STRING.join((nodename, 'list_parameters')) + node.create_service( + ListParameters, list_parameters_service_name, self._list_parameters_callback, + qos_profile=qos_profile_parameters + ) + set_parameters_service_name = TOPIC_SEPARATOR_STRING.join((nodename, 'set_parameters')) + node.create_service( + SetParameters, set_parameters_service_name, self._set_parameters_callback, + qos_profile=qos_profile_parameters + ) + set_parameters_atomically_service_name = \ + TOPIC_SEPARATOR_STRING.join((nodename, 'set_parameters_atomically')) + node.create_service( + SetParametersAtomically, set_parameters_atomically_service_name, + self._set_parameters_atomically_callback, + qos_profile=qos_profile_parameters + ) + + def _describe_parameters_callback(self, request, response): + node = self._node_weak_ref() + if node is not None: for name in request.names: try: descriptor = node.describe_parameter(name) except ParameterNotDeclaredException: descriptor = ParameterDescriptor() response.descriptors.append(descriptor) - return response + return response - def _get_parameters_callback(self, request, response): + def _get_parameters_callback(self, request, response): + node = self._node_weak_ref() + if node is not None: for name in request.names: p = node.get_parameter_or(name) response.values.append(p.get_parameter_value()) - return response + return response - def _get_parameter_types_callback(self, request, response): + def _get_parameter_types_callback(self, request, response): + node = self._node_weak_ref() + if node is not None: for name in request.names: response.types.append(node.get_parameter_or(name).type_) + return response + + def _list_parameters_callback(self, request, response): + names_with_prefixes = [] + node = self._node_weak_ref() + if node is None: + return response + for name in node._parameters.keys(): + if PARAMETER_SEPARATOR_STRING in name: + names_with_prefixes.append(name) + continue + elif request.prefixes: + for prefix in request.prefixes: + if name.startswith(prefix): + response.result.names.append(name) + continue + else: + response.result.names.append(name) + if 1 == request.depth: return response - def _list_parameters_callback(self, request, response): - names_with_prefixes = [] - for name in node._parameters.keys(): - if PARAMETER_SEPARATOR_STRING in name: - names_with_prefixes.append(name) - continue - elif request.prefixes: - for prefix in request.prefixes: - if name.startswith(prefix): - response.result.names.append(name) - continue - else: - response.result.names.append(name) - if 1 == request.depth: - return response + if not request.DEPTH_RECURSIVE == request.depth: + names_with_prefixes = filter( + lambda name: + name.count(PARAMETER_SEPARATOR_STRING) < request.depth, names_with_prefixes + ) + for name in names_with_prefixes: + if request.prefixes: + for prefix in request.prefixes: + if name.startswith(prefix + PARAMETER_SEPARATOR_STRING): + response.result.names.append(name) + full_prefix = PARAMETER_SEPARATOR_STRING.join( + name.split(PARAMETER_SEPARATOR_STRING)[0:-1]) + if full_prefix not in response.result.prefixes: + response.result.prefixes.append(full_prefix) + if prefix not in response.result.prefixes: + response.result.prefixes.append(prefix) + else: + prefix = PARAMETER_SEPARATOR_STRING.join( + name.split(PARAMETER_SEPARATOR_STRING)[0:-1]) + if prefix not in response.result.prefixes: + response.result.prefixes.append(prefix) + response.result.names.append(name) - if not request.DEPTH_RECURSIVE == request.depth: - names_with_prefixes = filter( - lambda name: - name.count(PARAMETER_SEPARATOR_STRING) < request.depth, names_with_prefixes - ) - for name in names_with_prefixes: - if request.prefixes: - for prefix in request.prefixes: - if name.startswith(prefix + PARAMETER_SEPARATOR_STRING): - response.result.names.append(name) - full_prefix = PARAMETER_SEPARATOR_STRING.join( - name.split(PARAMETER_SEPARATOR_STRING)[0:-1]) - if full_prefix not in response.result.prefixes: - response.result.prefixes.append(full_prefix) - if prefix not in response.result.prefixes: - response.result.prefixes.append(prefix) - else: - prefix = PARAMETER_SEPARATOR_STRING.join( - name.split(PARAMETER_SEPARATOR_STRING)[0:-1]) - if prefix not in response.result.prefixes: - response.result.prefixes.append(prefix) - response.result.names.append(name) + return response - return response - - def _set_parameters_callback(self, request, response): + def _set_parameters_callback(self, request, response): + node = self._node_weak_ref() + if node is not None: for p in request.parameters: param = Parameter.from_parameter_msg(p) try: @@ -97,9 +148,11 @@ def _set_parameters_callback(self, request, response): reason=str(e) ) response.results.append(result) - return response + return response - def _set_parameters_atomically_callback(self, request, response): + def _set_parameters_atomically_callback(self, request, response): + node = self._node_weak_ref() + if node is not None: try: response.result = node.set_parameters_atomically([ Parameter.from_parameter_msg(p) for p in request.parameters]) @@ -108,52 +161,4 @@ def _set_parameters_atomically_callback(self, request, response): successful=False, reason=str(e) ) - return response - - nodename = node.get_name() - - describe_parameters_service_name = \ - TOPIC_SEPARATOR_STRING.join((nodename, 'describe_parameters')) - node.create_service( - DescribeParameters, - describe_parameters_service_name, - _describe_parameters_callback, - qos_profile=qos_profile_parameters - ) - get_parameters_service_name = TOPIC_SEPARATOR_STRING.join((nodename, 'get_parameters')) - node.create_service( - GetParameters, - get_parameters_service_name, - _get_parameters_callback, - qos_profile=qos_profile_parameters - ) - get_parameter_types_service_name = \ - TOPIC_SEPARATOR_STRING.join((nodename, 'get_parameter_types')) - node.create_service( - GetParameterTypes, - get_parameter_types_service_name, - _get_parameter_types_callback, - qos_profile=qos_profile_parameters - ) - list_parameters_service_name = TOPIC_SEPARATOR_STRING.join((nodename, 'list_parameters')) - node.create_service( - ListParameters, - list_parameters_service_name, - _list_parameters_callback, - qos_profile=qos_profile_parameters - ) - set_parameters_service_name = TOPIC_SEPARATOR_STRING.join((nodename, 'set_parameters')) - node.create_service( - SetParameters, - set_parameters_service_name, - _set_parameters_callback, - qos_profile=qos_profile_parameters - ) - set_parameters_atomically_service_name = \ - TOPIC_SEPARATOR_STRING.join((nodename, 'set_parameters_atomically')) - node.create_service( - SetParametersAtomically, - set_parameters_atomically_service_name, - _set_parameters_atomically_callback, - qos_profile=qos_profile_parameters - ) + return response From d41fc53d10d39eafdc943e839ddec2d1a9d883ac Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Mon, 6 Jan 2020 15:06:36 -0300 Subject: [PATCH 3/4] Raise error if weak reference is invalid Signed-off-by: Ivan Santiago Paunovic --- rclpy/rclpy/parameter_service.py | 83 ++++++++++++++++---------------- 1 file changed, 41 insertions(+), 42 deletions(-) diff --git a/rclpy/rclpy/parameter_service.py b/rclpy/rclpy/parameter_service.py index 74d13a34a..69743b920 100644 --- a/rclpy/rclpy/parameter_service.py +++ b/rclpy/rclpy/parameter_service.py @@ -66,36 +66,31 @@ def __init__(self, node): ) def _describe_parameters_callback(self, request, response): - node = self._node_weak_ref() - if node is not None: - for name in request.names: - try: - descriptor = node.describe_parameter(name) - except ParameterNotDeclaredException: - descriptor = ParameterDescriptor() - response.descriptors.append(descriptor) + node = self._get_node() + for name in request.names: + try: + descriptor = node.describe_parameter(name) + except ParameterNotDeclaredException: + descriptor = ParameterDescriptor() + response.descriptors.append(descriptor) return response def _get_parameters_callback(self, request, response): - node = self._node_weak_ref() - if node is not None: - for name in request.names: - p = node.get_parameter_or(name) - response.values.append(p.get_parameter_value()) + node = self._get_node() + for name in request.names: + p = node.get_parameter_or(name) + response.values.append(p.get_parameter_value()) return response def _get_parameter_types_callback(self, request, response): - node = self._node_weak_ref() - if node is not None: - for name in request.names: - response.types.append(node.get_parameter_or(name).type_) + node = self._get_node() + for name in request.names: + response.types.append(node.get_parameter_or(name).type_) return response def _list_parameters_callback(self, request, response): names_with_prefixes = [] - node = self._node_weak_ref() - if node is None: - return response + node = self._get_node() for name in node._parameters.keys(): if PARAMETER_SEPARATOR_STRING in name: names_with_prefixes.append(name) @@ -136,29 +131,33 @@ def _list_parameters_callback(self, request, response): return response def _set_parameters_callback(self, request, response): - node = self._node_weak_ref() - if node is not None: - for p in request.parameters: - param = Parameter.from_parameter_msg(p) - try: - result = node.set_parameters_atomically([param]) - except ParameterNotDeclaredException as e: - result = SetParametersResult( - successful=False, - reason=str(e) - ) - response.results.append(result) + node = self._get_node() + for p in request.parameters: + param = Parameter.from_parameter_msg(p) + try: + result = node.set_parameters_atomically([param]) + except ParameterNotDeclaredException as e: + result = SetParametersResult( + successful=False, + reason=str(e) + ) + response.results.append(result) return response def _set_parameters_atomically_callback(self, request, response): - node = self._node_weak_ref() - if node is not None: - try: - response.result = node.set_parameters_atomically([ - Parameter.from_parameter_msg(p) for p in request.parameters]) - except ParameterNotDeclaredException as e: - response.result = SetParametersResult( - successful=False, - reason=str(e) - ) + node = self._get_node() + try: + response.result = node.set_parameters_atomically([ + Parameter.from_parameter_msg(p) for p in request.parameters]) + except ParameterNotDeclaredException as e: + response.result = SetParametersResult( + successful=False, + reason=str(e) + ) return response + + def _get_node(self): + node = self._node_weak_ref() + if node is None: + raise RuntimeError('Expected valid node weak reference') + return node From ab994d1531500a518192dc2fe40cc31d3c32f8a6 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Mon, 6 Jan 2020 18:19:05 -0300 Subject: [PATCH 4/4] Use ReferenceError instead of RuntimeError Signed-off-by: Ivan Santiago Paunovic --- rclpy/rclpy/parameter_service.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rclpy/rclpy/parameter_service.py b/rclpy/rclpy/parameter_service.py index 69743b920..936779cfb 100644 --- a/rclpy/rclpy/parameter_service.py +++ b/rclpy/rclpy/parameter_service.py @@ -159,5 +159,5 @@ def _set_parameters_atomically_callback(self, request, response): def _get_node(self): node = self._node_weak_ref() if node is None: - raise RuntimeError('Expected valid node weak reference') + raise ReferenceError('Expected valid node weak reference') return node