Convert all rcl_*_t types to shared pointers #450
Conversation
|
FYI: @wjwwood |
rclcpp/include/rclcpp/service.hpp
Outdated
| rcl_reset_error(); | ||
| } | ||
| } else { | ||
| RCLCPP_FATAL( |
There was a problem hiding this comment.
changed these to RCLCPP_FATAL (there is no RCLCPP_CRITICAL). Let me know if you would rather have it classified differently.
There was a problem hiding this comment.
I would expect FATAL to exit the process. I'd use ERROR instead.
There was a problem hiding this comment.
Sure thing.
rclcpp/include/rclcpp/service.hpp
Outdated
| rcl_reset_error(); | ||
| } | ||
| } else { | ||
| RCLCPP_FATAL( |
There was a problem hiding this comment.
I would expect FATAL to exit the process. I'd use ERROR instead.
rclcpp/src/rclcpp/client.cpp
Outdated
| if (handle) { | ||
| if (rcl_client_fini(client, handle.get()) != RCL_RET_OK) { | ||
| RCLCPP_ERROR( | ||
| rclcpp::get_logger(rcl_node_get_name(handle.get())).get_child("rclcpp"), |
There was a problem hiding this comment.
I may have missed this also on the previous review, but I think you should use rcl_node_get_logger_name here rather than rcl_node_get_name, which will not include the node's namespace.
rclcpp/include/rclcpp/client.hpp
Outdated
|
|
||
| std::shared_ptr<rcl_client_t> client_handle_; | ||
| std::shared_ptr<rcl_node_t> node_handle_; |
There was a problem hiding this comment.
Is this order change important? Otherwise I'd ask you to leave it as-is.
There was a problem hiding this comment.
We want the client handle to be destructed before the node handle. I agree that relying on implicit class destruction is naive. I'll make it explicit in the ClientBase desctructor.
rclcpp/include/rclcpp/service.hpp
Outdated
| @@ -118,16 +116,25 @@ class Service : public ServiceBase | |||
| using rosidl_typesupport_cpp::get_service_type_support_handle; | |||
| auto service_type_support_handle = get_service_type_support_handle<ServiceT>(); | |||
|
|
|||
| std::weak_ptr<rcl_node_t> weak_node_handle(node_handle_); | |||
| // rcl does the static memory allocation here | |||
| service_handle_ = std::shared_ptr<rcl_service_t>( | |||
| new rcl_service_t, [ = ](rcl_service_t * service) | |||
There was a problem hiding this comment.
I let this slide in the last review, but I actually think the implicit copy capture here is dangerous, because it also captures this, and therefore it's too each to accidentally use something from this inside the lambda in the future which will compile and work until the case where you delete the class but something else has a handle to the rcl_service_t (applies to each of the entity types).
I'd feel better about it with explicitly listing what you need to capture by copy or reference, rather than using one of the catch all ones. So in this case it should be just [weak_node_handle].
There was a problem hiding this comment.
No problem. Changing...
rclcpp/src/rclcpp/client.cpp
Outdated
| @@ -80,7 +88,8 @@ ClientBase::service_is_ready() const | |||
| { | |||
| bool is_ready; | |||
| rcl_ret_t ret = | |||
| rcl_service_server_is_available(this->get_rcl_node_handle(), client_handle_.get(), &is_ready); | |||
| rcl_service_server_is_available( | |||
| this->get_rcl_node_handle(), get_client_handle().get(), &is_ready); | |||
There was a problem hiding this comment.
Please do not wrap both the right hand side of an assignment as well as the arguments, but if you have to, make sure to indent the arguments again. Also please use this-> with methods of this class.
There was a problem hiding this comment.
I think I understand what you are asking but if not, let me know.
Converts all rcl_*_t types in the memory allocation strategy to shared pointers to prevent crash happening when a subscriber is reset. Issue: ros2#349
guillaumeautran
force-pushed
the
issue-349-2
branch
from
ec03711
to
800243b
Compare
Signed-off-by: Ralf Anton Beier <ralf_beier@me.com>
* added db directory creation to storage factory Signed-off-by: Marwan Taher <marokhaled99@gmail.com> * moved db directory creation to rosbag2_cpp Signed-off-by: Marwan Taher <marokhaled99@gmail.com> * rasing exception if dir already exists Signed-off-by: Marwan Taher <marokhaled99@gmail.com> * removed dir creation from record.py, added dir creation to sequential_compression_writer and refactored dir creation in sequential_writer Signed-off-by: Marwan Taher <marokhaled99@gmail.com> * fixed failing tests Signed-off-by: Marwan Taher <marokhaled99@gmail.com> * fixing review comments Signed-off-by: Marwan Taher <marokhaled99@gmail.com> * Apply suggestions from code review Co-authored-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Converts all rcl_*_t types in the memory allocation strategy to shared pointers to prevent crash happening when a subscriber is reset.
Issue: #349