Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Relax rmw_context_fini() API contract #242
Relax rmw_context_fini() API contract #242
Changes from all commits
07a96bd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
are this being deleted because we are relaxing the requirement or because it's redundant with L132?
If the first case holds, isn't possible to ensure that in the rmw implementations?
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.
Both. Implementations complain about incorrect implementation identifier on a zero-initialized context. Even when that's not the case, guarding against partial zero-initialization (or bad initialization in the most general case) is a tough one (are init options valid to be finalized?).
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.
And I just realized we're not finalizing init options in
rmw_context_t
!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 that guarding against "partially-initialized" or bad initialized is hard, but guarding against "zero-initialized" is possible.
It's not clear if a "zero-initialized" context is a "valid argument" or not, thus the docblock should be clear about that.
e.g.: zero-initialized is a valid argument for
rmw_init
but not forrmw_context_fini
.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.
Doesn't it read
right above?
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.
yeap, but it's not clear what error code it returns:
which of both apply?
should it be whatever the rmw implementation preferred? or should it be well specified?
IMO, the following code should have a specified behavior:
and behave in the same way for all rmw implementations.
In the doc block, it's not clear if in this situation a rmw implementation returns
RMW_RET_INVALID_ARGUMENT
orRMW_RET_INCORRECT_RMW_IMPLEMENTATION
(or at least, it's not super clear to me).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.
Your comment is spot on and is directly related to ros2/rmw_implementation#107 (comment).
There's no clear definition of what an invalid context is. Common
rmw
implementations don't bother and, at best, simply check members for nullity. But which one goes first is undefined and thus whileRMW_RET_INCORRECT_RMW_IMPLEMENTATION
is the usual outcome, anrmw
may equally check for theimpl
pointer first and returnRMW_RET_INVALID_ARGUMENT
. So I chose to relax the contract to match what is.But let's say we want consistency. We can add a byte-to-byte comparison between the given context and a zero-initialized one to cover that specific kind of invalid context. The rest would remain UB. CC @brawner
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.
Yeah, it's impossible to pretend an specific error a in partially initialized context, because it would depend in the order the rmw implementation do the checks.
I prefer consistency in the case of the zero initialized context, but I also don't mind if it's preferred to relax the requirement. In the later case, I would prefer having an explicit sentence in the docblock saying that the return code for that case is unspecified (and different to RMW_RET_OK).
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 thought this through a few more times. Zero-initialized contexts used anywhere but for
rmw_init()
is bogus code. But on the other hand, it is one of the known context states.So I think I'll shift gears and drop this patch in favor of constraining all init/shutdown API to behave in a predictable way for those known states.
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.
See #243.