Make get_rcl_allocator a function family (try 2) #2046
Conversation
As explained in ros2#1254, there's conceptually no way to implement RCL allocators in terms of C++ allocators. In order to fix this behavior, we have to delete the generic version of get_rcl_allocator. Since some ROS code depends on this, we need to allow users to write their own version of get_rcl_allocator for allocators that support the C-style interface (most do). So this CL changes get_rcl_allocator from a template function into a family of (potentially templated) functions, which allows users to add their own overloads and rely on the "most specialized" mechanism for function specialization to select the right one. See http://www.gotw.ca/publications/mill17.htm for details. This also allows us to return get_rcl_default_allocator for all specializations of std::allocator (previously, only for std::allocator), which will already fix ros2#1254 for pretty much all clients. I'll continue to work on deleting the generic version, though, to make sure that nobody is accidentally bitten by it. I've tried to test this by doing a full ROS compilation following the Dockerfile of the source Docker image, and all packages compile. Signed-off-by: Steve Wolter <[email protected]> Addressed code style test failures. Signed-off-by: Steve Wolter <[email protected]> Update comments. Incidentally, this also reruns the flaky test suite, which seems to be using the real DDS middleware and to be prone to cross-talk. Signed-off-by: Steve Wolter <[email protected]> Also update recently added tests. Signed-off-by: Steve Wolter <[email protected]> Added reference to bug number. Signed-off-by: Steve Wolter <[email protected]> Extend allocator lifetime in options. Signed-off-by: Steve Wolter <[email protected]> Drop the generic version of get_rcl_allocator. This allows us to simplify a bunch of code as well. Signed-off-by: Steve Wolter <[email protected]> Update rclcpp/include/rclcpp/allocator/allocator_common.hpp Co-authored-by: William Woodall <[email protected]> Signed-off-by: Steve Wolter <[email protected]> Revert accidental deletion of allocator creation. Signed-off-by: Steve Wolter <[email protected]>
|
@clalancette could you review this and let me know if this was what you were expecting in a rebase of this? Do you understand why we have custom allocators? This seems like really error-prone code to be writing without sanitizers in CI. |
| (void)allocator; // Remove warning | ||
| return rcl_get_default_allocator(); |
There was a problem hiding this comment.
Does this seem useless, and should users call the underlying rcl function? Maybe this is needed for backward compatibility or something?
tylerjw
changed the title
|
Thanks for reviving this pr, but I was recently investigating the efficacy of the original pr and I think it actually has a bug in it too (doesn't correctly solve the issue). I've been working on a branch that may address this correctly in some work related to space-ros, but I haven't gotten to testing it properly. I hope to open that pr after the Thanksgiving break. |
|
@wjwwood have you had a chance to post your work somewhere? |
|
Any news on this? @wjwwood |
|
I know that pings on PRs can be quite annoying, but I'd also love to see this fixed! @wjwwood could you maybe explain what the issue with the original code would be? It seems to be fairly straightforward in using the default rcl allocator. Is there a problem with that? |
|
We currently have to maintain a branch with these fixed to allow using asan with rclcpp. These fixes work for us. @wjwwood, sorry again for a ping on this, but what are the issues you are seeing? Is there something we could attempt to fix to allow a merge here? |
6 participants
This is a rebase of
I don't understand this code but would really like to be able to use asan in my tests.
If someone could explain this it might be helpful. My best guess from reading the other PR is that C++ allocators just don't work for some reason (or this implementation of them does not) and instead we are using some struct from rcl anyway.