Conversation
Signed-off-by: Stephen Brawner <brawner@gmail.com>
|
Testing current unit tests,
|
|
Testing with unit tests introduced in #1290
|
jacobperron
left a comment
There was a problem hiding this comment.
Taking a step back, to me the promise pertains to the goal handle (which may eventually contain a result). Therefore, I don't think it makes sense to set an exception on the promise if there's an issue requesting the result, rather that exception should somehow manifest itself later when trying to access the result. Does that make sense?
|
|
||
| promise->set_value(goal_handle); | ||
| if (options.goal_response_callback) { | ||
| options.goal_response_callback(future); |
There was a problem hiding this comment.
It was moved in response to #701 (review); if make_result_aware is called first, we risk having the result callback triggered before the goal response callback (unlikely, but I think this is possible if the action server is running in the same process).
There was a problem hiding this comment.
Hmm, reflecting on this, it seems to me that the goal response callback should simply be taking a concrete goal handle instead of a future one. Additionally, failing to make the goal handle result aware should not result in calling code losing access to the goal handle. Getting the future result should throw, but the exception could carry the goal handle (which is still valid) with it.
That's not a fix we can backport though.
There was a problem hiding this comment.
@hidmic I agree with everything you've said. I think it's okay to not backport changes (though maybe we could by supporting two different signatures for the goal response callback).
There was a problem hiding this comment.
Adjusted the goal_response_callback signature, but I'm still not decided about what to do exception and goal handle bit.
There was a problem hiding this comment.
I think we can remove the try-catch altogether. I'll open a PR in a moment with a proposed alternative.
Signed-off-by: Stephen Brawner <brawner@gmail.com>
|
Closing in favor of #1311 |
This PR fixes a potential issue, where
promise->set_valueis called before a potential call topromise->set_exception. Only one can be called, otherwise astd::future_erroris thrown. This was the original ordering in #594, but it was rearranged in #738. @jacobperron @hidmic if this is breaking something unintentionally, please let me know. It should pass current unit tests as is. This patch is required in order to pass tests in #1290.Signed-off-by: Stephen Brawner brawner@gmail.com