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.

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

Merged
merged 7 commits into from
Aug 14, 2020

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Aug 10, 2020

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>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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

Comment on lines 35 to 46

/// 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines 65 to 66
* Be aware, that this might deallocate the memory and therefore invalidates any
* pointers to this storage.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

* \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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

* \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
Copy link
Contributor

Choose a reason for hiding this comment

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

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>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Aug 11, 2020

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>
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

hidmic commented Aug 11, 2020

CI up to test_rmw_implementation and rmw:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

* 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`
Copy link
Member

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.

Copy link
Contributor Author

@hidmic hidmic Aug 12, 2020

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

Copy link
Contributor Author

@hidmic hidmic Aug 12, 2020

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

* Attribute | Adherence
* ------------------ | -------------
* Allocates Memory | Maybe [1]
* Thread-Safe | Yes [2]
Copy link
Member

Choose a reason for hiding this comment

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

same as above, this is not thread safe IMO

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic merged commit db38b6a into master Aug 14, 2020
@hidmic hidmic deleted the hidmic/update-rmw-serialize-docs branch August 14, 2020 17:05
ahcorde pushed a commit that referenced this pull request Oct 13, 2020
* 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 rmw_(de)serialize() API documentation.
* Add rmw_serialize_message_t API documentation.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants