Node.createService does not work with async callback

[ldegen]

Description

It seems Node.createService has some problems when dealing with a asynchronous callback functions.

• Library Version: 0.23.2
• ROS Version: humble
• Platform / OS: Ubuntu 22.04.3 LTS (running in docker container)

Steps To Reproduce

const rclnodejs = require('rclnodejs');

function createNode() {
  const node = new rclnodejs.Node('bug');
  node.createService('rclnodejs_bug/srv/GetAnswer', 'bug/get_answer', async (req, rsp) => {
    const response = rsp.template;
    response.answer = 42;
    setTimeout(()=>rsp.send(response), 100);
  });
  return node;
};

(async ()=>{
  await rclnodejs.init();
  const node = createNode();
  node.spin();
})();

I run the above code using node (20.10.0).

Then on a second terminal, I run ros2 service call /bug/get_answer rclnodejs_bug/srv/GetAnswer

Expected Behavior

requester: making request: rclnodejs_bug.srv.GetAnswer_Request()
^Chlr@kikuchiyo:/workspaces/pwd$ ros2 service call /bug/get_answer rclnodejs_bug/srv/GetAnswer 
requester: making request: rclnodejs_bug.srv.GetAnswer_Request()

response:
rclnodejs_bug.srv.GetAnswer_Response(answer=42)

Actual Behavior

/workspaces/pwd/rclnodejs_bug/node_modules/rclnodejs/generated/rclnodejs_bug/rclnodejs_bug__srv__GetAnswer_Response.js:64
        throw new TypeError('Invalid argument: answer in GetAnswer_Response');
        ^

TypeError: Invalid argument: answer in GetAnswer_Response
    at GetAnswer_ResponseWrapper.freeze (/workspaces/pwd/rclnodejs_bug/node_modules/rclnodejs/generated/rclnodejs_bug/rclnodejs_bug__srv__GetAnswer_Response.js:64:15)
    at GetAnswer_ResponseWrapper.serialize (/workspaces/pwd/rclnodejs_bug/node_modules/rclnodejs/generated/rclnodejs_bug/rclnodejs_bug__srv__GetAnswer_Response.js:69:10)
    at Service.processRequest (/workspaces/pwd/rclnodejs_bug/node_modules/rclnodejs/lib/service.js:82:44)
    at /workspaces/pwd/rclnodejs_bug/node_modules/rclnodejs/lib/node.js:235:21
    at Node._runWithMessageType (/workspaces/pwd/rclnodejs_bug/node_modules/rclnodejs/lib/node.js:1654:5)
    at Node.execute (/workspaces/pwd/rclnodejs_bug/node_modules/rclnodejs/lib/node.js:226:12)

Node.js v20.10.0

Workaround

Remove the async keyword from the callback. If necessary, move code that uses await and friends to a separate function.

What I think is the problem

Have a look at this piece of code in service.js:

https://github.com/RobotWebTools/rclnodejs/blob/4576609fa1941b2694eb1068eaccacf17cecf151/lib/service.js#L77-L84

It seems that the code expects callback to either return a response synchronously, or to call response.send(...). The if condition in line 80 tries to determine if the latter has not happened yet and if in fact there is a return value. The problem here is that for an async function, there is always a return value: A Promise.

In my toy example above, this Promise object gets misinterpreted as an actual response value. Also, the check for response.sent does not help in this case, because the response is send asynchronous. So at the time that the if condition is evaluated, it has in fact not been sent yet.

The resulting behavior from the service module is that it will try to send a second response, ultimately failing (because the returned value is not a response, but a Promise resolving to undefined), and causing a considerable amount of confusion in for example a mocha test suite that was executing this.

As for possible mitigations, I am not sure. Maybe it would help to simply ignore the return value? I think this is undocumented behavior anyway?.

[minggangw]

@ldegen thanks for submitting the issue and investigating the details, I will take a look later. Please feel free to open a PR if you are interested in fixing it, thanks!

[ldegen]

@minggangw I think I have an idea how to fix it without breaking existing code: Simply wrap the return value of callback in a Promise.resolve(...) and then wait for this to resolve before continuing. That should tick all the boxes, I think. Testing this now, if it works, I'll create a PR.

[minggangw]

@minggangw I think I have an idea how to fix it without breaking existing code: Simply wrap the return value of callback in a Promise.resolve(...) and then wait for this to resolve before continuing. That should tick all the boxes, I think. Testing this now, if it works, I'll create a PR.

Thanks, I have approved the PR and am planning to make a hotfix release once your PR merged.

[minggangw]

@ldegen your fix has landed onto latest v0.23.3 https://www.npmjs.com/package/rclnodejs/v/0.23.3 !