Topic fix rcl lifecycle test issue #715
Conversation
There was a problem hiding this comment.
It would be great if you could tackle the two issues reported in #713 individually. That makes reviewing/merging a tick faster.
I would recommend having this PR tackle the rcutils_reset_error findings and a follow up PR with the memory leaks.
rcl_lifecycle/src/rcl_lifecycle.c
Outdated
| @@ -161,17 +161,21 @@ rcl_lifecycle_transition_fini( | |||
|
|
|||
| rcl_ret_t ret = RCL_RET_OK; | |||
|
|
|||
| if (rcl_lifecycle_state_fini(transition->start, allocator) != RCL_RET_OK) { | |||
| ret = RCL_RET_ERROR; | |||
| if (transition->start) { | |||
There was a problem hiding this comment.
why is that needed? Calling fini/deallocate on a nullptr should be totally fine.
There was a problem hiding this comment.
Yes. Behavior is correct.
But I think it is better for rcl_lifecycle_transition_fini() to take charge of checking its members transition->start instead of depending on other function. ( Of course, this is not good to be included this commit)
Like below codes. It doesn't depend on rcl_yaml_node_struct_fini() checking.
Lines 626 to 630 in 944068f
| @@ -326,6 +332,7 @@ TEST(TestRclLifecycle, state_machine) { | |||
| // Node is null | |||
| ret = rcl_lifecycle_state_machine_fini(&state_machine, nullptr, &allocator); | |||
| EXPECT_EQ(ret, RCL_RET_ERROR); | |||
There was a problem hiding this comment.
I would expect to see << rcl_get_error_string().str here as well.
There was a problem hiding this comment.
EXPECT_EQ(ret, RCL_RET_ERROR) << rcl_get_error_string().str;
It means rcl_get_error_string().str is output if ret != RCL_RET_ERROR.
That is, ret is RCL_RET_OK. There is no error message.
So I think we need not to add <<rcl_get_error_string().str.
What do you think ?
Since |
Barry-Xu-2018
force-pushed
the
topic-fix-rcl-lifecycle-test-issue
branch
from
e5162a4
to
73de356
Compare
|
Please check again. |
Barry-Xu-2018
force-pushed
the
topic-fix-rcl-lifecycle-test-issue
branch
from
73de356
to
7718ecb
Compare
|
A couple of things: When running this, I still see the following output: Is there a chance you could address these as well? Looks like there's another But then further, when run with Granted, looking at the output, I can't really claim that all of them were introduced by this patch. But when running the same test on master I see only 284 bytes lost. That we leak memory is a problem by itself, but I would like you to verify that this patch indeed introduced another memory leak. |
About the cause of this problem, please look at #713 (comment).
Thanks for your information. |
|
In my environment, I execute
Based on above test result, memory leak is decreased obviously. |
|
@Barry-Xu-2018 thanks for verifying. I think it makes sense what you propose, to open up a separate issue for the memory leaks. As for the prints (overwriting the rcutils error), I think it makes sense to apply your changes here as well. It's best to address that issue completely here within the same PR. Also having it in a PR allows me to review it more easily. |
Thank you.
OK. I submit new commit |
| char * init_error_string = ""; | ||
| char * fini_error_string = ""; |
There was a problem hiding this comment.
consider renaming this to init_error_message and fini_error_message to stay consistent with fail_error_message.
Also I would recommend to add a bit of a docstring on top of each of the three error messages to get an idea on what's going on here.
There was a problem hiding this comment.
Also, why do we need 3 strings here?
I would assume 2 should be sufficient.
- init_error_message: The original cause or error why we land in
fail: - fini_error_message: A conglomerate of failures happening within
fail:.
| fini_error = rcl_get_error_string().str; | ||
| fini_error_string = rcutils_strdup(rcl_get_error_string().str, default_allocator); | ||
| if (fini_error_string != NULL) { | ||
| fini_error = rcl_get_error_string().str; |
There was a problem hiding this comment.
stay consistent with the logic above
| fini_error = rcl_get_error_string().str; | |
| fini_error = fini_error_string; |
There was a problem hiding this comment.
Agree.
But as above your suggestion, we will only use 2 variables.
So need not to this assignment-action.
| @@ -659,6 +660,9 @@ rcl_lifecycle_init_default_state_machine( | |||
| rcl_ret_t ret = RCL_RET_ERROR; | |||
| // Used for concatenating error messages in the fail: block. | |||
| const char * fail_error_message = ""; | |||
| char * init_error_string = ""; | |||
| char * fini_error_string = ""; | |||
| rcl_allocator_t default_allocator; | |||
There was a problem hiding this comment.
Why not initializing it directly? That would say it later on.
| rcl_allocator_t default_allocator; | |
| rcl_allocator_t default_allocator = rcl_get_default_allocator(); |
There was a problem hiding this comment.
default_allocator is only used in failure case.
I think it happens seldom.
So need not call rcl_get_default_allocator() each time.
What do you think ?
| if (init_error_string != NULL) { | ||
| fail_error_message = init_error_string; | ||
| } else { |
There was a problem hiding this comment.
I don't think this is necessary. If init_error_string is null, then so be it. Only set the fini_error_message to the strdup failure.
There was a problem hiding this comment.
If init_error_string is null, then so be it. Only set the fini_error_message to the strdup failure.
Sorry. I don't understand your comments.
Your comments describe what to do for else code.
if (init_error_string != NULL) {
fail_error_message = init_error_string;
} else {
fail_error_message = "Error while duplicating strings by rcutils_strdup() !";
}| } | ||
|
|
||
| if (rcl_lifecycle_transition_map_fini(&state_machine->transition_map, allocator) != RCL_RET_OK) { | ||
| const char * fini_error = ""; | ||
| if (rcl_error_is_set()) { | ||
| fini_error = rcl_get_error_string().str; | ||
| fini_error_string = rcutils_strdup(rcl_get_error_string().str, default_allocator); |
There was a problem hiding this comment.
So I think this should not be a strdup but rather a snprintf concatenating two error messages.
There was a problem hiding this comment.
The below code has concatenating two error messages. So not do this here.
rcl/rcl_lifecycle/src/default_state_machine.c
Lines 708 to 711 in bd6fbb3
Barry-Xu-2018
force-pushed
the
topic-fix-rcl-lifecycle-test-issue
branch
from
f6dfe65
to
cbb0132
Compare
…_lifecycle Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
Barry-Xu-2018
force-pushed
the
topic-fix-rcl-lifecycle-test-issue
branch
from
cbb0132
to
e861e2f
Compare
|
I rebase branch. |
|
|
Thank you. |
* Fix missing call fini() for lifecycle_transition and node in test_rcl_lifecycle Signed-off-by: Barry Xu <barry.xu@sony.com> * Fix error overwritten while allocator is Nullptr. Signed-off-by: Barry Xu <barry.xu@sony.com> * Optimize used variables Signed-off-by: Barry Xu <barry.xu@sony.com> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
* Fix missing call fini() for lifecycle_transition and node in test_rcl_lifecycle Signed-off-by: Barry Xu <barry.xu@sony.com> * Fix error overwritten while allocator is Nullptr. Signed-off-by: Barry Xu <barry.xu@sony.com> * Optimize used variables Signed-off-by: Barry Xu <barry.xu@sony.com> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> Co-authored-by: Barry Xu <barry.xu@sony.com>
Related issue is #713