Pass goal handle to goal response callback instead of a future

[jacobperron]

This resolves an issue where promise->set_value is called before a potential call to promise->set_exception.
If there is an issue sending a result request, set the exception on the future to the result in the goal handle, instead of the future to the goal handle itself.

Alternative to #1292.

[brawner]

This seems reasonable to me. I'll let @hidmic have the final approval though.

[hidmic]

@jacobperron what if an exception of a different type gets thrown?

[jacobperron]

Correct me if I'm wrong, but I think this is the only type that can be thrown, here.

[hidmic]

@jacobperron hmm, my only concern here is that the caller won't know that the result request went wrong till much later. It also means a caller can't cancel the goal just sent.

---

What if we just eat up the exception? It's not ideal, but ClientGoalHandle::is_result_aware() can still return false and ClientGoalHandle::async_get_result() (which will eventually be called anyways) can throw.

[jacobperron]

Perhaps it's not as ergonomic, but the caller can still immediately check the future returned by Client::async_get_result, and any error that occurred when sending the result request should be visible.

It also means a caller can't cancel the goal just sent.

This is a valid concern. I think what you're proposing is to remove the erasure logic below so that the user can still interact with the goal handle (even though it failed to get the result). I think calling invalidate() still makes sense so that calling Client::async_get_result still produces an exception.

[jacobperron]

The other option I hadn't considered was to not handle the exception at all. This way we leave it up to the caller how to handle the exception (and they know something went wrong right away).

[hidmic]

Right. If ClientGoalHandle::make_result_aware fails, the goal handle will not be result aware. Any attempt to get the future will result in an exception that the user can handle.

[jacobperron]

Take a look at 81239f1

As it is currently implemented, the user will get the exception set in the future (because we set result awareness to true above, before sending a result request).

But, I think it makes sense to throw the error as earlier if possible, so I've make a small change make this happen: d444e09

[jacobperron]

I don't think there's anything else to do 🤔 If a user attempts to request the result I'm not actually sure what would happen, but I guess it shouldn't be allowed considering the result future would have already been set to an exception. In retrospect, I think it keeps the logic simpler to let the exception live in the future (1176dad).
Currently, the user is not able to request the result more than once (even if it is successful).

[hidmic]

Hmm, I see. We're not dealing with exceptions correctly e.g. depending on where Client::async_get_result() throws, the handle may be tagged as result aware (banning future result requests) or not.

In my mind, a user that explicitly makes a goal handle result aware should get an exception if something goes wrong, and the goal handle should remain result unaware. That is, Client::async_get_result() should offer strong exception guarantees. It shouldn't be different for a user that implicitly makes a goal handle result aware by passing in a result callback to Client::async_send_goal(). If we don't make the latter throw (and there're valid arguments against), by keeping the goal handle result unaware we allow the user to check ClientGoalHandle::is_result_aware() or handle the exception that ClientGoalHandle::async_get_result() will throw. Does that make sense?

To invalidate the goal handle improves the situation quite a bit, but it delays error generation (until future::get, when it could show itself before spinning), it induces an spurious status change (to UNKNOWN until another status message comes along), and it prevents any retries on an otherwise unaware goal handle.

But I won't block you. CI's green and this is a fairly unusual case :)

[jacobperron]

Yeah, that makes sense to me. I've made changes so that in the event that a result request fails, ClientGoalHandle::is_result_aware() should return false and Client::async_get_result() raises an exception (9cb1776).

I couldn't find a nice way to do it with the existing state information, so I've introduced a new member to ClientGoalHandle for checking if invalidate() was called, which doubles as an improved error message.

PTAL

[hidmic]

Neat! Two questions. Could a goal handle be invalidated twice? Should we do something about the spurious UNKNOWN status on invalidation? Anyhow, LGTM.

[jacobperron]

Good questions. I think we certainly want to guard against multiple calls invalidate (see b145c73). As an aside, I wish it was easier to test this stuff 🙃 It might be worth revisiting the architecture to make validation easier.

I'm not sure what to do about the UNKNOWN status. We could remove setting it when the goal is invalidated. Really I think the status should be reporting the latest state from the action server. Since this is keeping the same behavior (ie. when the client is destroyed), I'll defer deciding to remove the unknown status to another PR.

[brawner]

Here are the builds from #1290. I'm still trying to figure out the Windows issue (if it pops up)

• Linux
• Linux-aarch64
• macOS
• Windows

[jacobperron]

Note to self: examples, demos, and other packages that register a goal response callback need to be updated.
I'll spend a little time looking at solutions for maintaining, but deprecated, the old signature.

