Incorrect Type for ClientGoalHandle.getResult

[csmith-rtr]

Description The type definitions for ClientGoalHandle.getResult indicates that it returns Promise<ActionResult<T>>. It actually returns a type

interface ActionResultWithStatus<T> {
    status: number,
    result: ActionResult<T>
};

• Library Version: 0.20.0
• ROS Version: foxy
• Platform / OS: ubuntu 20.04

Steps To Reproduce The following does not compile with tsc although if you cast result to any it does and result satisfies ActionResultWithStatus as described above.

import * as rclnodejs from 'rclnodejs'

async function main() {
  await rclnodejs.init();
  const node = new rclnodejs.Node('my_node');
  node.spin();
  const FollowJointTrajectory = rclnodejs.require('control_msgs/action/FollowJointTrajectory');

  const action_server = new rclnodejs.ActionServer(node, 'control_msgs/action/FollowJointTrajectory', '/my_action',
    (gh) => {
      gh.succeed();
      const result = new FollowJointTrajectory.Result();
      return result;
    }
  );

  const action_client = new rclnodejs.ActionClient(node, 'control_msgs/action/FollowJointTrajectory', '/my_action');
  await action_client.waitForServer();

  const goal = new FollowJointTrajectory.Goal();
  const gh = await action_client.sendGoal(goal);
  const result = await gh.getResult();
  console.log('Result: %j', result);
  console.log('Result Status: ', result.status);
  console.log('Result Result: ', result.result);
  node.stop();
}

main();

Expected Behavior compile without casting result to any

Actual Behavior

[wayneparrott]

Thx for filling this issue. Investigating.

[wayneparrott]

Quick feedback:

I setup the following environment: ros2 foxy w/ control msgs installed, rclnodejs 0.20, ts 4.4, node 12.18 and your example code. Since I installed the control_msgs package after installing the rclnodejs package I had to regenerate the javascript interfaces and TS types to pickup the new control msgs, e.g., npx generate-ros-messages

I'm not well versed on ros2 actions (yet) but I'm seeing the TS compiler and the generated types for the control_msgs are working correctly. See the screenshot of the TS error messages on your example code. From the error msg the TS compiler is correctly expecting a FollowJointTrajectoryResult {errorCode, errorString}. I'm not aware of the ActionResultWithStatus interface - can you point me to where that type is defined? What I do see is the idl>Javascript tranpilation created an internal impl msg class named control_msgs_action_FolowJointTrajectory_GetResult_Response.js with similar structure to the ActionResultWithStatus definition you posted. My understanding is you should not be getting an instance of that internal implementation class.

Note: I did not run any code; I only ran the TS compiler with your example. I can look deeper tomorrow to see if somehow the internal xxx_GetResult_Response.js code is returning. Please feel free to share any additional info that I/we can use to better understand and resolve this issue more quickly.

@minggangw please override this response if I'm missing something here.

[csmith-rtr]

ActionResultWithStatus is not defined anywhere I'm aware of, but that is the type that is returned. I believe you may need to return that type in order for clients to get the status since I think the ClientGoalHandle.status is set separately and may not be available until some point in the future when a new status message comes in (though it appears this also has the wrong type - it is a string but I believe it should be a number).

This is also what the action client example expects so it appears the code is correct but the types are wrong, though I'm not aware of what may have been intended in the implementation.

[wayneparrott]

Thx. Continuing to investigate. The principle dev that impl'ed the action api has not been involved on the project for some time. Will follow up as soon as I have more details.

[wayneparrott]

@csmith-rtr I'm able to replicate the bug as you've shared. Thanks again for the report. Working on a PR to resolve asap. Will update progress here.

[wayneparrott]

After a little debugging I've found 2 issues:

1. the ClientGoalHandle.status property is intended to hold the status code (state id) across the goal processing lifecycle. Yet, internally this property is not being updated due to a race condition.
2. this one is subjective and I'll review with @minggangw. The await ClientGoalHandle.getResult() is leaking the internal {status,result} object. It should only return the result property. If we make this change then the TS types will correctly reflect the api's behavior. If we choose to return the {status,result} object we will need to add an additional type declaration, e.g., <T>ResultWithStatus and modify the generator logic.

Looking at #1 now. Will discuss #2 with @minggangw

1 - a race condition arises when the action-server is sends a status msg before returning a goal-request-response message. The latter msg cause the action-client to setup internal state for the goalClient. Yet, the preceding status msg depends up on the goalClient state to be established which it isn't. Thus the status msg is ignored. A similar race condition arises when the goal request is completed.

[csmith-rtr]

I don't believe ClientGoalHandle.status can be guaranteed to be set by the time the result is accessed if it is only set via the */status topic. If you're going to remove status from await ClientGoalHandle.getResult() then I think you should make sure to set the status on the goal handle before resolving the promise.

[wayneparrott]

... I think you should make sure to set the status on the goal handle before resolving the promise

+1

[minggangw]

Sorry for the delay, as I'm still on holiday. @csmith-rtr does the example, action-client-example.js, run correctly on your side? I went through the code quickly, it seems that the getResult should return a Promise, not sure what goes wrong here.

[csmith-rtr]

@minggangw yes the example works correctly. The issue is that the typescript types do not match the value the promise resolves with.