Add support for spin_until_complete (take 2)

[christophebedard]

Reverts #1956, so it un-reverts #1874 (with some major updates), see #1874 (comment)

Replaces #1957

Closes #1821

This adds spin_until_complete(condition, timeout).

This also adds rclcpp::Executor::spin_for(duration). Original Issue: #1821 (Added by @fujitatomoya)

This goes along with the following PRs:

• Add support for spin_until_complete rclpy#1268
• Add new executor spin_until_complete API ros2_documentation#4252

[wjwwood]

I'm sorry to just now be commenting on this change (even though it's the second iteration), but I wanted to revoice some concerns about the way that the change is being made.

[jmachowinski]

This implementation is racy. If the condition switches after this line of code, and an infinite timeout is given, we got a dead lock.

[christophebedard]

I think that's on the user since they provide the condition, but I can document it.

[jmachowinski]

Currently the function is named spin_until_complete. Therefore I would expect, I can pass it a std::future or any condition, and it will do the correct thing, and protect me from this kind of races.

This is clearly not the case. I think it is not even possible with the current API.
To solve this, we would also need to pass a std::unique_lock, like done here:
https://en.cppreference.com/w/cpp/thread/condition_variable/wait

[jmachowinski]

Hm, as a second option, one could limit the timeout of spin_once_impl to a third optional argument 'condition_recheck_interval', so that we get a bounded wait time.

[christophebedard]

Wait, I don't see how this can cause an unwanted infinite wait/spin.

It is assumed that condition() is initially false. If it is true from the get-go, then it just returns without spinning. Otherwise, it spins once before checking if condition() is true, otherwise it spins once again, and returns if it times out. If condition() never changes to true and timeout is negative, then yes it will spin infinitely, but that's intentional.

Am I missing something?

[jmachowinski]

@wjwwood to quote your here :
"The thing is that if we merge this just because Monday is coming up, then we have to come back and undo what we merged through a deprecation cycle, which is very undesirable."

I still haven't seen any example of how this API would be used in a real word application.
@SteveMacenski you want this API for "Make the APIs easier to use / understand without exposing the technical details under the hood for the Jr Eng / researcher / student that shouldn't be required to learn the details, and"

The currently proposed API does not reach this goal, as you need to be aware that you need to wake the wait, and to do this, you need to either have a known side effect of another API (which can change by any internal implementation change) or explicit manage a guard condition.

[wjwwood]

That is true, and I stand by what I said, we don't want to merge things that we think will be obsolete immediately.

However, I believe that the situation where you're pulling my quote from (#2466 (comment)) and this one are quite different. First, it's not clear to me that what you've proposed here would actually replace the desire for the new API in this pr, so we may or may not deprecate what this pr adds even with the approach you suggested implemented and merged later. Whereas in the other situation, it's more of an A or B style design question. If we choose A now, we'll have a mess switching direction to B in the future, and it would never make sense to have both. Therefore it's more important to act when we're sure it's the right approach, unlike here, where adding a new function is by comparison low risk.

[jmachowinski]

I still believe, that the currently proposed API is a to big pitfall for end users.
E.g.

spin_until_complete([end = std::chrono::system_clock::now() + std::duration::Seconds(1)] ()
{
return end < std::chrono::system_clock::now();
});

will not work as expected. I also don't really see how the API will make the live of a user easier in its current state.

I also proposed

spin_until_complete(
    const std::function<bool(void)> & condition,
    std::chrono::duration<TimeRepT, TimeT> timeout = std::chrono::duration<TimeRepT, TimeT>(-1),
     std::chrono::duration<TimeRepT, TimeT> condition_check_interval = std::chrono::millisecons(10))

this approach, which is trivial to implements, and would make the situation way better.

[wjwwood]

I also don't really see how the API will make the live of a user easier in its current state.

I also don't think the callback approach is really that helpful (but I think it can result in less code), for that you should talk with @SteveMacenski. But I still think it's no better or worse than using futures.

this approach, which is trivial to implements, and would make the situation way better.

Having the check interval (essentially polling) is bandaid, really the condition should always be attached to some activity that causes the spin to interrupt in an event driven way. If you want something like the check interval, you can just use the timeout argument and loop the function, or you could use an empty timer, etc. etc.

Most of the time this is used to wait for a Service Client to receive a response to its request, or something similar, and for that this function will work fine, and in fact the future-based version of this worked fine.

---

@christophebedard @fujitatomoya can you guys comment on whether or not to delay this until after the release or not?

[christophebedard]

I also don't really see how the API will make the live of a user easier in its current state.

I also don't think the callback approach is really that helpful (but I think it can result in less code), for that you should talk with @SteveMacenski. But I still think it's no better or worse than using futures.

Just to be clear, I picked up this set of PRs mainly to finally push them across the finish line. I personally do not have a need for this API in itself, so I'd also like to hear what Steve (or any other potential user) has to say here.

@christophebedard @fujitatomoya can you guys comment on whether or not to delay this until after the release or not?

At this point I'd prefer to just push it to make sure to get it right.

[wjwwood]

I had a few questions/suggestions/comments, but generally the approach lgtm. I think once you've addressed them to your satisfaction @christophebedard, then it should be good to go.

[wjwwood]

lgtm, now that we've resolved the discussion about the API for now.

[wjwwood]

CI:

• Linux
• Linux-aarch64
• Windows

[christophebedard]

Should we merge this now, or wait until the rclpy PR (ros2/rclpy#1268) has been reviewed and is ready to go too?

[wjwwood]

We should really merge them together, because I don't think it's good if we have rclpy and rclcpp deviate from one another.

[wjwwood]

My CI above did not include your changes for rclpy unfortunately. We'll have to run separate CI for that.

[roncapat]

Any news on this?