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

finish doneCb before waitForResult returns #156

Merged
merged 12 commits into from
Apr 23, 2020
Merged

finish doneCb before waitForResult returns #156

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e836981
call doneCb before setting client state
jschleicher Nov 20, 2019
0db4fe3
add testcase to fail without the fix
jschleicher Nov 28, 2019
61d95ae
also test getState() in the callback
jschleicher Nov 28, 2019
5b07440
wait until the server is ready
jschleicher Nov 28, 2019
5b6c38c
increase time limit for unrelated test
jschleicher Feb 27, 2020
88d6c55
separate exercise_simple_client into cpp and python
jschleicher Feb 27, 2020
86ea5e4
increase debug output
jschleicher Feb 27, 2020
75fec60
wait for longer for server
jschleicher Feb 27, 2020
56d1afc
bump CMake minimum version to use new behavior of CMP0048
jschleicher Feb 27, 2020
94367db
test build with more debug output
jschleicher Feb 28, 2020
a48f586
Revert "wait for longer for server"
jschleicher Feb 28, 2020
cd2c25e
Revert "test build with more debug output"
jschleicher Mar 6, 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
2 changes: 1 addition & 1 deletion actionlib/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
cmake_minimum_required(VERSION 2.8.3)
cmake_minimum_required(VERSION 3.0.2)
project(actionlib)

find_package(catkin REQUIRED COMPONENTS actionlib_msgs message_generation roscpp std_msgs)
Expand Down
8 changes: 4 additions & 4 deletions actionlib/include/actionlib/client/simple_action_client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,15 +535,15 @@ void SimpleActionClient<ActionSpec>::handleTransition(GoalHandleT gh)
switch (cur_simple_state_.state_) {
case SimpleGoalState::PENDING:
case SimpleGoalState::ACTIVE:
if (done_cb_) {
done_cb_(getState(), gh.getResult());
}

{
boost::mutex::scoped_lock lock(done_mutex_);
setSimpleState(SimpleGoalState::DONE);
}

if (done_cb_) {
done_cb_(getState(), gh.getResult());
}

done_condition_.notify_all();
break;
case SimpleGoalState::DONE:
Expand Down
3 changes: 2 additions & 1 deletion actionlib/test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ add_rostest(test_python_server.launch)
add_rostest(test_python_server2.launch)
add_rostest(test_python_server3.launch)
add_rostest(test_python_simple_server.launch)
add_rostest(test_exercise_simple_clients.launch)
add_rostest(test_cpp_exercise_simple_client.launch)
add_rostest(test_python_exercise_simple_client.launch)
add_rostest(test_simple_action_server_deadlock_python.launch)

catkin_add_gtest(actionlib-destruction_guard_test destruction_guard_test.cpp)
Expand Down
21 changes: 9 additions & 12 deletions actionlib/test/simple_client_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,23 +79,17 @@ TEST(SimpleClient, easy_tests) {
}


void easyDoneCallback(bool * called, const SimpleClientGoalState & state,
void easyDoneCallback(bool * called, SimpleActionClient<TestAction> * ac, const SimpleClientGoalState & state,
const TestResultConstPtr &)
{
*called = true;
EXPECT_TRUE(ac->getState() == SimpleClientGoalState::SUCCEEDED)
<< "Expected [SUCCEEDED], but getState() returned [" << ac->getState().toString() << "]";
EXPECT_TRUE(state == SimpleClientGoalState::SUCCEEDED)
<< "Expected [SUCCEEDED], but goal state is [" << state.toString() << "]";
}

void easyOldDoneCallback(bool * called, const TerminalState & terminal_state,
const TestResultConstPtr &)
{
ros::Duration(0.1).sleep();
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this duration for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a callback, that takes some time and gives execution back to the main thread in the mean time. Without the fix, this test case easy_callback fails.

Copy link
Contributor Author

@jschleicher jschleicher Apr 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

See also reproduction step 2. in #155.

*called = true;
EXPECT_TRUE(terminal_state == TerminalState::SUCCEEDED)
<< "Expected [SUCCEEDED], but terminal state is [" << terminal_state.toString() << "]";
}

/* Intermittent failures #5087
TEST(SimpleClient, easy_callback)
{
ros::NodeHandle n;
Expand All @@ -104,18 +98,21 @@ TEST(SimpleClient, easy_callback)
bool started = client.waitForServer(ros::Duration(10.0));
ASSERT_TRUE(started);

// sleep a bit to make sure that all topics are properly connected to the server.
ros::Duration(0.01).sleep();
Copy link
Contributor

Choose a reason for hiding this comment

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

i believe this should be done by waitForServer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, it should. But actually it isn't, see my comment above: #156 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

i understand this is just a work-around to pass the test, but even with this duration SimpleClientFixture fails with server state is still ACTIVE (expected to be SUCCESS). so I expect this additional duration is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the unrelated test file exercise_simple_client.cpp that fails - adding another sleep over there would help.

[actionlib.rosunit-test_cpp_simple_client/just_succeed][FAILURE]
 /tmp/ws/src/actionlib/actionlib/test/exercise_simple_client.cpp:72


TestGoal goal;
bool finished;

bool called = false;
goal.goal = 1;
SimpleActionClient<TestAction>::SimpleDoneCallback func = boost::bind(&easyDoneCallback, &called, _1, _2);
SimpleActionClient<TestAction>::SimpleDoneCallback func = boost::bind(&easyDoneCallback, &called, &client, _1, _2);
client.sendGoal(goal, func);
finished = client.waitForResult(ros::Duration(10.0));
ASSERT_TRUE(finished);
EXPECT_TRUE(called) << "easyDoneCallback() was never called" ;
}
*/

void spinThread()
{
ros::NodeHandle nh;
Expand Down
9 changes: 9 additions & 0 deletions actionlib/test/test_cpp_exercise_simple_client.launch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<launch>

<node name="mock_simple_server"
pkg="actionlib" type="mock_simple_server.py" output="screen" />

<test test-name="test_cpp_simple_client" time-limit="90"
pkg="actionlib" type="actionlib-exercise_simple_client" />

</launch>
3 changes: 0 additions & 3 deletions .../test/test_exercise_simple_clients.launch → ...test_python_exercise_simple_client.launch
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,4 @@
<test test-name="test_py_simple_client"
pkg="actionlib" type="exercise_simple_client.py" />

<test test-name="test_cpp_simple_client"
pkg="actionlib" type="actionlib-exercise_simple_client" />

</launch>
2 changes: 1 addition & 1 deletion actionlib_tools/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
cmake_minimum_required(VERSION 2.8.3)
cmake_minimum_required(VERSION 3.0.2)
project(actionlib_tools)

find_package(catkin REQUIRED)
Expand Down