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

Jump to bottom

Update init/shutdown API documentation. #243

Merged
merged 2 commits into from
Jun 29, 2020
Merged

Update init/shutdown API documentation. #243

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
37a9d77
Update init/shutdown API documentation.
hidmic Jun 19, 2020
326e6d6
Further constrain init/shutdown API contracts.
hidmic Jun 23, 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
49 changes: 35 additions & 14 deletions rmw/include/rmw/init.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,19 @@ rmw_get_zero_initialized_context(void);

/// Initialize the middleware with the given options, and yielding an context.
/**
* The given context must be zero initialized, and is filled with
* middleware specific data upon success of this function.
* Context is filled with middleware specific data upon success of this function.
* The context is used when initializing some entities like nodes and
* guard conditions, and is also required to properly call rmw_shutdown().
* guard conditions, and is also required to properly call `rmw_shutdown()`.
*
* \pre The given options must have been initialized
* i.e. `rmw_init_options_init()` called on it.
* \pre The given context must be zero initialized.
*
Copy link
Member

@ivanpauno ivanpauno Jun 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

is it possible to ensure that the context will remain zero initialized if rmw_init failed?

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'll assume you mean rmw_init(). If we can provide an adequate return code, we can rollback what we've done and reset the content of the struct. Note this talks about the rmw_context_t struct and not about any other global state that an rmw implementation may initialize as a side effect. We can't make promises there.

Copy link
Member

Choose a reason for hiding this comment

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

If it doesn't add much burden to the implementation, I would try to always enforce a "strong failure guarantee" (equivalent to the C++ strong exception safety guarantee), more than only a "basic failure" guarantee).

I'll assume you mean rmw_init()

Yes, edited.

Note this talks about the rmw_context_t struct and not about any other global state that an rmw implementation may initialize as a side effect

yes, there might be visible side effects that we can't control.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it doesn't add much burden to the implementation, I would try to always enforce a "strong failure guarantee"

Yeah, I know where you're going. Because I don't know for sure what is (or will be) out there, I try not to commit to more than it is strictly necessary for it not to be imprecise. In this specific case, I'd rather not make strong guarantees about anything but the struct just yet. @wjwwood for feedback.

Copy link
Member

Choose a reason for hiding this comment

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

I don't mind one way or the other. We don't usually reset structures on failure, but we do things like reclaim resources that would otherwise be leaked, which is a little more important in my opinion. The user could always re-zero init the structure if it failed. I think the only important guarantee is that no resources are leaked, so it is not needed to call shutdown or fini on it and it's say to overwrite it.

* \post If initialization fails, context will remain zero initialized.
*
* \remarks If options are zero-initialized, then `RMW_RET_INVALID_ARGUMENT` is returned.
* If context has been already initialized (`rmw_init()` was called on it), then
* `RMW_RET_INVALID_ARGUMENT` is returned.
*
* <hr>
* Attribute | Adherence
Expand All @@ -73,9 +82,9 @@ rmw_get_zero_initialized_context(void);
* \param[in] options initialization options to be used during initialization
* \param[out] context resulting context struct
* \return `RMW_RET_OK` if successful, or
* \return `RMW_RET_INVALID_ARGUMENT` if any arguments are invalid, or
* \return `RMW_RET_INCORRECT_RMW_IMPLEMENTATION` if the implementation
* identifier does not match, or
* \return `RMW_RET_INVALID_ARGUMENT` if any arguments are null or invalid, or
* \return `RMW_RET_ERROR` if an unexpected error occurs.
*/
RMW_PUBLIC
Expand All @@ -85,8 +94,11 @@ rmw_init(const rmw_init_options_t * options, rmw_context_t * context);

/// Shutdown the middleware for a given context.
/**
* The given context must be a valid context which has been initialized
* with rmw_init().
* \pre The given context must be a valid context which has been initialized with `rmw_init()`.
*
* \remarks If context is zero initialized, then `RMW_RET_INVALID_ARGUMENT` is returned.
* If context has been already invalidated (`rmw_shutdown()` was called on it), then
* this function is a no-op and `RMW_RET_OK` is returned.
*
* <hr>
* Attribute | Adherence
Expand All @@ -100,9 +112,9 @@ rmw_init(const rmw_init_options_t * options, rmw_context_t * context);
*
* \param[in] context resulting context struct
* \return `RMW_RET_OK` if successful, or
* \return `RMW_RET_INVALID_ARGUMENT` if any argument are invalid, or
* \return `RMW_RET_INCORRECT_RMW_IMPLEMENTATION` if the implementation
* identifier does not match, or
* \return `RMW_RET_INVALID_ARGUMENT` if the argument is null or invalid, or
* \return `RMW_RET_ERROR` if an unexpected error occurs.
*/
RMW_PUBLIC
Expand All @@ -112,12 +124,17 @@ rmw_shutdown(rmw_context_t * context);

/// Finalize a context.
/**
* The context to be finalized must have been previously initialized with
* `rmw_init()`, and then later invalidated with `rmw_shutdown()`.
* If context is `NULL`, then `RMW_RET_INVALID_ARGUMENT` is returned.
* If context is zero-initialized, then `RMW_RET_INVALID_ARGUMENT` is returned.
* If context is initialized and valid (`rmw_shutdown()` was not called on it),
* then `RMW_RET_INVALID_ARGUMENT` is returned.
* 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 context unchanged.
* Otherwise, it will proceed despite errors, freeing as much resources as it can and zero
* initializing the given context.
*
* \pre The context to be finalized must have been previously initialized with
* `rmw_init()`, and then later invalidated with `rmw_shutdown()`.
*
* \remarks If context is zero initialized, then `RMW_RET_INVALID_ARGUMENT` is returned.
* If context is initialized and valid (`rmw_shutdown()` was not called on it), then
* `RMW_RET_INVALID_ARGUMENT` is returned.
*
* <hr>
* Attribute | Adherence
Expand All @@ -128,8 +145,12 @@ rmw_shutdown(rmw_context_t * context);
* Lock-Free | Yes [1]
* <i>[1] if `atomic_is_lock_free()` returns true for `atomic_uint_least64_t`</i>
*
* \return `RMW_RET_OK` if the shutdown was completed successfully, or
* This should be defined by the rmw implementation.
ivanpauno marked this conversation as resolved.
Show resolved Hide resolved
*
* \return `RMW_RET_OK` if successful, or
* \return `RMW_RET_INVALID_ARGUMENT` if any arguments are invalid, or
* \return `RMW_RET_INCORRECT_RMW_IMPLEMENTATION` if the implementation
* identifier does not match, or
* \return `RMW_RET_ERROR` if an unspecified error occur.
*/
RMW_PUBLIC
Expand Down