Fix un-advertisement and multiple-advertisement of Service callbacks

src/core/Service.js

@@ -11,6 +11,13 @@ import { EventEmitter } from 'eventemitter3';
  * @template TRequest, TResponse
  */
 export default class Service extends EventEmitter {
+  /**
+     * Stores a reference to the most recent service callback advertised so it can be removed from the EventEmitter during un-advertisement
+     * @private
+     * @type {((rosbridgeRequest) => any) | null}
+     */
+  _serviceCallback = null;
+  isAdvertised = false;
   /**
    * @param {Object} options
    * @param {Ros} options.ros - The ROSLIB.Ros connection handle.
@@ -22,9 +29,6 @@ export default class Service extends EventEmitter {
     this.ros = options.ros;
     this.name = options.name;
     this.serviceType = options.serviceType;
-    this.isAdvertised = false;
-
-    this._serviceCallback = null;
   }
   /**
    * @callback callServiceCallback
@@ -85,46 +89,51 @@ export default class Service extends EventEmitter {
    * @param {advertiseCallback} callback - This works similarly to the callback for a C++ service and should take the following params
    */
   advertise(callback) {
-    if (this.isAdvertised || typeof callback !== 'function') {
-      return;
+    if (this.isAdvertised) {
+      throw new Error('Cannot advertise the same Service twice!');
     }
 
-    this._serviceCallback = callback;
-    this.ros.on(this.name, this._serviceResponse.bind(this));
+    // Store the new callback for removal during un-advertisement
+    this._serviceCallback = (rosbridgeRequest) => {
+      var response = {};
+      var success = callback(rosbridgeRequest.args, response);
+
+      var call = {
+        op: 'service_response',
+        service: this.name,
+        values: response,
+        result: success
+      };
+
+      if (rosbridgeRequest.id) {
+        call.id = rosbridgeRequest.id;
+      }
+
+      this.ros.callOnConnection(call);
+    };
+
+    this.ros.on(this.name, this._serviceCallback);
     this.ros.callOnConnection({
       op: 'advertise_service',
       type: this.serviceType,
       service: this.name
     });
     this.isAdvertised = true;
   }
+
   unadvertise() {
     if (!this.isAdvertised) {
-      return;
+      throw new Error(`Tried to un-advertise service ${this.name}, but it was not advertised!`);
     }
     this.ros.callOnConnection({
       op: 'unadvertise_service',
       service: this.name
     });
-    this.isAdvertised = false;
-  }
-  _serviceResponse(rosbridgeRequest) {
-    var response = {};
-    // @ts-expect-error -- possibly null
-    var success = this._serviceCallback(rosbridgeRequest.args, response);
-
-    var call = {
-      op: 'service_response',
-      service: this.name,
-      values: response,
-      result: success
-    };
-
-    if (rosbridgeRequest.id) {
-      call.id = rosbridgeRequest.id;
+    // Remove the registered callback
+    if (this._serviceCallback) {
+      this.ros.off(this.name, this._serviceCallback);
     }
-
-    this.ros.callOnConnection(call);
+    this.isAdvertised = false;
   }
 
   /**
@@ -135,7 +144,7 @@ export default class Service extends EventEmitter {
     if (this.isAdvertised) {
       throw new Error('Cannot advertise the same Service twice!');
     }
-    this.ros.on(this.name, async (rosbridgeRequest) => {
+    this._serviceCallback = async (rosbridgeRequest) => {
       /** @type {{op: string, service: string, values?: TResponse, result: boolean, id?: string}} */
       let rosbridgeResponse = {
         op: 'service_response',
@@ -151,7 +160,8 @@ export default class Service extends EventEmitter {
         }
         this.ros.callOnConnection(rosbridgeResponse);
       }
-    });
+    }
+    this.ros.on(this.name, this._serviceCallback);
     this.ros.callOnConnection({
       op: 'advertise_service',
       type: this.serviceType,

test/service.test.js

@@ -24,5 +24,32 @@ describe('Service', () => {
     })
     const response = await new Promise((resolve, reject) => client.callService({}, resolve, reject));
     expect(response).toEqual({success: true, message: 'foo'});
+    // Make sure un-advertisement actually disposes of the event handler
+    expect(ros.listenerCount(server.name)).toEqual(1);
+    server.unadvertise();
+    expect(ros.listenerCount(server.name)).toEqual(0);
+  })
+  it('Successfully advertises a service with a synchronous return', async () => {
+    const server = new Service({
+      ros,
+      serviceType: 'std_srvs/Trigger',
+      name: '/test_service'
+    });
+    server.advertise((request, response) => {
+      response.success = true;
+      response.message = 'bar';
+      return true;
+    });
+    const client = new Service({
+      ros,
+      serviceType: 'std_srvs/Trigger',
+      name: '/test_service'
+    })
+    const response = await new Promise((resolve, reject) => client.callService({}, resolve, reject));
+    expect(response).toEqual({success: true, message: 'bar'});
+    // Make sure un-advertisement actually disposes of the event handler
+    expect(ros.listenerCount(server.name)).toEqual(1);
+    server.unadvertise();
+    expect(ros.listenerCount(server.name)).toEqual(0);
   })
 })