Add any subscription callback types for shared_ptr<const T> #119
Conversation
| @@ -35,17 +35,23 @@ struct AnySubscriptionCallback | |||
| using SharedPtrCallback = std::function<void(const std::shared_ptr<MessageT> &)>; | |||
| using SharedPtrWithInfoCallback = | |||
| std::function<void(const std::shared_ptr<MessageT> &, const rmw_message_info_t &)>; | |||
| using ConstSharedPtrCallback = std::function<void(const std::shared_ptr<const MessageT> &)>; | |||
There was a problem hiding this comment.
The signature should not use a reference to a shared_ptr. The some for all the other signatures not touched in this PR.
There was a problem hiding this comment.
I will remove references to shared_ptrs but not to unique_ptrs as this seems to mess up how unique_ptrs are being copied for intra-process.
There was a problem hiding this comment.
But keeping the reference for unique pointers implies that the caller as well as the callee have a shared handle. If the caller lets the variable go out of scope while the callee is still holding the reference any access will access invalid memory.
There was a problem hiding this comment.
indeed. Well, I'll do some refactoring to see if I can make the correct callback signature work in rclcpp in this branch.
There was a problem hiding this comment.
ok, that wasn't as bad as I thought, I just changed a few places where the unique_ptr copy constructor was being used. I'll restart the CI jobs.
|
+1 with CI |
| >::type * = nullptr | ||
| > | ||
| void set(CallbackT callback) | ||
| { |
There was a problem hiding this comment.
Could you add a static_assert here to check for the second argument of the callback? It'd be better to add it as part of the std::enable_if, but... Windows.
There was a problem hiding this comment.
As you can see, I blindly copied this from the specializations of set for shared_ptr, so I don't really know what's going on. Is having multiple specializations of set like this not going to compile on Windows because of the greedy template resolution in VS2015? Should I move the arity check from the std::enable if and into the body of the function with static_assert?
There was a problem hiding this comment.
It'll work on Windows as is. The only thing is that the type of the second argument of the callback is not being checked. Unfortunately, we can't use std::is_same... for the second argument because of the lack of expression SFINAE in VS2015, so instead we can use static_assert in the body of the function to check the type.
There was a problem hiding this comment.
This is what we current do for service callbacks:
https://github.com/ros2/rclcpp/blob/master/rclcpp/include/rclcpp/node.hpp#L322
I'm thinking that maybe it'd make sense to move the is_same check from the enable_if to a static_assert in our code, since we only care about the arity of the callbacks. It'd also make our code more uniform.
There was a problem hiding this comment.
Still confused about this. I think we do care about the type as well as the arity of the callback here. If Windows were only looking at the arity and not the type, then I would expect any code that exercises the new callback type to fail, because the subscriber would end up trying to call the shared_ptr callback instead of the const_shared_ptr callback...
In fact, I'm added a test for this since the new callback type to make sure:
There was a problem hiding this comment.
That's true, I didn't see the other version of set, my bad. Still, it'd be good to have a static_assert for the second argument, since we can't add the check in enable_if because of Windows.
There was a problem hiding this comment.
The new test passes: http://ci.ros2.org/job/ros2_batch_ci_windows/460/testReport/%28root%29/test_subscription__rmw_connext_cpp/subscription_shared_ptr_const/
I'm still not convinced that we need a static assert, maybe we should chat about it offline?
There was a problem hiding this comment.
alright, static asserts have been added. I will rerun CI just to make sure.
jacquelinekay
added
in review
|
Restarting CI one more time, removing unique_ptr references from demo code (ros2/demos#27) http://ci.ros2.org/job/ros2_batch_ci_linux/413 |
|
Latest CI: William said +1 with CI, and the PR making the changes in |
jacquelinekay
force-pushed
the
const_shared_ptr
branch
from
874084a
to
971d638
Compare
|
@jacquelinekay I've solved/worked around the issues on Windows with callbacks that have variable number of arguments in #122 The changes in that PR may conflict with yours, let me know if you'd rather merge your PR first and I can fix the conflicts in mine. |
jacquelinekay
force-pushed
the
const_shared_ptr
branch
from
971d638
to
00daba2
Compare
Add any subscription callback types for shared_ptr<const T>
Galactic: Do not crash Executor when send_response fails due to client failure
I noticed this was missing in #118. This should enable us to set up a subscription callback with
ConstSharedPtr.