Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update service server/client creation/destruction API documentation. #276

Merged
merged 3 commits into from
Sep 24, 2020
Merged

Update service server/client creation/destruction API documentation. #276

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6d3b31d
Update service server/client creation/destruction API documentation.
hidmic Sep 22, 2020
e2f44e8
Minor cleanup
hidmic Sep 22, 2020
7a37488
Rephrase rmw_create_service() and rmw_create_client() briefs
hidmic Sep 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
150 changes: 129 additions & 21 deletions rmw/include/rmw/rmw.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1749,13 +1749,42 @@ rmw_return_loaned_message_from_subscription(
const rmw_subscription_t * subscription,
void * loaned_message);

/// Create an rmw client to communicate with the specified service
/// Create a service client that can send requests to a service server.
/**
* \param[in] node Handle to node with which to register this client
* \param[in] type_support The type_support of this rosidl service
* \param[in] service_name The name of the ROS 2 service to connect with
* \param[in] qos_policies The QoS profile policies to utilize for this connection
* \return The initialized client if successful, `nullptr` if not
* This function can fail, and therefore return `NULL`, if:
* - `node` is `NULL`, or
* - `node` does not belong to this implementation
* i.e. it does not have a matching implementation identifier, or
* - `type_support` is `NULL`, or
* - `service_name` is `NULL`, or
* - `service_name` is an empty string, or
* - (if ROS namespace conventions apply) `service_name` is invalid by
* rmw_validate_full_topic_name() definition, or
* - `qos_profile` is `NULL`, or
* - `qos_profile` has invalid or unknown policies, or
* - memory allocation fails during service client creation, or
* - an unspecified error occurs.
*
* <hr>
* Attribute | Adherence
* ------------------ | -------------
* Allocates Memory | Yes
* Thread-Safe | No
Copy link
Contributor Author

@hidmic hidmic Sep 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say no? create publisher isn't:

rmw/rmw/include/rmw/rmw.h

Line 298 in f48ebcf

* Thread-Safe | No

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@ivanpauno ivanpauno Sep 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

* Uses Atomics | Maybe [1]
* Lock-Free | Maybe [1]
* <i>[1] rmw implementation defined, check the implementation documentation</i>
*
* \pre Given `node` must be a valid node, as returned by rmw_create_node().
* \pre Given `type_support` must be a valid `rosidl` service type support, as
* returned by ROSIDL_GET_SRV_TYPE_SUPPORT().
*
* \param[in] node Node with which to register this service client.
* \param[in] type_support Type support of the service to be used.
* \param[in] service_name Name of the service to be used, often a fully qualified
* service name unless `qos_profile` is configured to avoid ROS namespace conventions
* i.e. to create a native service client.
* \param[in] qos_profile QoS policies for this service client's connections.
* \return rmw service client handle, or `NULL` if there was an error.
*/
RMW_PUBLIC
RMW_WARN_UNUSED
Expand All @@ -1766,11 +1795,36 @@ rmw_create_client(
const char * service_name,
const rmw_qos_profile_t * qos_policies);

/// Destroy and unregister a service client
/// Destroy and unregister a service client from its node.
/**
* \param[in] node The associated node whose client will be destroyed
* \param[in] client The service client to destroy
* \return RMW_RET_OK if successful, otherwise an appropriate error code
* This function will reclaim all associated resources, including the service client itself.
* Use of a destroyed service client is undefined behavior.
* This function will return early if a logical error, such as `RMW_RET_INVALID_ARGUMENT`
* or `RMW_RET_INCORRECT_RMW_IMPLEMENTATION`, ensues, leaving the given service client unchanged.
* Otherwise, it will proceed despite errors.
*
* <hr>
* Attribute | Adherence
* ------------------ | -------------
* Allocates Memory | No
* Thread-Safe | No
* Uses Atomics | Maybe [1]
* Lock-Free | Maybe [1]
* <i>[1] rmw implementation defined, check the implementation documentation</i>
*
* \pre Given `node` must be the one the service client was registered with.
* \pre Given `client` must be a valid service client, as returned by rmw_create_service().
*
* \param[in] node Node with which the given service client is registered.
* \param[in] client Service client to be destroyed.
* \return `RMW_RET_OK` if successful, or
* \return `RMW_RET_INVALID_ARGUMENT` if `node` is `NULL`, or
* \return `RMW_RET_INVALID_ARGUMENT` if `client` is `NULL`, or
* \return `RMW_RET_INCORRECT_RMW_IMPLEMENTATION` if the `node`
* implementation identifier does not match this implementation, or
* \return `RMW_RET_INCORRECT_RMW_IMPLEMENTATION` if the `client`
* implementation identifier does not match this implementation, or
* \return `RMW_RET_ERROR` if an unspecified error occurs.
*/
RMW_PUBLIC
RMW_WARN_UNUSED
Expand Down Expand Up @@ -1809,13 +1863,42 @@ rmw_take_response(
void * ros_response,
bool * taken);

