Update publisher creation/destruction API documentation.#252
Conversation
| * Otherwise, it will proceed despite errors, freeing as many resources as it can, including | ||
| * the publisher handle. Usage of a deallocated publisher handle is undefined behavior. | ||
| * | ||
| * \pre Given node must be the one the publisher was registered with. |
There was a problem hiding this comment.
This makes me wonder, why don't we keep a reference to the node in the publisher?
There was a problem hiding this comment.
I understand what you mean, but I think the scope of that suggestion is a little broader than the goal of this PR.
There was a problem hiding this comment.
Yeap, not planning to address it here. Just wondering why we seemingly introduce a point of failure for no apparent reason.
There was a problem hiding this comment.
I'm not sure, but I think it might encourage the caller to make sure they keep the node around (as you need it to call fini on the publisher, so it's obvious it needs to be kept around), but I don't actually remember the logic behind it. Obviously if they just follow the docs, then this would not be a useful argument.
There was a problem hiding this comment.
Thanks for the insight. We can revisit at a later point in time. I wasn't planning to change it here anyways.
rmw/include/rmw/rmw.h
Outdated
| * arguments for now. | ||
| * This function can fail, and therefore return `NULL`, if: | ||
| * - node is not a valid non-null handle for this rmw implementation, | ||
| * as returned by `rmw_create_node()`, or it is associated to a shutdown context |
There was a problem hiding this comment.
Behavior on a shutdown context was not specified. I think this makes sense.
There was a problem hiding this comment.
I'm not sure if it makes sense.
One of the possible paths to fix ros2/rclcpp#1139 is to reduce the side-effects that calling shutdown has, and this will go against that.
There was a problem hiding this comment.
Oof, that's a long winded discussion. Short term, I don't mind as long as behavior is specified.
Skimming through that thread though, this comment seems to suggest this is the current expected behavior, nevermind future changes.
There was a problem hiding this comment.
Skimming through that thread though, this comment seems to suggest this is the current expected behavior, nevermind future changes.
I think that we have settled in that PR that we want to avoid that behavior in the future.
Currently, that behavior is only enforced in rcl in "some parts" of the API (not everywhere, some things continue working after shutdown).
IMO, we should avoid introducing similar checks in rcl, it will make change to happen much more difficult in the future.
There was a problem hiding this comment.
We can settle over there, as I disagree with some of the arguments.
There was a problem hiding this comment.
Sure, but this PR shouldn't be blocked waiting on that decision.
Currently, none of the rmw implementations are checking if shutdown has already been called.
There was a problem hiding this comment.
Fair point. I do want to speed things up.
ivanpauno
left a comment
There was a problem hiding this comment.
Some minor comments, otherwise LGTM
rmw/include/rmw/rmw.h
Outdated
| * arguments for now. | ||
| * This function can fail, and therefore return `NULL`, if: | ||
| * - node is not a valid non-null handle for this rmw implementation, | ||
| * as returned by `rmw_create_node()`, or it is associated to a shutdown context |
There was a problem hiding this comment.
I'm not sure if it makes sense.
One of the possible paths to fix ros2/rclcpp#1139 is to reduce the side-effects that calling shutdown has, and this will go against that.
| * Otherwise, it will proceed despite errors, freeing as many resources as it can, including | ||
| * the publisher handle. Usage of a deallocated publisher handle is undefined behavior. | ||
| * | ||
| * \pre Given node must be the one the publisher was registered with. |
There was a problem hiding this comment.
I understand what you mean, but I think the scope of that suggestion is a little broader than the goal of this PR.
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
a02d3c3 to
06966ef
Compare
|
CI up
|
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Precisely what the title says.