diff --git a/src/core/Service.js b/src/core/Service.js index 1fe8935ba..dfcdea3b3 100644 --- a/src/core/Service.js +++ b/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,12 +89,30 @@ 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, @@ -98,33 +120,20 @@ export default class Service extends EventEmitter { }); 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, diff --git a/test/service.test.js b/test/service.test.js index e9d9aba15..645d5d0be 100644 --- a/test/service.test.js +++ b/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); }) })