Enhance ABI stability

[ivanpauno]

Feature request

Feature description

We have run many times in the issue of not being able to backport a fix because it would break ABI.
e.g.:

• Extend the lifetime of rcl_publisher_t by holding onto the shared pointer. #1119 is almost impossible to backport.
• [eloquent] Ensure rcl_clock accesses are thread-safe (ABI-safe version). #1056

Implementation considerations

We should evaluate where the effort matters and where it doesn't.

[wjwwood]

I have some concern with the PIMPL pattern being used extensively, due to complexity and performance.

The complexity comes from just having so many more indirections within the code, especially in the case that you need to write a template method but access data via the impl class, which can cause some awkwards scenarios where you need to type erase data.

The performance is just chasing pointers can be expensive if done everywhere. I'd like to consider the "fast PIMPL" pattern, which is a bit odd, but it can result in lots of performance improvements in certain cases:

http://www.gotw.ca/gotw/028.htm

[ivanpauno]

FastPIMPL looks good to me.

There's also a lot of places where we can take advantage of an indirection we are already doing and have ABI stability for free. e.g.:

std::shared_ptr<Publisher<MessageT>>
create_publisher(...)
...
class Publisher : public PublisherBase

We are already returning a pointer to the object and never returning the object directly.
Because we're inheriting from enable_shared_from_this, we actually shouldn't construct a Publisher or PublisherBase object directly.

With some slight modifications to the current API, we can provide ABI stability:

1. Make PublisherBase constructors protected.
   Document that classes that inherit from PublisherBase must have private (or protected) constructors and a friend factory function that returns a shared_ptr.
2. Delete copy constructor/copy assignment of PublisherBase (or make them private).
3. Do 1. and 2. for Publisher.
4. Ensure we don't provide methods that when inlined can break ABI stability. i.e.: don't explicetly refer to members in methods that can be inlined, use getters instead.

1 2 3 will make impossible for an user to hold a Publisher/PublisherBase object directly.
They will have to hold always a std::shared_ptr<Pub*>.

4 ensures we don't break ABI stability because of inlined methods.
e.g. instead of accessing intra_process_is_enabled_ here, we should write this->is_intra_process_enabled().

This last rule is hard to follow correctly and needs careful reviewers ...

We could also provide PublisherHandle and PublisherBaseHandle classes, in replacement of std::shared_ptr<Pub*>.
That can make ABI stability a bit easier to follow.

[wjwwood]

Those all seem like reasonable things to try. In general I'm in favor of going over the API and trying to find places to make use more future proof, but we should just be really thoughtful for each of them rather than blindly applying PIMPL everywhere.

[ivanpauno]

Those all seem like reasonable things to try. In general I'm in favor of going over the API and trying to find places to make use more future proof, but we should just be really thoughtful for each of them rather than blindly applying PIMPL everywhere.

Yes I agree, I changed the title so the goal is clearer.