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 init/shutdown API documentation. #243

Merged
merged 2 commits into from
Jun 29, 2020

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Jun 23, 2020

Constrain behavior for known context states. Alternative to #242.

Constrain behavior for known context states.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
rmw/include/rmw/init.h Show resolved Hide resolved
* guard conditions, and is also required to properly call `rmw_shutdown()`.
*
* \pre The given context must be zero initialized.
*
Copy link
Member

@ivanpauno ivanpauno Jun 23, 2020

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.

@hidmic hidmic requested a review from brawner June 23, 2020 17:06
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Jun 29, 2020

Alright, going in!

@hidmic hidmic merged commit 4a1914a into master Jun 29, 2020
@delete-merged-branch delete-merged-branch bot deleted the hidmic/update-init-shutdown-docs branch June 29, 2020 17:14
ahcorde pushed a commit that referenced this pull request Oct 13, 2020
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
ahcorde pushed a commit that referenced this pull request Oct 13, 2020
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.

None yet

5 participants