From 49a847100b4eda631508df4677292075ddf586bc Mon Sep 17 00:00:00 2001 From: Ezra Brooks Date: Tue, 19 Mar 2024 14:15:27 -0600 Subject: [PATCH 1/3] Fix unadvertisement of Service callbacks --- src/core/Service.js | 60 ++++++++++++++++++++++++++------------------ test/service.test.js | 27 ++++++++++++++++++++ 2 files changed, 62 insertions(+), 25 deletions(-) diff --git a/src/core/Service.js b/src/core/Service.js index 1fe8935ba..7aa78d9d1 100644 --- a/src/core/Service.js +++ b/src/core/Service.js @@ -24,6 +24,10 @@ export default class Service extends EventEmitter { this.serviceType = options.serviceType; this.isAdvertised = false; + /** + * Stores a reference to the most recent service callback advertised so it can be removed from the EventEmitter during un-advertisement + * @type {((rosbridgeRequest) => any) | null} + */ this._serviceCallback = null; } /** @@ -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); }) }) From 6833a786acdfb9fbc1434cb30a0ca3bfb5492645 Mon Sep 17 00:00:00 2001 From: Ezra Brooks Date: Wed, 20 Mar 2024 08:05:48 -0600 Subject: [PATCH 2/3] Move _serviceCallback declaration into the class body --- src/core/Service.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/core/Service.js b/src/core/Service.js index 7aa78d9d1..f0371f8d2 100644 --- a/src/core/Service.js +++ b/src/core/Service.js @@ -11,6 +11,12 @@ 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; /** * @param {Object} options * @param {Ros} options.ros - The ROSLIB.Ros connection handle. @@ -23,12 +29,6 @@ export default class Service extends EventEmitter { this.name = options.name; this.serviceType = options.serviceType; this.isAdvertised = false; - - /** - * Stores a reference to the most recent service callback advertised so it can be removed from the EventEmitter during un-advertisement - * @type {((rosbridgeRequest) => any) | null} - */ - this._serviceCallback = null; } /** * @callback callServiceCallback From 50c9f080cf2e7ddd28b1b4472bf7022f8c1fc259 Mon Sep 17 00:00:00 2001 From: Ezra Brooks Date: Wed, 20 Mar 2024 08:06:29 -0600 Subject: [PATCH 3/3] also move isAdvertised --- src/core/Service.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/Service.js b/src/core/Service.js index f0371f8d2..dfcdea3b3 100644 --- a/src/core/Service.js +++ b/src/core/Service.js @@ -17,6 +17,7 @@ export default class Service extends EventEmitter { * @type {((rosbridgeRequest) => any) | null} */ _serviceCallback = null; + isAdvertised = false; /** * @param {Object} options * @param {Ros} options.ros - The ROSLIB.Ros connection handle. @@ -28,7 +29,6 @@ export default class Service extends EventEmitter { this.ros = options.ros; this.name = options.name; this.serviceType = options.serviceType; - this.isAdvertised = false; } /** * @callback callServiceCallback