Fixes to support proper lifecycle of the rmw objects and other tear down issues #51
Conversation
|
+1 |
| if (rmw_destroy_client(client_handle_) == RMW_RET_ERROR) { | ||
| std::cerr << "Error in destruction of rmw client handle: " << | ||
| (rmw_get_error_string() ? rmw_get_error_string() : "") << | ||
| std::endl; |
There was a problem hiding this comment.
That's what uncrustify does this to it... I'm pretty nonplussed about it's ability to handle some of these cases, but I don't have time to look into why it does that or come up with another solution. I guess I can add the comments around it to disable the check.
|
+1 |
This prevents the node handle from getting deleted before things it created can be deleted. I also added destructors where necessary.
| if (rmw_destroy_client(client_handle_) == RMW_RET_ERROR) { | ||
| // *INDENT-OFF* | ||
| std::cerr << "Error in destruction of rmw client handle: " | ||
| << (rmw_get_error_string() ? rmw_get_error_string() : "") |
There was a problem hiding this comment.
Hmm. If the error string is unavailable, should the ternary statement instead return something like "RMW error unknown"?
There was a problem hiding this comment.
Maybe it should be something like "error string not set". In reality the else case shouldn't happen, this is just defensive so that we don't trying to send a null pointer to std::cerr.
I also think we should encapsulate this logic in a function, maybe rmw_get_error_string_safe or something like that, where it would guarantee that cstring is returned ("" or "error string not set") and a null pointer is not. Currently it can return null if it is not set.
There was a problem hiding this comment.
+1 for putting it in a function, since it's repeated several places in this code.
There was a problem hiding this comment.
I made it into a function, const char * rmw_get_error_string_safe(). I'm open to different names (should be easy to change it).
There was a problem hiding this comment.
Oh and it returned "error string not set" rather than "", so that will change existing behavior in a few places, but it shouldn't make a difference.
| std::shared_ptr<rmw_node_t> node_handle, | ||
| rmw_client_t * client_handle, | ||
| const std::string & service_name) | ||
| : node_handle_(node_handle), client_handle_(client_handle), service_name_(service_name) | ||
| {} | ||
|
|
||
| ~ClientBase() |
There was a problem hiding this comment.
Unrelated to the PR itself but probably necessary for cleaning up user created instances:
Shouldn't the destructor be virtual? (same for multiple other classes)
|
I think all of the comments have been addressed. |
|
+1 |
| rmw_destroy_client(client_handle_); | ||
| client_handle_ = nullptr; | ||
| if (client_handle_) { | ||
| if (rmw_destroy_client(client_handle_) == RMW_RET_ERROR) { |
There was a problem hiding this comment.
Should we stick to one pattern? E.g. rmw_*(..) != RMW_RET_OK.
|
+1 for squash and merge. As a follow up I would replace the usage of |
Fixes to support proper lifecycle of the rmw objects and other tear down issues
* check rmw id using RCL_CHECK_RMW_ID_MATCHES_OR_DIE * return a more distinct return code on mismatch * rename env var to RCL_ASSERT_RMW_ID_MATCHES * [style] fixup
While prototyping the intra-process system I ran into some blocking teardown issues that manifested in my multi node examples during runtime. So I had to fix them. This will be matched with pr's on rmw and the implementations.
Connects ros2/ros2#69