Add ability to pass in allocator for some methods

[methylDragon]

Feature request

Feature description

A couple of destruction methods have signatures that make it impossible to obtain an allocator from an active context, which means that any of the members those methods are meant to deallocate cannot use a user defined allocator to do so.

An example is the rmw_destroy_guard_condition

I suspect there are a couple more, such as rmw_destroy_wait_set, etc.

This could be done by either sticking the context or node pointer in the relevant structs, or passing it into the method like how rmw_destroy_publisher does it. Though this might be API breaking for all existing rmw implementations.

Implementation considerations

On the other hand, I'm not sure if the recommended way to allocate or deallocate dynamic memory is via rmw_allocate and rmw_free. It looks like those just boil down to a malloc and free/delete call.

I presume that using allocators (that is, rcutils_allocator_t) would be preferable?

[ivanpauno]

A couple of destruction methods have signatures that make it impossible to obtain an allocator from an active context, which means that any of the members those methods are meant to deallocate cannot use a user defined allocator to do so.

The destructor doesn't need an allocator, an allocator should be passed at construction time and stored by the implementation.

This could be done by either sticking the context or node pointer in the relevant structs, or passing it into the method like how rmw_destroy_publisher does it. Though this might be API breaking for all existing rmw implementations.

The general preference is to store the allocator internally, so it's obvious that the same allocator was used for construction and destruction. In the case of an object needing a pointer to a "parent" object, IMO it's preferred to stick it internally. The reason is the same one, if somebody pass the wrong node to rmw_destroy_publisher, that would be a problem (rmw_destroy_publisher is one of the few exceptions to this).

Between using the allocator in the context and being able to pass allocators individually, I'm not quite sure what's preferred. That would need some discussion.

Though the API have allocators passed in some places, the rmw implementations aren't using it consistently. They are randomly using one of:

• Functions here. Those are wrappers to malloc, etc.
• new/delete
• An allocator sticked in the object

My feeling is that allocation in rmw implementations needs a general cleanup, and that would require some discussion.

[gbiggs]

I agree with the need for a general cleanup.

Regarding rmw_destroy_publisher, what's the reason for the exception? Or does that need changing to not receive an allocator?

[ivanpauno]

Regarding rmw_destroy_publisher, what's the reason for the exception? Or does that need changing to not receive an allocator?

The publisher destructor is receiving a pointer to the parent node, not an allocator (the original post mentioned that, because the allocator stored in the node could be used). There is some rationale here https://github.com/ros2/rmw/pull/252#discussion_r459879748.

In the case of allocators, I think we (almost) always stick it internally. Doing that avoids errors.

In the case of rmw_destroy_publisher(rmw_node_t *, rmw_publisher_t *), either passing the node pointer or not can cause problems:

• passing it -> you can pass the wrong node pointer
• sticking it -> the publisher is "borrowing" the node pointer, but it doesn't control its lifetime. It's prone to use-after-free errors.

In this last case, I think that passing the pointer might be less error prone. The only problem is that we don't do that consistently (e.g rmw_destroy_node, rmw_destroy_guard_condition doesn't receive a context).

[jacobperron]

Between using the allocator in the context and being able to pass allocators individually, I'm not quite sure what's preferred.

IMO, passing individual allocators to the functions that may need them makes for a flexible design while allowing destroy functions to more easily keep track of what allocator was used. Like in rcl, it allows for using different allocators for different structs. We could follow a similar pattern as rcl and define options structs (containing allocators) and pass these options into the relevant rmw functions. This breaks API, but having an options struct seems like a nicer long-term solution for maintaining API stability.

[jacobperron]

If we decide to pass allocators individually, it'd be nice if the rmw allocator is API-compatible with the rcl allocator. Then we can just forward the allocators that users provide at the rcl layer. This should be easy if we define it as

typedef rcutils_allocator_t rmw_allocator_t;

Something to consider.

[ivanpauno]

IMO, passing individual allocators to the functions that may need them makes for a flexible design while allowing destroy functions to more easily keep track of what allocator was used

The advantage of passing the allocator in both the constructor and destructor is that later you can easily use the same allocator with many objects without each of them having a copy of (or a reference to) the allocator.

I'm not sure if it allows destroy functions to "more easily keep track" of what allocator was used. It seems more error prone to pass it twice.

[ivanpauno]

If we decide to pass allocators individually, it'd be nice if the rmw allocator is API-compatible with the rcl allocator. Then we can just forward the allocators that users provide at the rcl layer. This should be easy if we define it as

IMO, we should use the rcutils_allocator_t directly, I don't see much advantages of typedef-ing it. We need only one allocator API to be able to share allocators.

[jacobperron]

I don't know what the advantages are in typedef-ing the allocator; rcl does this. Maybe it allows us to change our minds later and define a custom allocator without changing symbol names?

---

There's also this TODO (but I don't know the context behind it, no pun intended):

https://github.com/ros2/rmw/blob/9f3c3e1b010a95013eb119ce2ae9b225bdfdc1ba/rmw/include/rmw/init_options.h#L60-L62