diff --git a/CHANGELOG.md b/CHANGELOG.md index a30736c..2a92069 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ All notable changes to `@homebridge/ciao` will be documented in this file. This - fix: send the probe and announce retry chatter to debug instead of the console (#72) - fix: let an empty DEBUG decline a prerelease build's automatic debug output (#72) - fix: read DEBUG before the debug package erases an empty one (#72) +- fix: cancel queued advertisement after service destruction (#73) ## v1.3.10 (2026-07-08) diff --git a/src/CiaoService.ts b/src/CiaoService.ts index 45c7950..239fd00 100644 --- a/src/CiaoService.ts +++ b/src/CiaoService.ts @@ -449,12 +449,27 @@ export class CiaoService extends EventEmitter { * @returns */ public async destroy(): Promise { - await this.end(); - + const endPromise = this.end(); this.destroyed = true; + + try { + await endPromise; + } catch (error) { + this.destroyed = false; + throw error; + } + this.removeAllListeners(); } + /** + * Internal lifecycle query used by {@link Responder} to avoid directly reading private state. + * @private + */ + public isDestroyed(): boolean { + return this.destroyed; + } + /** * @returns The fully qualified domain name of the service, used to identify the service. */ diff --git a/src/Responder.ts b/src/Responder.ts index 3377718..751e8f2 100644 --- a/src/Responder.ts +++ b/src/Responder.ts @@ -283,11 +283,22 @@ export class Responder implements PacketHandler { // - Prober will call the Responder to generate responses to its queries to // resolve name conflicts the same way as with other services on the network - this.promiseChain = this.promiseChain // we synchronize all ongoing probes here - .then(() => service.rebuildServiceRecords()) // build the records the first time for the prober - .then(() => this.probe(service)); // probe errors are catch below + this.promiseChain = this.promiseChain.then(() => { + if (service.isDestroyed()) { + throw Prober.CANCEL_REASON; + } + + service.rebuildServiceRecords(); + return this.probe(service); // probe errors are catch below + }); return this.promiseChain.then(() => { + if (service.isDestroyed()) { + service.serviceState = ServiceState.UNANNOUNCED; + callback(); + return; + } + // we are not returning the promise returned by announced here, only PROBING is synchronized this.announce(service).catch(reason => { // handle announce errors diff --git a/src/advertise-destroy-race.spec.ts b/src/advertise-destroy-race.spec.ts new file mode 100644 index 0000000..176aa6e --- /dev/null +++ b/src/advertise-destroy-race.spec.ts @@ -0,0 +1,192 @@ +import { CiaoService, InternalServiceEvent, ServiceState } from "./CiaoService"; +import { Responder } from "./Responder"; +import { NetworkManager } from "./NetworkManager"; +import { Announcer } from "./responder/Announcer"; + +interface FakeResponderInternal { + server: { + sendQueryBroadcast: jest.Mock; + getBoundInterfaceNames: jest.Mock; + send: jest.Mock; + }; + promiseChain: Promise; + servicePointer: Map; + announcedServices: Map; + getAnnouncedServices: () => IterableIterator; + probe?: (service: CiaoService) => Promise; +} + +interface FakeResponderFacade { + advertiseService: (service: CiaoService, callback: (error?: Error | undefined) => void) => Promise; + unpublishService: (service: CiaoService) => Promise; +} + +function makeFakeResponder() { + const server = { + sendQueryBroadcast: jest.fn(async () => []), + getBoundInterfaceNames: jest.fn(() => []), + send: jest.fn(async () => ({ status: "fulfilled", interface: "en0" })), + }; + + const responder = Object.create(Responder.prototype) as unknown as FakeResponderInternal; + responder.server = server; + responder.promiseChain = Promise.resolve(); + responder.servicePointer = new Map(); + responder.announcedServices = new Map(); + responder.getAnnouncedServices = () => responder.announcedServices.values(); + + return { responder, server }; +} + +function makeService(): CiaoService { + const networkManager = { getInterfaceMap: () => new Map() } as unknown as NetworkManager; + return new CiaoService(networkManager, { + name: "Test Service", + type: "http", + port: 4711, + }); +} + +function wire(responder: Responder | FakeResponderInternal, service: CiaoService): void { + const responderFacade = responder as unknown as FakeResponderFacade; + service.on(InternalServiceEvent.PUBLISH, responderFacade.advertiseService.bind(responderFacade, service)); + service.on(InternalServiceEvent.UNPUBLISH, responderFacade.unpublishService.bind(responderFacade, service)); +} + +async function flush(): Promise { + await Promise.resolve(); + await Promise.resolve(); +} + +async function advance(ms: number): Promise { + jest.advanceTimersByTime(ms); + await flush(); +} + +describe("immediate advertise()/destroy() lifecycle race", () => { + let announceSpy: jest.SpiedFunction; + let randomSpy: jest.SpiedFunction; + + beforeEach(() => { + jest.useFakeTimers(); + announceSpy = jest.spyOn(Announcer.prototype, "announce"); + randomSpy = jest.spyOn(Math, "random").mockReturnValue(0); + }); + + afterEach(() => { + jest.useRealTimers(); + announceSpy.mockRestore(); + randomSpy.mockRestore(); + }); + + it("does not probe, announce, or retry after a destroy raced before startup", async () => { + const { responder, server } = makeFakeResponder(); + const service = makeService(); + wire(responder, service); + + const advertisePromise = service.advertise(); + expect(service.serviceState).toBe(ServiceState.UNANNOUNCED); + + await service.destroy(); + + for (let i = 0; i < 20; i++) { + await advance(250); + } + + expect({ + probeBroadcasts: server.sendQueryBroadcast.mock.calls.length, + announcements: announceSpy.mock.calls.length, + serviceState: service.serviceState, + announcedServices: Array.from(responder.getAnnouncedServices()).length, + }).toEqual({ + probeBroadcasts: 0, + announcements: 0, + serviceState: ServiceState.UNANNOUNCED, + announcedServices: 0, + }); + await expect(advertisePromise).resolves.toBeUndefined(); + expect(jest.getTimerCount()).toBe(0); + }); + + it("does not announce when destroy lands after probing has started", async () => { + const { responder } = makeFakeResponder(); + const service = makeService(); + wire(responder, service); + + let finishProbe: () => void = () => { + throw new Error("probe did not start"); + }; + const probePromise = new Promise(resolve => { + finishProbe = resolve; + }); + const probe = jest.fn((probingService: CiaoService) => { + probingService.serviceState = ServiceState.PROBING; + return probePromise.then(() => { + probingService.serviceState = ServiceState.PROBED; + }); + }); + responder.probe = probe; + + const advertisePromise = service.advertise(); + await flush(); + + expect(probe).toHaveBeenCalledTimes(1); + expect(service.serviceState).toBe(ServiceState.PROBING); + + await service.destroy(); + finishProbe(); + await flush(); + + await expect(advertisePromise).resolves.toBeUndefined(); + expect(announceSpy).not.toHaveBeenCalled(); + expect(service.serviceState).toBe(ServiceState.UNANNOUNCED); + expect(Array.from(responder.getAnnouncedServices())).toHaveLength(0); + expect(jest.getTimerCount()).toBe(0); + }); + + it("survives repeated immediate advertise/destroy races without leftover work", async () => { + const cycles = 50; + + for (let i = 0; i < cycles; i++) { + const { responder, server } = makeFakeResponder(); + const service = makeService(); + wire(responder, service); + + const advertisePromise = service.advertise(); + await service.destroy(); + await expect(advertisePromise).resolves.toBeUndefined(); + + expect(server.sendQueryBroadcast).not.toHaveBeenCalled(); + expect(service.serviceState).toBe(ServiceState.UNANNOUNCED); + expect(Array.from(responder.getAnnouncedServices())).toHaveLength(0); + } + + await advance(5000); + expect(announceSpy).not.toHaveBeenCalled(); + expect(jest.getTimerCount()).toBe(0); + }); + + it("keeps the service usable when shutdown rejects", async () => { + const service = makeService(); + const shutdownError = new Error("goodbye failed"); + let shutdownAttempts = 0; + service.serviceState = ServiceState.ANNOUNCED; + service.on(InternalServiceEvent.UNPUBLISH, callback => { + callback(shutdownAttempts++ === 0? shutdownError: undefined); + }); + + await expect(service.destroy()).rejects.toThrow(shutdownError); + + await expect(service.end()).resolves.toBeUndefined(); + expect(shutdownAttempts).toBe(2); + expect(service.listenerCount(InternalServiceEvent.UNPUBLISH)).toBe(1); + }); + + it("preserves the existing guard against destroying a service twice", async () => { + const service = makeService(); + + await service.destroy(); + + await expect(service.destroy()).rejects.toThrow("Cannot end destroyed service!"); + }); +});