Updating client API to be able to remove pending requests

[ivanpauno]

Fixes #1697.

This modifies a few things in the client API:

• Modifies the async_send_request() methods that take a callback as a second argument to not return a future anymore.
  Though this breaks API, nobody was actually using that in the ROS 2 core.
  If someone wants to have both a callback and a future, they can capture the future in a lambda and set it.
  • Alternative: create a SharedFutureAndRequestID class, similar to the FutureAndRequestID class introduced here.
  • I'm planning to deprecate the current callback signatures that accept a Future, and instead have the two listed below.
    That wasn't done here to avoid having to create simultaneous PRs in other repos.
    • void (SharedResponse)
    • void (SharedRequest, SharedResponse)
• Modifies the async_send_request() methods that take only one argument to return a FutureAndRequestID.
  • This is a future like class.
    Though it breaks ABI, it has some implicit conversions to std::future to not break API.
    The implicit conversion by value was only added to not break existing code and generates a deprecation warning.
• Adds a remove_pending_request() method, the return value of any of the async_send_request() overloads can be passed to it.

Examples:

auto future = client->async_send_request(request);
auto result = executor->spin_until_complete(future, timeout);
if (result == timeout) {
  client->remove_request(future);
  // handle timeout
}

When using callbacks, you need to combine the client with a Timer to prune the old ones:

  // this example supposes you're only waiting for one response at a time
  //
  // setup timer
  this->timer = this->create_wall_timer(prune_old_requests, [this] () {
    if (this->client->remove_request(this->last_request_id)) {
      // handle timeout
    }
  };
  this->timer.cancel();

  // when sending a request
  this->last_request_id = client->send_async_request(request, [this] (SharedResponse response) {
    this->handle_response(response);
    this->timer.cancel();
  }
  this->timer.reset();

[ivanpauno]

When using callbacks, you need to combine the client with a Timer to prune the old ones:

I'm planning to write a more complete example of how to do this, but I would like first to get some early feedback.

[ivanpauno]

I think I actually don't need this two custom implicit conversions, but actually one to SharedFuture to avoid breaking pre-existing code.
I'm taking a look locally.

[ivanpauno]

I think we can make this two members public, this is simply a pair there's no invariant here.

The future like methods were added to avoid breaking pre-existing code.
It's not a bad idea to deprecate all of them though, that way we don't break pre-existing code but users get a warning and they take a look to remove_pending_request().

[ivanpauno]

Though this breaks API, nobody was actually using that in the ROS 2 core.

I finally found some tests in test_rclcpp that were using that.
Though I still think it's weird, you typically either want a callback or a future, not both.

If someone wants to have both a callback and a future, they can capture the future in a lambda and set it.

Note: this isn't that easy.
The issue is that std::function is a wrapper over a copyable callable.
A lambda that captures a promise is not copyable, so you cannot pass that to async_send_request().
To solve that you need to wrap the promise to something that's actually copyable (e.g. a shared_ptr, a wrapper that actually moves on copy (which might be problematic), etc).
We could also be delting that requirement from our api by having a rcpputils::unique_function{} implementation.

Alternative: create a SharedFutureAndRequestID class, similar to the FutureAndRequestID class introduced here.

If anyone feels that's a better solution than breaking this use case, I can update the PR.

[jacobperron]

Though I still think it's weird, you typically either want a callback or a future, not both.

Is there a technical reason for not providing both, or is it just aesthetic? We could still maintain API stability and document that the user can (or should) use the returned future to prune the request if there's a timeout.

---

The approach looks okay to me. Have you thought about a way to handle pruning requests internally (so the user doesn't have to worry about it)? Alternatively, have you consider something like this: #1697 (comment) ?

[ivanpauno]

Is there a technical reason for not providing both, or is it just aesthetic?

It's mainly the second, but it's also performance.
Why to set a promise value that will be ignored by most users?

We could still maintain API stability and document that the user can (or should) use the returned future to prune the request if there's a timeout.

We cannot remove the pending request with the future, that's why I created the FutureAndRequestID class.
But yes, I can create a SharedFutureAndRequestID to keep backwards compatibility.

That wasn't done here to avoid having to create simultaneous PRs in other repos.
void (SharedResponse)
void (SharedRequest, SharedResponse)

If we later add this, we could in that case only return the request id and not a future.
I'm having to set a promise either way, as the current callback signature is taking a SharedResponse, so I will go ahead with the SharedFutureAndRequestId solution.

The approach looks okay to me. Have you thought about a way to handle pruning requests internally (so the user doesn't have to worry about it)?

Yes but that doesn't fit well with the async_send_request API.
My idea is basically this #1697 (comment).
We have a timer for each pending request, and we cancel() the timer when we get a response.
It involves using more resources though (creating timers and adding them to the wait set / executor).
I will explore this solution a bit more.

Alternatively, have you consider something like this: #1697 (comment) ?

Yes, I think that solution is interesting.
The issue is that we basically don't need the internal pending_requests_ map in that case, and the behavior to handle a response is different.
As we're not storing the pending requests internally, we can only have one callback to handle all responses (not one per request send).
That means, we need a different class implementing the ClientBase interface.
I could add a class like that, but I preferred to first extend the current API.

---

Long term, I would rather have some simpler wrappers of the rcl handles that don't know anything about callbacks at all, and then on top of that we could build a Condition (similar as waitable) and a task concept.

Subscription sub{node, topic_name, sub_opts};
Client cli{node, service_names, cli_opts};

executor.on_condition(sub.has_message_condition(), [&sub](TaskHandle handle) {
  // take message and do something with it
  // the TaskHandle can be used to e.g. cancel this task
  // or maybe more powerful things, like swapping the condition the task is waiting on.
});
client.send_request(request);
auto complete_token = executor.on_condition_once(
  cli.has_response_condition(),
  [&cli](TaskHandle handle) {
    // take and handle response here
  });
executor.spin_until_complete(complete_token, timeout);

Don't look into the details of the snippet above, but an approach like that has some benefits:

• It separates concerns better.
• You don't need to add everything to the executor.
  e.g. if a client is not waiting on a response, no need to add it to the internal wait set.
  In the same fashion, if you don't need to take data from a subscription for some time, you can avoid to have a task waiting on it.
• Maybe: can handle multithreaded approach without the need of callback groups.

[ivanpauno]

I clicked the merge button in the wrong window 🤦‍♂️.
I've reverted in #1733, I will open a new PR replacing this one.