diff --git a/package-lock.json b/package-lock.json index f98cccf77..47b7c0c84 100644 --- a/package-lock.json +++ b/package-lock.json @@ -32,7 +32,7 @@ "vitest": "^3.0.2" }, "engines": { - "node": ">=0.10" + "node": ">=18" } }, "node_modules/@asamuzakjp/css-color": { @@ -2793,19 +2793,6 @@ "url": "https://opencollective.com/eslint" } }, - "node_modules/eslint/node_modules/@eslint/js": { - "version": "9.32.0", - "resolved": "https://registry.npmjs.org/@eslint/js/-/js-9.32.0.tgz", - "integrity": "sha512-BBpRFZK3eX6uMLKz8WxFOBIFFcGFJ/g8XuwjTHCqHROSIsopI+ddn/d5Cfh36+7+e5edVS8dbSHnBNhrLEX0zg==", - "dev": true, - "license": "MIT", - "engines": { - "node": "^18.18.0 || ^20.9.0 || >=21.1.0" - }, - "funding": { - "url": "https://eslint.org/donate" - } - }, "node_modules/eslint/node_modules/escape-string-regexp": { "version": "4.0.0", "resolved": "https://registry.npmjs.org/escape-string-regexp/-/escape-string-regexp-4.0.0.tgz", diff --git a/src/core/Service.js b/src/core/Service.js index 9d9984468..b3d6b2997 100644 --- a/src/core/Service.js +++ b/src/core/Service.js @@ -94,7 +94,9 @@ export default class Service extends EventEmitter { */ advertise(callback) { if (this.isAdvertised) { - throw new Error('Cannot advertise the same Service twice!'); + // If already advertised, unadvertise first then re-advertise with new callback + // This prevents throwing an error and allows smooth re-advertising + this.unadvertise(); } // Store the new callback for removal during un-advertisement @@ -127,17 +129,26 @@ export default class Service extends EventEmitter { unadvertise() { if (!this.isAdvertised) { - throw new Error(`Tried to un-advertise service ${this.name}, but it was not advertised!`); + // Silently return if not advertised instead of throwing an error + // This prevents race conditions where multiple unadvertise calls happen + return; } - this.ros.callOnConnection({ - op: 'unadvertise_service', - service: this.name - }); - // Remove the registered callback + + // Remove the registered callback first to stop processing new requests if (this._serviceCallback) { this.ros.off(this.name, this._serviceCallback); + this._serviceCallback = null; } + + // Mark as not advertised before sending the message + // This ensures that any new advertise calls won't be blocked this.isAdvertised = false; + + // Send the unadvertise message to the server + this.ros.callOnConnection({ + op: 'unadvertise_service', + service: this.name + }); } /** @@ -146,8 +157,11 @@ export default class Service extends EventEmitter { */ advertiseAsync(callback) { if (this.isAdvertised) { - throw new Error('Cannot advertise the same Service twice!'); + // If already advertised, unadvertise first then re-advertise with new callback + // This prevents throwing an error and allows smooth re-advertising + this.unadvertise(); } + this._serviceCallback = async (rosbridgeRequest) => { /** @type {{op: string, service: string, values?: TResponse, result: boolean, id?: string}} */ let rosbridgeResponse = { diff --git a/test/service.test.js b/test/service.test.js index 645d5d0be..1e6b81332 100644 --- a/test/service.test.js +++ b/test/service.test.js @@ -52,4 +52,97 @@ describe('Service', () => { server.unadvertise(); expect(ros.listenerCount(server.name)).toEqual(0); }) + + it('Handles re-advertisement gracefully without throwing errors', async () => { + const server = new Service({ + ros, + serviceType: 'std_srvs/Trigger', + name: '/test_readvertise' + }); + + // First advertisement + server.advertise((_request, response) => { + response.success = true; + response.message = 'first'; + return true; + }); + + expect(server.isAdvertised).toBe(true); + expect(ros.listenerCount(server.name)).toEqual(1); + + // Re-advertise with different callback - should not throw + expect(() => { + server.advertise((_request, response) => { + response.success = true; + response.message = 'second'; + return true; + }); + }).not.toThrow(); + + expect(server.isAdvertised).toBe(true); + expect(ros.listenerCount(server.name)).toEqual(1); + + server.unadvertise(); + expect(server.isAdvertised).toBe(false); + expect(ros.listenerCount(server.name)).toEqual(0); + }) + + it('Handles multiple unadvertise calls gracefully', () => { + const server = new Service({ + ros, + serviceType: 'std_srvs/Trigger', + name: '/test_multiple_unadvertise' + }); + + server.advertise((_request, response) => { + response.success = true; + return true; + }); + + expect(server.isAdvertised).toBe(true); + + // First unadvertise + server.unadvertise(); + expect(server.isAdvertised).toBe(false); + + // Second unadvertise - should not throw + expect(() => { + server.unadvertise(); + }).not.toThrow(); + + expect(server.isAdvertised).toBe(false); + }) + + it('Handles re-advertisement with advertiseAsync gracefully', async () => { + const server = new Service({ + ros, + serviceType: 'std_srvs/Trigger', + name: '/test_readvertise_async' + }); + + // First advertisement + server.advertiseAsync(async () => { + return { + success: true, + message: 'first' + } + }); + + expect(server.isAdvertised).toBe(true); + + // Re-advertise with different callback - should not throw + expect(() => { + server.advertiseAsync(async () => { + return { + success: true, + message: 'second' + } + }); + }).not.toThrow(); + + expect(server.isAdvertised).toBe(true); + + server.unadvertise(); + expect(server.isAdvertised).toBe(false); + }) })