Update subscription API documentation #256
Conversation
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
|
CI up to
|
rmw/include/rmw/rmw.h
Outdated
| * - topic_name is not a valid non-null topic name, according to | ||
| * `rmw_validate_full_topic_name()` | ||
| * - qos_profile is not a fully specified non-null profile i.e. no UNKNOWN policies | ||
| * - subscription_options is not a valid non-null option set, as returned by |
There was a problem hiding this comment.
It's a minor language thing, but "as returned by" implies that you have to pass the rmw_subscription_options_t returned by rmw_get_default_subscription_options().
rmw/include/rmw/rmw.h
Outdated
| /// Finalize a given subscription handle, reclaim the resources, and deallocate the subscription | ||
| /// handle. | ||
| /** | ||
| * This function will return early if a logical error, such as `RMW_RET_INVALID_ARGUMENT` |
There was a problem hiding this comment.
Can this be specific about what logical errors are allowed to trigger an early return, rather than saying "such as"?
There was a problem hiding this comment.
Yes it can, and in this case, it's purely phrasing i.e. replacing "such as" with "namely" will remain true.
| * 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 subscription handle | ||
| * unchanged. | ||
| * Otherwise, it will proceed despite errors, freeing as many resources as it can, including |
There was a problem hiding this comment.
freeing as many resources as it can
This is terribly non-deterministic. After this function returns, the caller won't know what has been freed and what has not.
There was a problem hiding this comment.
This is terribly non-deterministic. After this function returns, the caller won't know what has been freed and what has not.
I guess the other alternative is to give up as soon as we hit the first error. That also seems non-deterministic (and hard to document), but would that be better in your view?
There was a problem hiding this comment.
This is terribly non-deterministic. After this function returns, the caller won't know what has been freed and what has not.
I wouldn't say non-deterministic, but rather unspecified. And yes, the caller won't know to what extent the entity was finalized, nor can do anything about it as the subscription will have been deallocated.
As to whether that's a good error handling approach during finalization or not, we've discussed this quite a bit throughout related PRs. To proceed with finalization despite errors precludes the need for strong error (exception) guarantees, which may not even be achievable for a given implementation, and/or dealing with partially finalized entities, likely implementation specific and thus unspecified. The fact that finalization functions are often invoked from object destructors in higher-level language bindings (i.e. at a point where the object is going out of scope anyways) may have also influenced the decision.
I've had mixed feelings about this in the past. But at this point I think that allowing a finalization function to fail helps no one, let alone returning before completion. If it does, it's because you've hit (a) a transient error, which an implementation could deal with by deferring side effects if need be, (b) a logical error, in which the case the program is already broken, or (c) a system error, in which case you can no longer trust the OS that's supporting you.
There was a problem hiding this comment.
Yeah, I guess it's the same as the approach that C++ destructors take: If there's an error in the destructor, give up and assume everything is broken beyond repair.
It is good, though, to be a bit more specific about what errors will cause predictable failure and which will leave things in an indeterminate state. The recent changing of "such as" to "namely" helps there but I think the text could be formatted to make it more obvious, e.g. using a list.
There was a problem hiding this comment.
If there's an error in the destructor, give up and assume everything is broken beyond repair.
Yeah, but that's largely dependent on the application. Just like in a C++ destructor, you could suppress/log that error (exception) and keep going, which is by far the common pattern in rcl and above.
It is good, though, to be a bit more specific about what errors will cause predictable failure and which will leave things in an indeterminate state.
Hmm, I'm not exactly sure what you're asking for. If you mean that we should specify all possible return codes, they are listed exhaustively below. If you mean that we should specify failure modes, we can't as that's RMW implementation specific. If you mean that we should split the generic RMW_RET_ERROR return code into a few ones to aid the caller on how to proceed, I agree we could but that's orthogonal to this patch (and a new feature entirely).
There was a problem hiding this comment.
Yeah, I guess it's the same as the approach that C++ destructors take: If there's an error in the destructor, give up and assume everything is broken beyond repair.
Maybe that will make destruction code simpler (no need to handle chained errors).
There was a problem hiding this comment.
Hmm, I'm not exactly sure what you're asking for.
If there are errors that could occur but will result in the function not doing anything, those need to be explicitly listed. If there are errors that are known that may leave things in an indefinite state, those also should be listed.
The two are separate from a statement saying "Unknown errors may occur". Basically I'm looking for information that allows a developer to be certain about whether things are now in an unknown state or not. The current wording achieves that, but I feel that a list is more understandable. It's not a requirement, though, so feel free to ignore this if you wish.
There was a problem hiding this comment.
If there are errors that could occur but will result in the function not doing anything, those need to be explicitly listed.
They are (though I'm not entirely convinced we should be handling logical errors in run-time in any way but terminating the program).
If there are errors that are known that may leave things in an indefinite state, those also should be listed.
There are no known errors, only a blanket one.
Basically I'm looking for information that allows a developer to be certain about whether things are now in an unknown state or not.
See e6620b0. If you get RMW_RET_ERROR, then it's highly likely that something didn't get fully finalized.
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
|
@gbiggs PTAL! |
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
* Update subscription creation/destruction API documentation. * Update subscription QoS querying API documentation. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
* Update subscription creation/destruction API documentation. * Update subscription QoS querying API documentation. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Precisely what the title says. Each commit is to be kept separately.