Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Increase rcl_lifecycle test coverage and add more safety checks #649

Merged
merged 7 commits into from
May 22, 2020
Merged

Increase rcl_lifecycle test coverage and add more safety checks #649

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
44dbbed
Increase test coverage and add more safety checks
brawner May 13, 2020
70465b7
More coverage
brawner May 14, 2020
152aaa3
Concatenating error messages
May 18, 2020
71607bc
PR Fixup
May 19, 2020
f269c23
Forgot a
May 20, 2020
a4961f1
PR Fixup
May 22, 2020
661b4b7
Moving var to top
May 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions rcl_lifecycle/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,16 @@ if(BUILD_TESTING)
)
target_link_libraries(test_multiple_instances ${PROJECT_NAME})
endif()
ament_add_gtest(test_rcl_lifecycle
test/test_rcl_lifecycle.cpp
)
if(TARGET test_rcl_lifecycle)
ament_target_dependencies(test_rcl_lifecycle
"rcl"
"osrf_testing_tools_cpp"
)
target_link_libraries(test_rcl_lifecycle ${PROJECT_NAME})
endif()
ament_add_gtest(test_transition_map
test/test_transition_map.cpp
)
Expand Down
23 changes: 20 additions & 3 deletions rcl_lifecycle/src/default_state_machine.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,7 +650,7 @@ _register_transitions(
return ret;
}

// default implementation as despicted on
// default implementation as depicted on
// design.ros2.org
rcl_ret_t
rcl_lifecycle_init_default_state_machine(
Expand Down Expand Up @@ -690,10 +690,27 @@ rcl_lifecycle_init_default_state_machine(

return ret;

fail:
// Semicolon handles the "a label can only be part of a statement..." error
fail:;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fail:;
fail:

Or was that semicolon intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compiler wouldn't let me declare current_error directly after fail: without it. The semicolon effectively creates a new statement. Should I move the variable declarations to the top?

// If rcl_lifecycle_transition_map_fini() fails, it will clobber the error string here, twice
// Here, we concatenate the error strings if that happens
const char * current_error = (rcl_error_is_set()) ? rcl_get_error_string().str : "";
rcl_reset_error();

if (rcl_lifecycle_transition_map_fini(&state_machine->transition_map, allocator) != RCL_RET_OK) {
RCL_SET_ERROR_MSG("could not free lifecycle transition map. Leaking memory!\n");
const char * fini_error = (rcl_error_is_set()) ? rcl_get_error_string().str : "";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I interpret this correctly this line will always result in an empty string, given that you reset the error message just above.

rcl_reset_error();

RCL_SET_ERROR_MSG_WITH_FORMAT_STRING(
"Freeing transition map failed while handling a previous error. Leaking memory!"
"\nOriginal error:\n\t%s\nError encountered in rcl_lifecycle_transition_map_fini():\n\t%s\n",
current_error, fini_error);
} else if (strcmp(current_error, "") != 0) {
RCL_SET_ERROR_MSG(current_error);
brawner marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to iterate again over this, but I think that else if branch is not needed. If I see this correctly, we'll fetch the error message before just to reset it again in this branch.
Why not just retrieving the existing error message within the first if branch?

something like this:

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;
     rcl_reset_error();
  }
  RCL_SET_ERROR_MSG_WITH_FORMAT_STRING(
  "Freeing transition map failed while handling a previous error. Leaking memory!"
  "\nOriginal error:\n\t%s\nError encountered in rcl_lifecycle_transition_map_fini():\n\t%s\n",
  current_error, fini_error);
}

if (!rcl_error_is_set()) {
  RCL_SET_ERROR_MSG("Unspecified error ..");
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem originally was that the error message was already set. But if rcl_lifecycle_transition_map_fini failed, it would set its own message, and then the message would get set a third time inside this first if loop. So the goal was to keep track of current_message, and append fini_error if it existed and throw a combined error that was hopefully more helpful than either of them individually.

However, if rcl_lifecycle_transition_map_fini succeeds, we still want the user to know what caused the code to reach fail: in the first place, which is in current_message , but only if rcl_error_is_set() was true in line 697.

Originally when I submitted this, I just relied on rcutils to log the transition of error messages, since it warns if a new error message is set while a previous one had already existed. However, since I'm pretty new to the ROS 2 C codebase, I'm really not confident about any particular approach here 🤷‍♂️

} else {
RCL_SET_ERROR_MSG("Unspecified in default_state_machine _register_transitions()");
}

return RCL_RET_ERROR;
}

