Conversation
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
| * Attribute | Adherence | ||
| * ------------------ | ------------- | ||
| * Allocates Memory | Yes | ||
| * Thread-Safe | No |
There was a problem hiding this comment.
I'd expect it to be thread-safe though. Any arguments against?
There was a problem hiding this comment.
I would say no? create publisher isn't:
Line 298 in f48ebcf
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
In fastrtps, the situation is a bit different:
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
left a comment
There was a problem hiding this comment.
Two small suggestions, then lgtm
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
|
Alright, going in ! |
…276) Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
…276) Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Precisely what the title says.