-
Notifications
You must be signed in to change notification settings - Fork 67
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes me wonder, why don't we keep a reference to the node in the publisher?
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Behavior on a shutdown context was not specified. I think this makes sense.
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'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can settle over there, as I disagree with some of the arguments.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point. I do want to speed things up.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Precisely what the title says.