Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: availability checks not stopped on extension stop #20093

Merged
merged 5 commits into from
Dec 10, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 22 additions & 5 deletions lib/extension/availability.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,16 @@ const retrieveOnReconnect = [
{keys: ['color', 'color_temp'], condition: (state: KeyValue): boolean => state.state === 'ON'},
];

interface PinqQueueEntry {
device: Device;
interrupted: boolean;
}

export default class Availability extends Extension {
private timers: {[s: string]: NodeJS.Timeout} = {};
private availabilityCache: {[s: string]: boolean} = {};
private retrieveStateDebouncers: {[s: string]: () => void} = {};
private pingQueue: Device[] = [];
private pingQueue: PinqQueueEntry[] = [];
private pingQueueExecuting = false;

private getTimeout(device: Device): number {
Expand Down Expand Up @@ -59,20 +64,21 @@ export default class Availability extends Extension {
}

private addToPingQueue(device: Device): void {
this.pingQueue.push(device);
this.pingQueue.push({device, interrupted: false});
this.pingQueueExecuteNext();
}

private removeFromPingQueue(device: Device): void {
const index = this.pingQueue.findIndex((d) => d.ieeeAddr === device.ieeeAddr);
const index = this.pingQueue.findIndex((entry) => entry.device.ieeeAddr === device.ieeeAddr);
index != -1 && this.pingQueue.splice(index, 1);
}

private async pingQueueExecuteNext(): Promise<void> {
if (this.pingQueue.length === 0 || this.pingQueueExecuting) return;
this.pingQueueExecuting = true;

const device = this.pingQueue[0];
const entry = this.pingQueue[0];
const device = entry.device;
let pingedSuccessfully = false;
const available = this.availabilityCache[device.ieeeAddr] || this.isAvailable(device);
const attempts = available ? 2 : 1;
Expand All @@ -92,6 +98,12 @@ export default class Availability extends Extension {
}
}

if (entry.interrupted) {
// On stop, an async ping may have just been executing. To prevent side effects, exit here
// to avoid triggering any follow-up activity (e.g., re-queuing another ping attempt).
return;
}

this.publishAvailability(device, !pingedSuccessfully);
this.resetTimer(device);
this.removeFromPingQueue(device);
Expand Down Expand Up @@ -183,8 +195,13 @@ export default class Availability extends Extension {
}

override async stop(): Promise<void> {
// Stop ongoing ping and clear queued pings
this.pingQueue.forEach((entry) => entry.interrupted = true);
protyposis marked this conversation as resolved.
Show resolved Hide resolved
this.pingQueue = [];

Object.values(this.timers).forEach((t) => clearTimeout(t));
super.stop();

await super.stop();
}

private retrieveState(device: Device): void {
Expand Down
17 changes: 17 additions & 0 deletions test/availability.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import zigbeeHerdsman from './stub/zigbeeHerdsman';
import utils from '../lib/util/utils';
import * as settings from '../lib/util/settings';
import Controller from '../lib/controller';
import Availability from '../lib/extension/availability';
import flushPromises from './lib/flushPromises';
import stringify from 'json-stable-stringify-without-jsonify';

Expand Down Expand Up @@ -344,4 +345,20 @@ describe('Availability', () => {
expect(MQTT.publish).toHaveBeenCalledWith('zigbee2mqtt/group_tradfri_remote/availability',
'online', {retain: true, qos: 1}, expect.any(Function));
});

it('Should clear the ping queue on stop', async () => {
const availability = controller.extensions.find((extension) => extension instanceof Availability);
const publishAvailabilitySpy = jest.spyOn(availability, 'publishAvailability');

devices.bulb_color.zh = { ping: jest.fn().mockImplementation(() => new Promise((resolve) => setTimeout(resolve, 1000)))};
availability.addToPingQueue(devices.bulb_color);
availability.addToPingQueue(devices.bulb_color_2);

await availability.stop();
await advancedTime(utils.minutes(1));

expect(availability.pingQueue).toEqual([]);
// Validate the stop-interrupt implicitly by checking that it prevents further function invocations
expect(publishAvailabilitySpy).not.toHaveBeenCalled();
});
});