/// Create an rmw service server that responds to requests
/// Create a service server that can reply to client requests.
/**
* \param[in] node The node that responds the service requests
* \param[in] type_support The type support description of this service
* \param[in] service_name The name of this service advertised across the ROS graph
* \param[in] qos_policies The QoS profile policies to utilize for connections
* \return The created service object if successful, otherwise a nullptr
* This function can fail, and therefore return `NULL`, if:
* - `node` is `NULL`, or
* - `node` does not belong to this implementation
* i.e. it does not have a matching implementation identifier, or
* - `type_support` is `NULL`, or
* - `service_name` is `NULL`, or
* - `service_name` is an empty string, or
* - (if ROS namespace conventions apply) `service_name` is invalid by
* rmw_validate_full_topic_name() definition, or
* - `qos_profile` is `NULL`, or
* - `qos_profile` has invalid or unknown policies, or
* - memory allocation fails during service server creation, or
* - an unspecified error occurs
*
* <hr>
* Attribute | Adherence
* ------------------ | -------------
* Allocates Memory | Yes
* Thread-Safe | No
* Uses Atomics | Maybe [1]
* Lock-Free | Maybe [1]
* <i>[1] rmw implementation defined, check the implementation documentation</i>
*
* \pre Given `node` must be a valid node, as returned by rmw_create_node().
* \pre Given `type_support` must be a valid `rosidl` service type support, as
* returned by ROSIDL_GET_SRV_TYPE_SUPPORT().
*
* \param[in] node Node with which to register this service server.
* \param[in] type_support Type support of the service to be served.
* \param[in] service_name Name of the service to be served, often a fully qualified
* service name unless `qos_profile` is configured to avoid ROS namespace conventions
* i.e. to create a native service server.
* \param[in] qos_profile QoS policies for this service server's connections.
* \return rmw service handle, or `NULL` if there was an error.
*/
RMW_PUBLIC
RMW_WARN_UNUSED
Expand All @@ -1824,13 +1907,38 @@ rmw_create_service(
const rmw_node_t * node,
const rosidl_service_type_support_t * type_support,
const char * service_name,
const rmw_qos_profile_t * qos_policies);
const rmw_qos_profile_t * qos_profile);

/// Destroy and unregister the service from this node
/// Destroy and unregister a service server from its node.
/**
* \param[in] node The node that owns the service that is being destroyed
* \param[in] service Pointer to the service type created in `rmw_create_service`
* \return RMW_RET_OK if successful, otherwise an appropriate error code
* This function will reclaim all associated resources, including the service server itself.
* Use of a destroyed service server is undefined behavior.
* This function will return early if a logical error, such as `RMW_RET_INVALID_ARGUMENT`
* or `RMW_RET_INCORRECT_RMW_IMPLEMENTATION`, ensues, leaving the given service server unchanged.
* Otherwise, it will proceed despite errors.
*
* <hr>
* Attribute | Adherence
* ------------------ | -------------
* Allocates Memory | No
* Thread-Safe | No
* Uses Atomics | Maybe [1]
* Lock-Free | Maybe [1]
* <i>[1] rmw implementation defined, check the implementation documentation</i>
*
* \pre Given `node` must be the one the service server was registered with.
* \pre Given `service` must be a valid service server, as returned by rmw_create_service().
*
* \param[in] node Node with which the given service server is registered.
* \param[in] service Service server to be destroyed.
* \return `RMW_RET_OK` if successful, or
* \return `RMW_RET_INVALID_ARGUMENT` if `node` is `NULL`, or
* \return `RMW_RET_INVALID_ARGUMENT` if `service` is `NULL`, or
* \return `RMW_RET_INCORRECT_RMW_IMPLEMENTATION` if the `node`
* implementation identifier does not match this implementation, or
* \return `RMW_RET_INCORRECT_RMW_IMPLEMENTATION` if the `service`
* implementation identifier does not match this implementation, or
* \return `RMW_RET_ERROR` if an unspecified error occurs.
*/
RMW_PUBLIC
RMW_WARN_UNUSED
Expand Down