Update serialization/deserialization API documentation. #258

hidmic · 2020-08-10T21:40:36Z

Precisely what the title says. Just clarifying what was already given, plus specifying rmw_serialized_message_t (regardless of it being an alias).

I'll point out that this API departs from the rest, in that every argument MUST be correct or you'll incur UB.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

gbiggs · 2020-08-11T00:33:24Z

rmw/include/rmw/serialized_message.h

+
+/// Finalize a serialized message.
+/**
+ * Passing an uninitialized instance to this function leads to undefined


Is it better to make this a checked pre-condition?

At the very least, it's probably a good idea to add a \pre notice like we did above.

This needs an update. Passing uninitialized arguments will always be UB, and known states (zero-initialized or initialized) will be handled correctly in runtime.

clalancette · 2020-08-11T14:20:21Z

rmw/include/rmw/serialized_message.h

+
+/// Finalize a serialized message.
+/**
+ * Passing an uninitialized instance to this function leads to undefined


At the very least, it's probably a good idea to add a \pre notice like we did above.

clalancette · 2020-08-11T14:22:02Z

rmw/include/rmw/serialized_message.h

+
+/// Initialize a serialized message, zero initializing its contents.
+/**
+ * \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
+ * \return `RMW_RET_ERROR` if an unexpected error occurs
+ */
 #define rmw_serialized_message_init rcutils_uint8_array_init


Just a thought here. I would find this documentation easier to read if rmw_serialized_message_init had arguments. That is, can we change the macro to:

#define rmw_serialized_message_init(serialized_message, message_capacity, allocator) rcutils_uint8_array_init(serialized_message, message_capacity, allocator)

The same goes for all of the other rmw_serialized* macros in this file.

clalancette · 2020-08-11T14:23:58Z

rmw/include/rmw/serialized_message.h

+ * Be aware, that this might deallocate the memory and therefore invalidates any
+ * pointers to this storage.


This sentence is grammatically correct, but I would still find a modification slightly easier to read:

Suggested change

* Be aware, that this might deallocate the memory and therefore invalidates any

* pointers to this storage.

* Be aware, that this might deallocate the memory and therefore invalidate any

* pointers to this storage.

Sounds good. At some point we should correct rcutils_uint8_array_t API documentation as well.

clalancette · 2020-08-11T14:24:48Z

rmw/include/rmw/serialized_message.h

+ * truncated.
+ * Be aware, that this might deallocate the memory and therefore invalidates any
+ * pointers to this storage.
+ *


Should we add \pre serialized_message must have been initialized or similar?

We can constrain possible states to zero-initialized or initialized, but IMO to say that using an uninitialized variable is UB goes without saying.

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.

I think the added \pre and \remarks should have us covered.

clalancette · 2020-08-11T14:25:07Z

rmw/include/rmw/serialized_message.h

+ * \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 new_size is set to zero


Suggested change

* \return `RMW_RET_INVALID_ARGUMENT` if new_size is set to zero

* \return `RMW_RET_INVALID_ARGUMENT` if new_size is set to zero, or

clalancette · 2020-08-11T14:25:18Z

rmw/include/rmw/serialized_message.h

+ * \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


Suggested change

* \return 'RMW_RET_BAD_ALLOC` if no memory could be allocated correctly

* \return 'RMW_RET_BAD_ALLOC` if no memory could be allocated correctly, or

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic · 2020-08-11T15:39:54Z

Alright, PTAL! I've also added API attributes based on what current implementations do, thread-safety being the most constraining spec.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

clalancette

I like this a lot more. Thanks @hidmic! I'll suggest we run CI on this change, since there is a very small (but non-zero) chance that changing the macros breaks something.

hidmic · 2020-08-11T20:39:39Z

CI up to test_rmw_implementation and rmw:

Linux
Linux-aarch64
macOS
Windows

ivanpauno · 2020-08-12T12:57:59Z

rmw/include/rmw/rmw.h

