fix for waitset api change #7
Conversation
|
It doesn't look like you ever destroy the |
| @@ -111,6 +112,9 @@ rcl_wait_set_init( | |||
| wait_set->impl->rmw_subscriptions.subscriber_count = 0; | |||
| wait_set->impl->rmw_guard_conditions.guard_conditions = NULL; | |||
| wait_set->impl->rmw_guard_conditions.guard_condition_count = 0; | |||
| wait_set->impl->rmw_waitset = rmw_create_waitset( | |||
| NULL, 2*number_of_subscriptions + number_of_guard_conditions); | |||
There was a problem hiding this comment.
You should check to make sure this function succeeded, and if it doesn't you should goto fail and make sure it is cleaned up in __wait_set_clean_up.
|
C is like crawling on your hands and knees. I'd recommend putting the wait set clean up in |
|
Yeah man. Life is hard. c50ab45 |
| @@ -77,6 +78,7 @@ __wait_set_clean_up(rcl_wait_set_t * wait_set, rcl_allocator_t allocator) | |||
| assert(ret == RCL_RET_OK); // Defensive, shouldn't fail with size 0. | |||
| } | |||
| if (wait_set->impl) { | |||
| assert(rmw_destroy_waitset(wait_set->impl->rmw_waitset) == RMW_RET_OK); | |||
There was a problem hiding this comment.
This needs to be guarded by a if (wait_set->impl->rmw_waitset) {...} because it is possible for allocation of wait_set->impl to be successful, but wait_set->impl->rmw_waitset is still NULL or does not point to allocated memory.
|
+1 |
jacquelinekay
force-pushed
the
waitset_handle
branch
from
4b0b928 to
bae84b3
Compare
| @@ -79,6 +80,9 @@ __wait_set_clean_up(rcl_wait_set_t * wait_set, rcl_allocator_t allocator) | |||
| assert(ret == RCL_RET_OK); // Defensive, shouldn't fail with size 0. | |||
| } | |||
| if (wait_set->impl) { | |||
| if (wait_set->impl->rmw_waitset) { | |||
| assert(rmw_destroy_waitset(wait_set->impl->rmw_waitset) == RMW_RET_OK); | |||
There was a problem hiding this comment.
Shouldn't this be checked in release builds too?
There was a problem hiding this comment.
I'm following the pattern of the above code here. Since this function signature has no way of returning an error code, I'm not sure how to propagate the error to the highest level. @wjwwood any thoughts on how these functions should report errors in release builds?
There was a problem hiding this comment.
Sorry, I had to do some research first. This is definitely a bug and needs to be fixed through out. I'm in the process of auditing rcl for this problem.
In the example above rmw_destroy_waitset(...) is never run if -DNDEBUG is passed to the compiler.
There was a problem hiding this comment.
As to whether or not the check should be made, it is a situation where the it should never fail, but because it could in other scenarios there is a warning when you don't check the return value. This check is really defensive and should never be false.
There was a problem hiding this comment.
I actually don't see any other problematic uses of asserts (where needed code is avoided with NDEBUG) right now.
Perhaps we need a custom assert (it's a fairly common thing in projects) which is always checked. I think there's a need for "I want to make a defensive check that should never be false, and if it is it is not recoverable" but that is not disabled when -DNDEBUG is used. The consensus on the internet is that you can easily make one with printf and abort which are portable.
There was a problem hiding this comment.
I agree, I think checking it in debug only is ok (that was the idea with the assert anyways), but then it results in a unused variable warnings which requires us to but in that obnoxious other "no lint" line. I'm not sure whether or not the performance difference is appreciable.
There was a problem hiding this comment.
Another thing I'll mention here is that the other functions are implemented in this file and I can ensure they shouldn't fail in that scenario fairly easily. But the same might not be so easy to say of the rmw_destroy_waitset function since it is presumably implemented by each middleware implementation and might fail in some scenario. In which an assert might not be the right action to take here. But I think it's ok to handle that when it comes up and leave the assert for now.
There was a problem hiding this comment.
Since the existing implementations of rmw_destroy_waitset do return an error code in certain situations I think the return value should definitely be checked here (meaning also in release). Otherwise the rmw impl. might detect a problem but it stays unnoticed here.
There was a problem hiding this comment.
It's a good point that rmw_destroy_waitset isn't implemented in this file, unlike the other functions.
Should I change the signature of this function to return an error code to propagate it to the user?
There was a problem hiding this comment.
I've decided the minimal viable solution here is to mirror what's in publisher.c and move the rmw_destroy_waitset call to wait_set_fini, which already has the right function signature to propagate an error code.
jacquelinekay
force-pushed
the
waitset_handle
branch
from
2eb4850 to
d6ea437
Compare
fix for waitset api change
Add allocators for clients and services
Rename set_events_executor_callback->set_listener_callback
Connects to ros2/rmw#49