Expand Down
42 changes: 34 additions & 8 deletions rcl_lifecycle/src/rcl_lifecycle.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ rcl_lifecycle_get_zero_initialized_state()
rcl_lifecycle_state_t state;
state.id = 0;
state.label = NULL;
state.valid_transitions = NULL;
state.valid_transition_size = 0;
return state;
}

Expand All @@ -58,6 +60,10 @@ rcl_lifecycle_state_init(
RCL_SET_ERROR_MSG("state pointer is null\n");
return RCL_RET_ERROR;
}
if (!label) {
RCL_SET_ERROR_MSG("State label is null\n");
return RCL_RET_ERROR;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return RCL_RET_ERROR;
return RCL_RET_INVALID_ARGUMENT;

}

state->id = id;
state->label = rcutils_strndup(label, strlen(label), *allocator);
Expand Down Expand Up @@ -118,7 +124,22 @@ rcl_lifecycle_transition_init(

if (!transition) {
RCL_SET_ERROR_MSG("transition pointer is null\n");
return RCL_RET_OK;
return RCL_RET_ERROR;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return RCL_RET_ERROR;
return RCL_RET_INVALID_ARGUMENT;

}

if (!label) {
RCL_SET_ERROR_MSG("label pointer is null\n");
return RCL_RET_ERROR;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return RCL_RET_ERROR;
return RCL_RET_INVALID_ARGUMENT;

}

if (!start) {
RCL_SET_ERROR_MSG("start state pointer is null\n");
return RCL_RET_ERROR;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return RCL_RET_ERROR;
return RCL_RET_INVALID_ARGUMENT;

}

if (!goal) {
RCL_SET_ERROR_MSG("goal state pointer is null\n");
return RCL_RET_ERROR;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return RCL_RET_ERROR;
return RCL_RET_INVALID_ARGUMENT;

}

transition->start = start;
Expand All @@ -140,7 +161,7 @@ rcl_lifecycle_transition_fini(
const rcl_allocator_t * allocator)
{
if (!allocator) {
RCL_SET_ERROR_MSG("can't initialize transition, no allocator given\n");
RCL_SET_ERROR_MSG("can't finalize transition, no allocator given\n");
return RCL_RET_ERROR;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return RCL_RET_ERROR;
return RCL_RET_INVALID_ARGUMENT;

}
// it is already NULL
Expand Down Expand Up @@ -192,6 +213,14 @@ rcl_lifecycle_state_machine_init(
bool default_states,
const rcl_allocator_t * allocator)
{
if (!state_machine) {
RCL_SET_ERROR_MSG("State machine is null\n");
return RCL_RET_ERROR;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return RCL_RET_ERROR;
return RCL_RET_INVALID_ARGUMENT;

}
if (!node_handle) {
RCL_SET_ERROR_MSG("Node handle is null\n");
return RCL_RET_ERROR;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return RCL_RET_ERROR;
return RCL_RET_INVALID_ARGUMENT;

}
if (!allocator) {
RCL_SET_ERROR_MSG("can't initialize state machine, no allocator given\n");
return RCL_RET_ERROR;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return RCL_RET_ERROR;
return RCL_RET_INVALID_ARGUMENT;

Expand All @@ -207,15 +236,12 @@ rcl_lifecycle_state_machine_init(
}

if (default_states) {
rcl_ret_t ret =
rcl_lifecycle_init_default_state_machine(state_machine, allocator);
ret = rcl_lifecycle_init_default_state_machine(state_machine, allocator);
if (ret != RCL_RET_OK) {
// init default state machine might have allocated memory,
// so we have to call fini
if (rcl_lifecycle_state_machine_fini(state_machine, node_handle, allocator) != RCL_RET_OK) {
// error already set
return RCL_RET_ERROR;
}
ret = rcl_lifecycle_state_machine_fini(state_machine, node_handle, allocator);
return RCL_RET_ERROR;
}
}

Expand Down
27 changes: 20 additions & 7 deletions rcl_lifecycle/src/transition_map.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@ rcl_lifecycle_transition_map_fini(
rcl_lifecycle_transition_map_t * transition_map,
const rcutils_allocator_t * allocator)
{
if (!allocator) {
RCL_SET_ERROR_MSG("can't free transition map, no allocator given\n");
return RCL_RET_ERROR;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return RCL_RET_ERROR;
return RCL_RET_INVALID_ARGUMENT;

}

rcl_ret_t fcn_ret = RCL_RET_OK;

// free valid transitions for all states
Expand Down Expand Up @@ -87,15 +92,16 @@ rcl_lifecycle_register_state(
allocator, "invalid allocator", return RCUTILS_RET_INVALID_ARGUMENT)

// add new primary state memory
transition_map->states_size += 1;
unsigned int new_states_size = transition_map->states_size + 1;
rcl_lifecycle_state_t * new_states = allocator->reallocate(
transition_map->states,
transition_map->states_size * sizeof(rcl_lifecycle_state_t),
new_states_size * sizeof(rcl_lifecycle_state_t),
allocator->state);
if (!new_states) {
RCL_SET_ERROR_MSG("failed to reallocate memory for new states");
return RCL_RET_ERROR;
}
transition_map->states_size = new_states_size;
transition_map->states = new_states;
transition_map->states[transition_map->states_size - 1] = state;

Expand All @@ -117,32 +123,39 @@ rcl_lifecycle_register_transition(
return RCL_RET_ERROR;
}

// we add a new transition, so increase the size
transition_map->transitions_size += 1;
rcl_lifecycle_state_t * goal = rcl_lifecycle_get_state(transition_map, transition.goal->id);
if (!goal) {
RCL_SET_ERROR_MSG_WITH_FORMAT_STRING("state %u is not registered\n", transition.goal->id);
return RCL_RET_ERROR;
}
// Attempt to add new transition, don't update map if it fails
unsigned int new_transitions_size = transition_map->transitions_size + 1;
rcl_lifecycle_transition_t * new_transitions = allocator->reallocate(
transition_map->transitions,
transition_map->transitions_size * sizeof(rcl_lifecycle_transition_t),
new_transitions_size * sizeof(rcl_lifecycle_transition_t),
allocator->state);
if (!new_transitions) {
RCL_SET_ERROR_MSG("failed to reallocate memory for new transitions");
return RCL_RET_BAD_ALLOC;
}
transition_map->transitions_size = new_transitions_size;
transition_map->transitions = new_transitions;
// finally set the new transition to the end of the array
transition_map->transitions[transition_map->transitions_size - 1] = transition;

// we have to copy the transitons here once more to the actual state
// as we can't assign only the pointer. This pointer gets invalidated whenever
// we add a new transition and re-shuffle/re-allocate new memory for it.
state->valid_transition_size += 1;
unsigned int new_valid_transitions_size = state->valid_transition_size + 1;
rcl_lifecycle_transition_t * new_valid_transitions = allocator->reallocate(
state->valid_transitions,
state->valid_transition_size * sizeof(rcl_lifecycle_transition_t),
new_valid_transitions_size * sizeof(rcl_lifecycle_transition_t),
allocator->state);
if (!new_valid_transitions) {
RCL_SET_ERROR_MSG("failed to reallocate memory for new transitions on state");
return RCL_RET_ERROR;
}
state->valid_transition_size = new_valid_transitions_size;
state->valid_transitions = new_valid_transitions;

state->valid_transitions[state->valid_transition_size - 1] = transition;
Expand Down
8 changes: 7 additions & 1 deletion rcl_lifecycle/test/test_default_state_machine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,13 @@ TEST_F(TestDefaultStateMachine, zero_init) {
TEST_F(TestDefaultStateMachine, default_init) {
rcl_lifecycle_state_machine_t state_machine = rcl_lifecycle_get_zero_initialized_state_machine();

auto ret = rcl_lifecycle_init_default_state_machine(&state_machine, this->allocator);
// Because this init method is so complex, the succession of failures caused by a null
// allocator will result in several error messages overwriting themselves.
auto ret = rcl_lifecycle_init_default_state_machine(&state_machine, nullptr);
EXPECT_EQ(RCL_RET_ERROR, ret);
rcutils_reset_error();

ret = rcl_lifecycle_init_default_state_machine(&state_machine, this->allocator);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;

ret = rcl_lifecycle_state_machine_fini(&state_machine, this->node_ptr, this->allocator);
Expand Down
Loading