+ * 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`


what if ros_message is being modified in another thread while being serialized?

IMO, this function is simply NO thread safe.

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() or rcl_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.

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.

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.

Sounds like a good plan to me.

ivanpauno · 2020-08-12T12:58:55Z

rmw/include/rmw/rmw.h

+ * Attribute          | Adherence
+ * ------------------ | -------------
+ * Allocates Memory   | Maybe [1]
+ * Thread-Safe        | Yes [2]


same as above, this is not thread safe IMO

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

* Update rmw_(de)serialize() API documentation. * Add rmw_serialize_message_t API documentation. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Update serialization/deserialization API documentation.

cca21b7

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic mentioned this pull request Aug 10, 2020

Add serialize/deserialize API test coverage. ros2/rmw_implementation#118

Merged

Add rmw_serialize_message_t API documentation.

b821e77

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic requested review from ivanpauno and clalancette August 10, 2020 22:12

gbiggs reviewed Aug 11, 2020

View reviewed changes

clalancette requested changes Aug 11, 2020

View reviewed changes

hidmic added 3 commits August 11, 2020 12:34

Address peer review comments.

6e652f2

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Document rmw_(de)serialize() API attributes.

2625c33

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Relax rmw_(de)serialize() API attributes.

b43d14e

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic requested review from clalancette and gbiggs August 11, 2020 15:39

Augment rmw_serialized_message_t documentation.

106d36c

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

clalancette reviewed Aug 11, 2020

View reviewed changes

clalancette approved these changes Aug 11, 2020

View reviewed changes

ivanpauno reviewed Aug 12, 2020

View reviewed changes

Relax thread-safety specs.

839edab

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

ivanpauno approved these changes Aug 12, 2020

View reviewed changes

hidmic mentioned this pull request Aug 12, 2020

Standardize thread-safety documentation ros2/ros2#1004

Open

gbiggs approved these changes Aug 12, 2020

View reviewed changes

hidmic merged commit db38b6a into master Aug 14, 2020

hidmic deleted the hidmic/update-rmw-serialize-docs branch August 14, 2020 17:05

hidmic mentioned this pull request Aug 14, 2020

Extend rmw_serialized_message_t tests. #261

Merged

ahcorde pushed a commit that referenced this pull request Oct 13, 2020

Update serialization/deserialization API documentation. (#258)

f8733f4

* Update rmw_(de)serialize() API documentation. * Add rmw_serialize_message_t API documentation. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

ahcorde pushed a commit that referenced this pull request Oct 13, 2020

Update serialization/deserialization API documentation. (#258)

da129f9

* Update rmw_(de)serialize() API documentation. * Add rmw_serialize_message_t API documentation. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update serialization/deserialization API documentation. #258

Update serialization/deserialization API documentation. #258

hidmic commented Aug 10, 2020 •

edited

Loading

gbiggs Aug 11, 2020

clalancette Aug 11, 2020

hidmic Aug 11, 2020

clalancette Aug 11, 2020

clalancette Aug 11, 2020

clalancette Aug 11, 2020

hidmic Aug 11, 2020

clalancette Aug 11, 2020

hidmic Aug 11, 2020

gbiggs Aug 12, 2020

hidmic Aug 12, 2020

clalancette Aug 11, 2020

clalancette Aug 11, 2020

hidmic commented Aug 11, 2020

clalancette left a comment

hidmic commented Aug 11, 2020

ivanpauno Aug 12, 2020

hidmic Aug 12, 2020 •

edited

Loading

hidmic Aug 12, 2020 •

edited

Loading

hidmic Aug 12, 2020

gbiggs Aug 18, 2020

ivanpauno Aug 12, 2020

		* Be aware, that this might deallocate the memory and therefore invalidates any
		* pointers to this storage.

	* \return `RMW_RET_INVALID_ARGUMENT` if new_size is set to zero
	* \return `RMW_RET_INVALID_ARGUMENT` if new_size is set to zero, or

	* \return 'RMW_RET_BAD_ALLOC` if no memory could be allocated correctly
	* \return 'RMW_RET_BAD_ALLOC` if no memory could be allocated correctly, or

Update serialization/deserialization API documentation. #258

Update serialization/deserialization API documentation. #258

Conversation

hidmic commented Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic commented Aug 11, 2020

clalancette left a comment

Choose a reason for hiding this comment

hidmic commented Aug 11, 2020

Choose a reason for hiding this comment

hidmic Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

hidmic Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic commented Aug 10, 2020 •

edited

Loading

hidmic Aug 12, 2020 •

edited

Loading

hidmic Aug 12, 2020 •

edited

Loading