finish doneCb before waitForResult returns #156

jschleicher · 2019-11-20T10:11:13Z

Fixes #155

~~Is indigo-devel the correct branch? Or should this be targeted to noetic to avoid breaking old use-cases?~~

include/actionlib/client/simple_action_client.h

as suggested by rhaschke in ros#156 (comment)

jschleicher · 2019-11-28T13:06:55Z

@mjcarroll Please review.
And also have a look at #150 which fixes the same problem in the python client.

mjcarroll · 2020-02-24T18:16:22Z

@jschleicher Looks good, but please retarget to noetic-devel.

Fixes ros#155

by introducing a short sleep (essentially inducing a scheduler context switch) in the done-callback

as suggested by rhaschke in ros#156 (comment)

jschleicher · 2020-02-25T11:04:47Z

@mjcarroll Thank you! I've rebased onto noetic-devel.

as in easy_tests

since the simple action server otherwise cancels parallel requests from the two python and cpp clients

to fix the corresponding CMake warning

This reverts commit 75fec60.

mjcarroll · 2020-03-03T17:24:06Z

@ros-pull-request-builder retest this please

This reverts commit 94367db.

jschleicher · 2020-03-06T10:50:12Z

@mjcarroll The tests are still unstable, which seems to be an occurrence of #119 (comment). Adding a delay time after waitForServer() does fix the tests, but we would have to introduce it into every single test case.

fujitatomoya · 2020-04-01T07:46:00Z

actionlib/test/simple_client_test.cpp

@@ -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();


i believe this should be done by waitForServer.

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

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.

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

fujitatomoya · 2020-04-01T07:47:10Z

actionlib/test/simple_client_test.cpp

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


what is this duration for?

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.

See also reproduction step 2. in #155.

fujitatomoya

LGTM

v4hn · 2020-04-23T08:31:51Z

@mjcarroll: @jschleicher continuously provided feedback on this for over four months now,
the only test that fails is not related to his patch but to a racing condition to setup a topic connection in time and it fixes a long-standing issue we have to work around in MoveIt.

It would be great to see this merged and released in noetic (and best-case also a backport to melodic, but at this point I would be happy to see this merged in any branch)!

mjcarroll · 2020-04-23T16:25:15Z

@v4hn Yes, I appreciate the iteration from @jschleicher.

At this point, I think it's acceptable to land on noetic, and I would be happy to review a backport to melodic

mjcarroll · 2020-04-23T16:25:52Z

@ros-pull-request-builder retest this please

mjcarroll · 2020-04-23T16:26:21Z

@mjcarroll The tests are still unstable, which seems to be an occurrence of #119 (comment). Adding a delay time after waitForServer() does fix the tests, but we would have to introduce it into every single test case.

@jschleicher Do you want to do this in a follow up PR?

