Rclcpp: Executor::spin_some() should check for work only once

[wjwwood]

I believe spin_some behaves contrary to how most people would assume. I believe most people assume that if there are three things to do when you call spin_some() only three things will be done then it would return. Even if new work items come up during that execution.

However, right now spin_some() will check if there is work to do after each work item is completed. This means that if you have more work coming in than you can execute, spin_some() will never exit because there is always something to do.

This could be fixed by "claiming" everything there is to do when spin_some() starts, then executing each of them and then exit. Basically only checking for work one time, at the beginning of the method.

[wjwwood]

This was previous discussed in a related, but ultimately different, issue:

https://github.com/ros2/rclcpp/issues/280#issuecomment-289627771

[tj10200]

Hey, I'm pretty new to working on github projects. I'd like to take a shot at fixing this issue. Can you give me permission to create branches so I can submit a pull request? Or what is the process there?

[dhood]

Hi @tj10200, rather than giving contributors write access, we ask them to make a pull request from a fork (see https://help.github.com/articles/fork-a-repo/). Let us know if you get stuck at a particular point.

In terms of general contribution workflow, if you'd like to work on this issue you can claim it (as you have done), and comment here for general discussion on the direction of the work, and then once you open a PR we can review the code details.

[tj10200]

Thanks @dhood, I appreciate the help. I submitted a pull request with a minor change on the loop, but maybe I got ahead of myself a bit. Is there a README on how to get the project building. Specifically how to get ament_cmake_ros and ament_cmake projects integrated so the CMakeLists can find them?

[wjwwood]

@tj10200 there are quite a few dependencies for rclcpp, I'd recommend follow our ROS 2 development setup here:

https://github.com/ros2/ros2/wiki/Installation#building-from-source (use one of the ones from the "Building from Source" list)

Then you'll be able to build and test only rclcpp using the --only rclcpp argument to our build tool (ament.py build). There's also these references:

• https://github.com/ros2/ros2/wiki/Ament-Tutorial#run-the-build
• https://github.com/ros2/ros2/wiki/Ament-Tutorial#run-the-tests
• https://github.com/ros2/ros2/wiki/Contributing#contributing-code

[tj10200]

Hi, @wjwwood. I've gone through a few iterations of trying to get the code to build using the guides you provided and keep getting stuck on this compilation error in the rclcpp project:

service.hpp:128:71: error: there are no arguments to ‘rcl_node_get_logger_name’ that depend on a template parameter, so a declaration of ‘rcl_node_get_logger_name’ must be available [-fpermissive]
rclcpp::get_logger(rcl_node_get_logger_name(handle.get())).get_child("rclcpp")

I can't find any definition of rcl_node_get_logger_name so I assume this must be some environmental thing I am missing.

I am on a Ubuntu 16.04 desktop VM. The guide said this was the supported version.
gcc 5.4.0
cmake 3.5.1

Any thoughts?

[mikaelarguedas]

rcl_get_logger_name has been added after the Ardent release, so my guess is that you;re using an old version of the code.

Can you try to build the entire stack from source following these instructions ?
In the step "Get ROS 2.0 code", make sure to get the ros2.repos file from the master branch, i.e:
replace release-latest with masterin the URL.
before: wget https://raw.githubusercontent.com/ros2/ros2/release-latest/ros2.repos
after: wget https://raw.githubusercontent.com/ros2/ros2/master/ros2.repos

HTH

[tj10200]

Thanks all, I got the code building. It was a combination of not using master and the rviz directory not building on my VM which led to missing cmake dependencies when I went back and tried to use --only rclcpp.

[oswinso]

Is this still in progress? I'd like to take this up if nobody is working on this right now.

[oswinso]

@wjwwood I'm having some difficulties with this issue.
I tried to make with both the publisher and subscriber on the same node. However, what I found was that with a Mutually Exclusive group (the default option), the first AnyExecutable from get_next_executable sets can_be_take_from to false, which prevents the second executable from also being taken. This results in only the callback happening for the first message.

I then tried to set can_be_taken_from to true so that I could take multiple from the same Mutually Exclusive group, since they would still be executed one by one. However, this resulted in get_next_executable returning true forever, probably due to returning the same AnyExecutable, and I'm not sure exactly where the AnyExecutables are being "claimed".

Do you have any tips on how I should approach this? Having only just started on this codebase, I don't have enough knowledge about the entire stack for ROS2, such as the details of how remove_null_handles works in regards to whether there is a message or not in a Subscriber's queue. Thanks!

[wjwwood]

I don't think you need to change the callback groups, instead I'd recommend making a new function, maybe get_ready_executables which first collects all AnyExecutable, then updates callback group's can_be_taken_from.

The remove_null_handles does just that, it takes a list of subscription handles which may or may not each be null, and removes the null ones, leaving just the non-null ones. The non-null ones are ready to be taken from (at least once).

When waiting (rcl_wait()) you give lists of handles, e.g. subscription handles, and when one or more things are "ready" then it null's out any that are not ready from the input list. That's how you know what's ready and what's not.

[oswinso]

Hey @wjwwood. I'm still a bit confused about how the process of "claiming" the AnyExecutable works.

My understanding is that the "claiming" is only done during the execution when they are taken from, and so there is no way of telling how many messages I can take from a particular subscription when I am trying to collect the AnyExecutables.

Right now, since each AnyExecutable represents one subscription handle which could have many messages, when I try and collect AnyExecutables I only am able to collect the one subscription_handle, and I'm can't tell how many messages there are in that subscription handle, so I am only able to handle the first message of the subscription handle even if there may be many messages.

[roger-strain]

Starting to look into ROS2 stuff, and figured it would be good to look at some existing issues to get a feel for what still needs to be done. Not trying to steal anything away from others who are working it, just investigating. My investigation made me wonder whether this particular issue is still relevant, given the changes introduced in #558, which added a max_duration to spin_some in order to prevent the behavior of continuing to run if things keep getting added to the various queues.

[jacobperron]

558 can be used as a workaround, but think the issue still stands given that spin_some will continue to add more work as long as the max duration hasn't been exceeded. At minimum, I think the documentation could be made more clear about the behavior of spin_some.

[wjwwood]

As @jacobperron said, for this to be really resolved, we would want spin some to check for work when it starts, execute that work, and then exit. But that's more complicated than it sounds.

Either way, I'd say this is still an issue because it will basically never exit before the max spin time is exceeded when there's periodic work (timers and in many cases subscriptions) that takes a long time.

[ivanpauno]

Solved by https://github.com/ros2/rclcpp/pull/844, closing!