Return result of future from spin_node_until_future_complete

[jacquelinekay]

This is a proposed API change to make spin_until_future_complete less prone to blocking errors.

The previous behavior of the function was to block until the future was complete or if rclcpp was interrupt (i.e. by ctrl-C), and return the future object it was waiting on when complete.

This led to ros2/rmw_connext#97, where the user could not ctrl-C out of the client//service example because accessing the future object caused the thread to block. However, after a ctrl-C the future would never complete because the executor stopped executing.

Instead of returning a shared_future object, the new behavior is to return a return code indicating SUCCESS, INTERRUPTED, or TIMEOUT. If SUCCESS, the future is safe to access without blocking. Also, this PR implements a timeout for the function.

This PR also checks the future before calling a potentially blocking spin function, to prevent indefinite blocking on rmw_wait in case a response already came in before spin_node_until_future_complete was called.

[esteve]

Not so sure about changing the API to return a copy of the future result, instead of the future itself. This would fix the issue too:

{
  // Check the future before entering the while loop.
  // If the future is already complete, don't try to spin.
  std::future_status status = future.wait_for(std::chrono::seconds(0));

  while (rclcpp::utilities::ok()) {
    if (status == std::future_status::ready) {
      return future;
    }
    executor.spin_node_once(node_ptr);
    status = future.wait_for(std::chrono::seconds(0));
  }
  // Raise exception
  std::runtime_error("Ctrl-C pressed or an error");
}

this way we can still copy the future around, but still address the Ctrl-C signal. What do you think?

We should add support for timeouts too at some point.

[jacquelinekay]

Hmm, are there other places where we throw an exception if rclcpp::ok() is false? In the other spin functions it's simply used as a signal to exit from the function. I'm hesitant to do it because there doesn't seem to be a precedent for it. Receiving the interrupt signal doesn't feel like unexpected behavior to me, but I guess it usually means the program is going to end anyway...

Can we get a third opinion on this? @wjwwood maybe?

[esteve]

@jacquelinekay I don't think there's another place where a spin function throws an exception. However extending this to add support for timeouts, we'd have to notify call points whether the spin function returned because:

• the future is valid
• the timeout was reached
• the Ctrl-C signal was triggered

via either an exception or an object that represents the status, so returning the result of the future wouldn't be feasible.

[jacquelinekay]

Hmm, if we end up introducing timeouts then I think the status code option is the best one, so that we can differentiate between the three different completion states.

[tfoote]

I agree that a return code makes sense.

[jacquelinekay]

Alright, I'll throw this back In Progress until I've prototyped a new return type for this function.

[jacquelinekay]

Ok, I prototyped a "FutureReturnCode" enum, changed the function to return that type, and implemented the timeout argument. I wasn't exactly sure how to modify the parameter code for this new change; I throw exceptions in most cases where spin_until_future_complete does not return a success.

[esteve]

+1, it'd great to add tests for this.

[jacquelinekay]

Jenkins looks fine:

http://ci.ros2.org/job/ros2_batch_ci_linux/343/
http://ci.ros2.org/job/ros2_batch_ci_osx/262/
http://ci.ros2.org/job/ros2_batch_ci_windows/396/

Unless there are objections, I'm going to squash, merge, and make an issue for adding tests.

[esteve]

+1