Update service server/client creation/destruction API documentation.

[hidmic]

Precisely what the title says.

[hidmic]

I'd expect it to be thread-safe though. Any arguments against?

[wjwwood]

I would say no? create publisher isn't:

rmw/rmw/include/rmw/rmw.h

Line 298 in f48ebcf

* Thread-Safe | No

[hidmic]

Yeah, I know it says it's not. I added it :)

But reflecting a bit on it, don't we assume it is everywhere else? rclcpp doesn't do much to ensure not two services get created concurrently (see here, no locks). Same for publishers. And looking at implementations, they do seem thread-safe (take that with a grain bag of salt, I haven't audited that code).

[wjwwood]

Well, that might be a bug then? I'm hesitant to place a lot of thread-safety requirements on the rmw API, because it might make it more difficult to implement on various systems, and in scenarios like real-time systems where blocking is bad. That's the entire reason for mentioning locks and atomics in this stanza originally. For the major cases like publish and take, it is (imo) unavoidable to ask for it to be thread-safe and niche systems may choose to address that with polling or other lock-free operations/datastructures, but I don't really want to put that requirement in too many places. It's easier and perhaps more efficient to do the locking in the client library in most cases.

[hidmic]

Fair enough. By the same token, I wonder if even client libraries should be locking (by default it's fine, but in general it could limit usage and/or hurt performance).

[ivanpauno]

rclcpp isn't mutexing the access to rcl_node_t, so either we should fix that or make these functions thread safe.

e.g.: weird race condition when registering the same type in rmw_connext ros2/rmw_connext#442.
Maybe, I should have made access to rcl_node_t from rclcpp mutually exclusive instead of that.

[ivanpauno]

In fastrtps, the situation is a bit different:

https://github.com/ros2/rmw_fastrtps/blob/721591b4fd849e9b30374e1d6afe7b7db8c06874/rmw_fastrtps_cpp/src/publisher.cpp#L120-L131

Worst case, that will log an error, because we're ignoring the return value of the "registerType" function.
But if we want to avoid that TOCTTOU race, mutexed access to the node will not solve the problem as different nodes share the same participant.

I would say that access with the same rcl_node_t doesn't need to be thread safe (that should be guaranted by rclcpp/rclpy/rcl<another_language>), but the function should be re-entrant for different nodes (i.e. if the function is making access to state somehow shared between the nodes, the implementation must make sure that access is safe).

Does that make sense?

[wjwwood]

Two small suggestions, then lgtm

[hidmic]

CI up to test_rmw_implementation and rcl:

• Linux
• Linux-aarch64
• macOS
• Windows

[hidmic]

CI up to test_rmw_implementation and rcl:

• Linux
• Linux-aarch64
• macOS
• Windows

[hidmic]

Alright, going in !