-
Notifications
You must be signed in to change notification settings - Fork 67
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.
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 serialization/deserialization API documentation. #258
Changes from 6 commits
cca21b7
b821e77
6e652f2
2625c33
b43d14e
106d36c
839edab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -556,7 +556,27 @@ rmw_publisher_assert_liveliness(const rmw_publisher_t * publisher); | |
/** | ||
* The ROS message is serialized into a byte stream contained within the | ||
* rmw_serialized_message_t structure. | ||
* The serialization format depends on the underlying middleware. | ||
* The serialization format depends on the underlying implementation. | ||
* | ||
* \pre Given ROS message must be a valid non-null instance, initialized | ||
* by the caller and matching the provided typesupport. | ||
* \pre Given typesupport must be a valid non-null instance, as provided | ||
* by `rosidl` APIs. | ||
* \pre Given serialized message must be a valid non-null instance, initialized | ||
* by the caller. | ||
* | ||
* <hr> | ||
* Attribute | Adherence | ||
* ------------------ | ------------- | ||
* Allocates Memory | Maybe [1] | ||
* Thread-Safe | Yes [2] | ||
* Uses Atomics | Maybe [3] | ||
* Lock-Free | Maybe [3] | ||
* <i>[1] if the given serialized message does not have enough capacity to hold | ||
* the ROS message serialization</i> | ||
* <i>[2] as long as no two concurrent calls make use of the same `serialized_message` | ||
* object and a given `serialized_message` allocator is thread-safe as well </i> | ||
* <i>[3] rmw implementation defined, check the implementation documentation</i> | ||
* | ||
* \param[in] ros_message the typed ROS message | ||
* \param[in] type_support the typesupport for the ROS message | ||
|
@@ -577,10 +597,27 @@ rmw_serialize( | |
/** | ||
* The given rmw_serialized_message_t's internal byte stream buffer is deserialized | ||
* into the given ROS message. | ||
* The ROS message must already be allocated and initialized, and must match | ||
* the given typesupport structure. | ||
* The serialization format expected in the rmw_serialized_message_t depends on the | ||
* underlying middleware. | ||
* underlying implementation. | ||
* | ||
* \pre Given serialized message must be a valid non-null instance, such | ||
* as that returned by `rmw_serialize()`, matching provided typesupport | ||
* and ROS message. | ||
* \pre Given typesupport must be a valid non-null instance, as provided | ||
* by `rosidl` APIs. | ||
* \pre Given ROS message must be a valid non-null instance, initialized | ||
* by the caller. | ||
* | ||
* <hr> | ||
* Attribute | Adherence | ||
* ------------------ | ------------- | ||
* Allocates Memory | Maybe [1] | ||
* Thread-Safe | Yes [2] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as above, this is not thread safe IMO |
||
* Uses Atomics | Maybe [3] | ||
* Lock-Free | Maybe [3] | ||
* <i>[1] if the given ROS message contains unbounded fields</i> | ||
* <i>[2] as long as no two concurrent calls make use of the same `ros_message` object </i> | ||
* <i>[3] rmw implementation defined, check the implementation documentation</i> | ||
* | ||
* \param[in] serialized_message the serialized message holding the byte stream | ||
* \param[in] type_support the typesupport for the typed ros message | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,17 +22,81 @@ extern "C" | |
|
||
#include "rcutils/types/uint8_array.h" | ||
|
||
// aliases for rcutils_uint8_array_t | ||
/* | ||
* For now this is a simple aliasing from a serialized message to a uint8 array. | ||
/** | ||
* \struct rmw_serialized_message_t | ||
* | ||
* \brief Serialized message as a string of bytes. | ||
* | ||
* It includes (but it is not limited to) the following members: | ||
* \i \c buffer the reference to internal storage, as a pointer | ||
* \i \c buffer_length the size of stored contents, as an unsigned integer | ||
* \i \c buffer_capacity the capacity of internal storage, as an unsigned integer | ||
*/ | ||
|
||
/* For now this is a simple aliasing from a serialized message to a uint8 array. | ||
* However, in future developments this serialized message can become something | ||
* more complex and is therefore aliased. | ||
*/ | ||
typedef rcutils_uint8_array_t rmw_serialized_message_t; | ||
|
||
/// Return a zero initialized serialized message struct. | ||
#define rmw_get_zero_initialized_serialized_message rcutils_get_zero_initialized_uint8_array | ||
#define rmw_serialized_message_init rcutils_uint8_array_init | ||
#define rmw_serialized_message_fini rcutils_uint8_array_fini | ||
#define rmw_serialized_message_resize rcutils_uint8_array_resize | ||
|
||
/// Initialize a serialized message, zero initializing its contents. | ||
/** | ||
* \pre Given serialized message must have been zero initialized. | ||
* | ||
* \param[inout] serialized_message a pointer to the serialized message to initialize | ||
* \param[in] message_capacity the size of the memory to allocate | ||
* \param[in] allocator the allocator to use for the memory allocation | ||
* \return `RMW_RET_OK` if successful, or | ||
* \return `RMW_RET_INVALID_ARGUMENT` if any arguments are invalid, or | ||
* \return 'RMW_RET_BAD_ALLOC` if no memory could be allocated correctly, or | ||
* \return `RMW_RET_ERROR` if an unexpected error occurs | ||
*/ | ||
#define rmw_serialized_message_init(serialized_message, message_capacity, allocator) \ | ||
rcutils_uint8_array_init(serialized_message, message_capacity, allocator) | ||
|
||
/// Finalize a serialized message. | ||
/** | ||
* \pre Given serialized message must have been initialized with `rmw_serialized_message_init()`. | ||
* | ||
* \remarks If serialized message is zero initialized, then `RMW_RET_INVALID_ARGUMENT` is returned. | ||
* | ||
* \param[in] serialized_message pointer to the serialized message to be cleaned up | ||
* \return `RMW_RET_OK` if successful, or | ||
* \return `RMW_RET_INVALID_ARGUMENT` if serialized_message is invalid, or | ||
* \return `RMW_RET_ERROR` if an unexpected error occurs | ||
*/ | ||
#define rmw_serialized_message_fini(serialized_message) \ | ||
rcutils_uint8_array_fini(serialized_message) | ||
|
||
/// Resize the internal buffer of the serialized message. | ||
/** | ||
* The internal buffer of the serialized message can be resized dynamically | ||
* if needed. | ||
* If the new size is smaller than the current capacity, then the memory is | ||
* truncated. | ||
* | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can constrain possible states to zero-initialized or initialized, but IMO to say that using an uninitialized variable is UB goes without saying. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The thing here is that we are not just saying "it's undefined", but we are in a way specifying whether implementers of this function declaration need to add a check for null pointers, etc. That, I think, makes it worth being more explicit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the added |
||
* \pre Given serialized message must have been initialized with `rmw_serialized_message_init()`. | ||
* | ||
* \remarks If serialized message is zero initialized, then `RMW_RET_INVALID_ARGUMENT` | ||
* is returned. | ||
* | ||
* \warning Be aware that this might deallocate the memory and therefore | ||
* invalidate any pointers to the internal buffer. | ||
* | ||
* \param[inout] serialized_message pointer to the serialized message | ||
* to be resized | ||
* \param[in] new_size the new size of the internal buffer | ||
* \return `RMW_RET_OK` if successful, or | ||
* \return `RMW_RET_INVALID_ARGUMENT` if serialized_message is invalid | ||
* or new_size is set to zero, or | ||
* \return `RMW_RET_BAD_ALLOC` if memory allocation failed, or | ||
* \return `RMW_RET_ERROR` if an unexpected error occurs | ||
*/ | ||
#define rmw_serialized_message_resize(serialized_message, new_size) \ | ||
rcutils_uint8_array_resize(serialized_message, new_size) | ||
|
||
#if __cplusplus | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if
ros_message
is being modified in another thread while being serialized?IMO, this function is simply NO thread safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're discussing in circles across PRs on what we mean by thread-safe.
If we say that for a function to be thread-safe not only it should be reentrant but the read-write operations it performs on any of its arguments are also thread-safe, then yes, you're right, this function is not thread-safe. And neither are
rcutils_system_time_now()
orrcl_parse_arguments()
. And I highly doubt any of our C APIs can fulfill that requirement (we don't put synchronization mechanisms in user facing API, barely any atomics), and thus we may as well put a blanket statement saying nothing is thread-safe.Now, if we say that these APIs are thread-safe as long as they are reentrant and we document (or imply) mutually exclusive write access on their output arguments as a requirement, then we can have some thread-safe APIs, and this one is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, looking at POSIX safety terminology, it doesn't seem it makes such promises on their arguments either. For instance,
memset
is supposedly MT-Safe even though concurrently modyfing the given buffer or even realloc'ing it may result in unexpected outcomes or memory corruption.This blog from RedHat discusses POSIX safety to some extent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, after an offline discussion with @ivanpauno, we've decided it's best to (1.) relax the thread-safety requirement here, and (2.) kickstart a separate discussion to standardize and document our thread-safety definitions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a good plan to me.