Fix callback group logic in executor #2476
Conversation
There was a problem hiding this comment.
@mjcarroll do we need to apply the same fix for, (where is irrelevant from #2142)
callback_group is null means that entity is not valid. AFAIS, rclcpp does not use ready_executables but user would?
CC: @jmachowinski
Should be straight forward. |
mjcarroll
force-pushed
the
mjcarroll/fix_cbg_condition
branch
from
7a009e0
to
16911a8
Compare
| @@ -0,0 +1,132 @@ | |||
| // Copyright 2017 Open Source Robotics Foundation, Inc. | |||
|
@jmachowinski can you take a look at my test here? I can't get a scenario where the executable (in this case a timer) is valid, while the callback group is not. In my case, even when resetting the callback group and the node, there are still 2 strong references to the callback group. |
mjcarroll
force-pushed
the
mjcarroll/fix_cbg_condition
branch
from
16911a8
to
b4f61ec
Compare
There was a problem hiding this comment.
changes lgtm, had a question about the tests
Also a general comment on the test, I'd love to see the use of spin() instead of spin_all(), or even spin_once() in a loop. And I've found that using a custom waitable that can be explicitly triggered rather than a timer can avoid unintentional race conditions and sleeps. Though we've been copying that custom waitable around a lot. It might be a good idea to create a new entity type that is like a TriggeredCallback class which would inherit from Waitable and would differ from a GuardCondition in that it has a callback to be called when the executor executes it, rather than when it is triggered (which the GuardCondition class already has). It would definitely be useful in tests and may even be useful for users, allowing them to manually schedule a callback to be run in the executor on demand.
If others agree I could work on that.
| // Add the callback group to the executor | ||
| auto executor = CustomExecutor(); | ||
| executor.add_node(node); | ||
| executor.spin_all(std::chrono::milliseconds(10)); |
There was a problem hiding this comment.
Just curious why is this needed?
There was a problem hiding this comment.
This was to clear out all of the node startup transients.
In this special case, the timer is not racy, as we just need it to be ready. Also I would like to keep the timer, as it uncovered a bug causing an endless loop. |
rclcpp/test/rclcpp/executors/test_executors_callback_group_behavior.cpp
Outdated
Show resolved
Hide resolved
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai> Co-authored-by: Janosch Machowinski <j.machowinski@nospam.org>
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai> Co-authored-by: Janosch Machowinski <j.machowinski@nospam.org>
mjcarroll
force-pushed
the
mjcarroll/fix_cbg_condition
branch
from
b4f61ec
to
06f99cc
Compare
|
@jmachowinski I have incorporated your changes from: https://github.com/cellumation/rclcpp/tree/fix_cbg_condition They have been squashed and I marekd you with co-authored-by
|
|
@mjcarroll I picked the test over to #2494 2494 also fixes the tests fails that came up in the CI. I think we can close this PR in favor for 2494 |
|
Ok, let's go ahead and close this then. We can reopen it if needed, and I think the feed back was addressed in the new pr as far as I can tell. For future reference, it's usually better to make prs to other prs rather than replacing them, though sometimes that doesn't work out due to needing to rebase. Or like me you mess up the pr or branch some how and have to close it 🙃. |
Fixes #2474
CC: @jmachowinski