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.

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?

@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.

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.

I agree that a return code makes sense.

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

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.

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

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.

+1