[jacobperron]

Here's of list of downstream changes required by this change:

• Update goal response callback signature demos#463
• Update goal response callback signature examples#291
• Update goal response callback signature geometry2#323
• Update goal response callback signature ros/ros_tutorials#100

I couldn't find a nice way to deprecate the old signature. Suggestions welcome that would make the change easier on users.

[jacobperron]

Err, I may have merged too early. The connected PRs need approvals too 😨

[ivanpauno]

New build failure in nightlies seem to be related to this: https://ci.ros2.org/view/nightly/job/nightly_linux_release/1681/.

[jacobperron]

Added a release note for Galactic: ros2/ros2_documentation#871

[v-lopez]

I apologise for coming back to this merged changes, but have you found any way of keeping compatibility between new and old code? Or the only solution right now is a branch for foxy and new developments on a separate branch?

[jacobperron]

@v-lopez Unfortunately, the original bug was "easily" fixed by changing API. In this case, I think the API break makes sense since the original API of passing a future was not necessary and part of the bug (see discussion in #1292 (comment)).

I think we can easily maintain backwards compatibility if we can use C++17, specifically std::variant. E.g. change the type of the goal callback member of the SendGoalOptions struct to a std::variant. I don't know if we've settled on the minimal C++ standard for Galactic. If we can use C++17, I'm happy to add a change to make this backward compatible.

[v-lopez]

In my use cases I've managed to workaround it using the returned shared_future from sending the goal.
My idea was more on the lines of adding the new callback to the foxy version somehow, since it is LTS a lot of packages are going to use it for the next years, and the old use was incorrect so I agree that it should be deprecated.

[jacobperron]

I don't think adding the new callback to Foxy can be done in an ABI (or API) compatible way. Also, my understanding is the error this bug causes happens rarely (if at all), so I think it's very unlikely that we would backport this change.

[jacobperron]

@v-lopez Please correct me if I'm wrong. If you are experiencing errors due to the bug this change fixes, perhaps we could patch it in Foxy a different way.

[v-lopez]

No, I've not experienced the bug at all.

Was just looking for a way to make the code compatible Foxy and onwards, luckily there's multiple ways of getting the GoalHandle, we can go with that. Thanks for your efforts.

[ivanpauno]

I think we can easily maintain backwards compatibility if we can use C++17, specifically std::variant. E.g. change the type of the goal callback member of the SendGoalOptions struct to a std::variant. I don't know if we've settled on the minimal C++ standard for Galactic. If we can use C++17, I'm happy to add a change to make this backward compatible.

Though using std::variant is nice, for this case we can manually implement an API backwards compatible adapter:

  struct SendGoalOptions
  {
    SendGoalOptions()
    : goal_response_callback(nullptr),
      feedback_callback(nullptr),
      result_callback(nullptr)
    {
    }

    /// Function called when the goal is accepted or rejected.
    /**
     * Takes a single argument that is a goal handle shared pointer.
     * If the goal is accepted, then the pointer points to a valid goal handle.
     * If the goal is rejected, then pointer has the value `nullptr`.
     */
    GoalResponseCallbackCompatibilitySelector goal_response_callback;

    /// Function called whenever feedback is received for the goal.
    FeedbackCallback feedback_callback;

    /// Function called when the result for the goal is received.
    ResultCallback result_callback;
  };

class GoalResponseCallbackCompatibilitySelector {
public:
  // implicit constructor
  [[deprecated("use new goal callback signature")]]
  GoalResponseCallbackCompatibilitySelector(GoalResponseCallbackOldSignature old_callback) : old_callback_(old_callback)
{}

  // implicit constructor
  GoalResponseCallbackCompatibilitySelector(GoalResponseCallbackNewSignature new_callback) : new_callback_(new_callback)
{}

private:
  friend rclcpp_action::Client;  // to avoid exposing getters of new_callback and old_callback publicly

  // Consumer of the class will first try to use new_callback_ and if it is `nullptr`, old_callback_ will be used.
  GoalResponseCallbackNewSignature new_callback_;
  GoalResponseCallbackOldSignature old_callback_;
};

Considering this change might affect several users, I think it's worth going through a ping-pong deprecation cycle.

[jacobperron]

Thanks for the suggestion, @ivanpauno 🙇
I agree, we should avoid a hard break and go through a deprecation cycle. Would you like to open a PR applying your suggestion? Otherwise, I'm happy to do it.

[ivanpauno]

@v-lopez see #1495, which enabled backwards compatibility.