Client only saves one callback at a time

[ejalaa12]

Description

When using a Service Client, and sending two request in a row, because the Client saves one callback at a time, only the latest is used when processing the response message from the service. (look at line 63) https://github.com/RobotWebTools/rclnodejs/blob/2e05913fb0e818f0c3a58904764ef752902a9a5e/lib/client.js#L51-L64

• Library Version: latest develop
• ROS Version: galactic
• Platform / OS: Linux Ubuntu 20.04

Steps To Reproduce

Here is a minimal example that reproduces the bug (based on the example/client-example.js):

'use strict';

const rclnodejs = require('../index.js');

rclnodejs
  .init()
  .then(() => {
    const node = rclnodejs.createNode('client_example_node');

    const client = node.createClient(
      'example_interfaces/srv/AddTwoInts',
      'add_two_ints'
    );

    // First call
    client.waitForService(1000).then((result) => {
      const request = {
        a: Math.floor(1),
        b: Math.floor(2),
      };
      if (!result) {
        console.log('Error: service not available');
        // rclnodejs.shutdown();
        return;
      }
      console.log(`Sending: ${typeof request}`, request);
      client.sendRequest(request, (response) => {
        console.log(`Result1: ${typeof response}`, response);
        // rclnodejs.shutdown();
      });
    });

    // Second call
    client.waitForService(1000).then((result) => {

      const request = {
        a: Math.floor(10),
        b: Math.floor(20),
      };
      if (!result) {
        console.log('Error: service not available');
        // rclnodejs.shutdown();
        return;
      }
      console.log(`Sending: ${typeof request}`, request);
      client.sendRequest(request, (response) => {
        console.log(`Result2: ${typeof response}`, response);
        // rclnodejs.shutdown();
      });
    });

    rclnodejs.spin(node);
    setTimeout(() => {
      rclnodejs.shutdown();
    }, 1000);
  })
  .catch((e) => {
    console.log(`Error: ${e}`);
  });

Expected Behavior

The console log output should be:

Sending: object { a: 1, b: 2 }
Sending: object { a: 10, b: 20 }
Result1: object { sum: 3 }
Result2: object { sum: 30 }

Actual Behavior

Result2 is printed twice:

Sending: object { a: 1, b: 2 }
Sending: object { a: 10, b: 20 }
Result2: object { sum: 3 }
Result2: object { sum: 30 }

[ejalaa12]

The _sequenceNumber is also overwritten with each new request so it cannot be used to match a callback.

One way to fix this would be to save each callback in a pending_requests map (matched to its sequence number).

And then when we get the response from rcl, we can use the sequence number that is included in the response header to match it with the correct callback like rclcpp does

We are not sure how the response from rcl is handled, as it must contain the correct sequence number. @minggangw do you have any insight on this?

[minggangw]

@ejalaa12 Yes, we should handle the response exactly as the rclcpp does, thanks for investigating the C++ implementation. For nodejs, as you said we can keep a sequence <-> callback map in my mind, and what you need is to change the following

https://github.com/RobotWebTools/rclnodejs/blob/b9ac65211cc19bd6c53a7bca6f1a59ab9591d64c/lib/node.js#L207-L214

• L.209: passing the sequence number stored in the map.
• L.213: passing the sequence number back to the processResponse() and letting the client find the proper callback if success is true.

I haven't touched the code for a looong time, maybe make some mistake, just for your reference.

[wayneparrott]

+1 Good suggestion! I noticed rclpy impl'ed async client support similarly: https://github.com/ros2/rclpy/pull/170

[ejalaa12]

Yes I think using the pending_request map is a good solution.

However, when receiving the response from rcl, we should have access to that response's header which contains the sequence number that we can match with the correct callback.

I think it must be done in the rcl binding but I'm not sure. Do you have any idea ?

[ejalaa12]

@wayneparrott thanks for pointing out rclpy. I found the solution there.

As you can see, they use rcl_take_response_with_info instead of rcl_take_response which allows them to get the header as well, which contains the sequence_number associated with the response that we can use to match it with a callback from the pending_request map.

So inside rcl_bindings.cpp, we need to change this code https://github.com/RobotWebTools/rclnodejs/blob/2e05913fb0e818f0c3a58904764ef752902a9a5e/src/rcl_bindings.cpp#L829-L852 and replace rcl_take_response with rcl_take_response_with_info and return the header (or just the sequence number) along with the message. I might need help with that part.

[ejalaa12]

NAN_METHOD(RclTakeResponse) {
  rcl_client_t* client = reinterpret_cast<rcl_client_t*>(
      RclHandle::Unwrap<RclHandle>(
          Nan::To<v8::Object>(info[0]).ToLocalChecked())
          ->ptr());
  int64_t sequence_number = Nan::To<int64_t>(info[1]).FromJust();

  rmw_service_info_t header;

  void* taken_response =
      node::Buffer::Data(Nan::To<v8::Object>(info[2]).ToLocalChecked());
  rcl_ret_t ret = rcl_take_response_with_info(client, &header, taken_response);
  header.request_id;     // TODO use this to return the sequence number from the response

  if (ret == RCL_RET_OK) {
    info.GetReturnValue().Set(Nan::True());
    return;
  }

  rcl_reset_error();
  info.GetReturnValue().Set(Nan::False());
}

It might look like this.

[minggangw]

@ejalaa12 & @imcelroy I think we can close this issue now, thanks for fixing this! If you find any bugs or you want to propose new features, please don't hesitate to open an issue. Meanwhile, you can add your names into CONTRIBUTORS if you like.

[ejalaa12]

@minggangw Thanks for being so responsive ! It made it easier to contribute :)

Meanwhile, you can add your names into CONTRIBUTORS if you like.

How can we do this?

[minggangw]

Just submit a PR to add your names and contributions into https://github.com/RobotWebTools/rclnodejs/blob/develop/CONTRIBUTORS.md, then I will merge it