* call doneCb before setting client state (Fixes ros#155) * add testcase to fail without the fix * also test getState() in the callback * wait until the server is ready * increase time limit for unrelated test * separate exercise_simple_client into cpp and python * increase debug output * wait for longer for server * bump CMake minimum version to use new behavior of CMP0048 * test build with more debug output * Revert "wait for longer for server" * Revert "test build with more debug output"

actionlib was fixed via ros/actionlib#156

* cleanup after actionlib fix actionlib was fixed via ros/actionlib#156 * Reset status variables before calling sendGoal() This fixes a race condition, where the execution status is set to RUNNING after the doneCallback has been called, e.g. because the controller immediately reports success.

jschleicher mentioned this pull request Nov 20, 2019

Possible Threading error during trajectory execution moveit/moveit#1493

Open

rhaschke reviewed Nov 21, 2019

View reviewed changes

include/actionlib/client/simple_action_client.h Outdated Show resolved Hide resolved

rhaschke mentioned this pull request Nov 21, 2019

Wait for done-callback moveit/moveit#1741

Closed

jschleicher changed the title ~~call doneCb before setting client state~~ WIP: call doneCb before setting client state Nov 21, 2019

jschleicher added a commit to PilzDE/actionlib that referenced this pull request Nov 28, 2019

also test getState() in the callback

ee4a663

as suggested by rhaschke in ros#156 (comment)

jschleicher changed the title ~~WIP: call doneCb before setting client state~~ finish doneCb before waitForResult returns Nov 28, 2019

jschleicher changed the base branch from indigo-devel to noetic-devel February 25, 2020 10:58

jschleicher added 3 commits February 25, 2020 12:02

call doneCb before setting client state

e836981

Fixes ros#155

add testcase to fail without the fix

0db4fe3

by introducing a short sleep (essentially inducing a scheduler context switch) in the done-callback

also test getState() in the callback

61d95ae

as suggested by rhaschke in ros#156 (comment)

jschleicher force-pushed the fix_waitForResult_doneCb branch from 17b64d8 to cf20253 Compare February 25, 2020 11:04

wait until the server is ready

5b07440

as in easy_tests

jschleicher force-pushed the fix_waitForResult_doneCb branch from cf20253 to 5b07440 Compare February 27, 2020 09:50

jschleicher added 4 commits February 27, 2020 11:34

increase time limit for unrelated test

5b6c38c

separate exercise_simple_client into cpp and python

88d6c55

since the simple action server otherwise cancels parallel requests from the two python and cpp clients

increase debug output

86ea5e4

wait for longer for server

75fec60

jschleicher force-pushed the fix_waitForResult_doneCb branch from bc7c000 to 347fb81 Compare February 27, 2020 12:43

bump CMake minimum version to use new behavior of CMP0048

56d1afc

to fix the corresponding CMake warning

jschleicher force-pushed the fix_waitForResult_doneCb branch from 347fb81 to 56d1afc Compare February 27, 2020 14:27

jschleicher added 2 commits February 28, 2020 11:44

test build with more debug output

94367db

Revert "wait for longer for server"

a48f586

This reverts commit 75fec60.

Revert "test build with more debug output"

cd2c25e

This reverts commit 94367db.

jschleicher force-pushed the fix_waitForResult_doneCb branch from bd4a33f to cd2c25e Compare March 6, 2020 10:12

jschleicher mentioned this pull request Mar 31, 2020

Avoid waiting in loops #164

Open

fujitatomoya reviewed Apr 1, 2020

View reviewed changes

fujitatomoya approved these changes Apr 14, 2020

View reviewed changes

captain-yoshi mentioned this pull request Apr 22, 2020

ControllerManager: wait for done-callback moveit/moveit#1783

Merged

mjcarroll approved these changes Apr 23, 2020

View reviewed changes

mjcarroll merged commit 9beedb8 into ros:noetic-devel Apr 23, 2020

rhaschke added a commit to ubi-agni/moveit that referenced this pull request Jul 10, 2024

cleanup after actionlib fix

f8edee1

actionlib was fixed via ros/actionlib#156

LeroyR pushed a commit to CentralLabFacilities/moveit that referenced this pull request Jul 11, 2024

cleanup after actionlib fix

62ee8a7

actionlib was fixed via ros/actionlib#156

LeroyR pushed a commit to CentralLabFacilities/moveit that referenced this pull request Jul 11, 2024

cleanup after actionlib fix

36da5ed

actionlib was fixed via ros/actionlib#156

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

finish doneCb before waitForResult returns #156

finish doneCb before waitForResult returns #156

jschleicher commented Nov 20, 2019 •

edited

Loading

jschleicher commented Nov 28, 2019

mjcarroll commented Feb 24, 2020

jschleicher commented Feb 25, 2020

mjcarroll commented Mar 3, 2020

jschleicher commented Mar 6, 2020

fujitatomoya Apr 1, 2020

jschleicher Apr 6, 2020

fujitatomoya Apr 9, 2020

jschleicher Apr 14, 2020

fujitatomoya Apr 1, 2020

jschleicher Apr 6, 2020

jschleicher Apr 6, 2020 •

edited

Loading

fujitatomoya left a comment

v4hn commented Apr 23, 2020

mjcarroll commented Apr 23, 2020

mjcarroll commented Apr 23, 2020

mjcarroll commented Apr 23, 2020

finish doneCb before waitForResult returns #156

finish doneCb before waitForResult returns #156

Conversation

jschleicher commented Nov 20, 2019 • edited Loading

jschleicher commented Nov 28, 2019

mjcarroll commented Feb 24, 2020

jschleicher commented Feb 25, 2020

mjcarroll commented Mar 3, 2020

jschleicher commented Mar 6, 2020

fujitatomoya Apr 1, 2020

Choose a reason for hiding this comment

jschleicher Apr 6, 2020

Choose a reason for hiding this comment

fujitatomoya Apr 9, 2020

Choose a reason for hiding this comment

jschleicher Apr 14, 2020

Choose a reason for hiding this comment

fujitatomoya Apr 1, 2020

Choose a reason for hiding this comment

jschleicher Apr 6, 2020

Choose a reason for hiding this comment

jschleicher Apr 6, 2020 • edited Loading

Choose a reason for hiding this comment

fujitatomoya left a comment

Choose a reason for hiding this comment

v4hn commented Apr 23, 2020

mjcarroll commented Apr 23, 2020

mjcarroll commented Apr 23, 2020

mjcarroll commented Apr 23, 2020

jschleicher commented Nov 20, 2019 •

edited

Loading

jschleicher Apr 6, 2020 •

edited

Loading