[Bug]: DuplicateDeviceError crashes library

[thieren]

Client version

2.1.0

Node version

16.13

Operating System type

Linux

Operating system version

Debian

Describe the bug

See code snippet from eufysecurity.ts (lines 495 ff.):

            promises.push(new_device.then((device: Device) => {
                device.on("property changed", (device: Device, name: string, value: PropertyValue) => this.onDevicePropertyChanged(device, name, value));
                device.on("raw property changed", (device: Device, type: number, value: string) => this.onDeviceRawPropertyChanged(device, type, value));
                device.on("crying detected", (device: Device, state: boolean) => this.onDeviceCryingDetected(device, state));
                device.on("sound detected", (device: Device, state: boolean) => this.onDeviceSoundDetected(device, state));
                device.on("pet detected", (device: Device, state: boolean) => this.onDevicePetDetected(device, state));
                device.on("motion detected", (device: Device, state: boolean) => this.onDeviceMotionDetected(device, state));
                device.on("person detected", (device: Device, state: boolean, person: string) => this.onDevicePersonDetected(device, state, person));
                device.on("rings", (device: Device, state: boolean) => this.onDeviceRings(device, state));
                device.on("locked", (device: Device, state: boolean) => this.onDeviceLocked(device, state));
                device.on("open", (device: Device, state: boolean) => this.onDeviceOpen(device, state));
                device.on("ready", (device: Device) => this.onDeviceReady(device));
                this.addDevice(device);
                return device;
            }).catch((device: Device) => {
                this.log.error("Error", device);
                return device;
            }));
        }
    }
    this.loadingDevices = Promise.all(promises).then((devices) => {
        devices.forEach((device) => {
            const station = this.getStation(device.getStationSerial());
            if (!station.isConnected()) {
                station.setConnectionType(this.config.p2pConnectionSetup);
                station.connect();
            }
        });
        this.loadingDevices = undefined;
    });

and the corresponding addDevice method:

private addDevice(device: Device): void {
    const serial = device.getSerial()
    if (serial && !Object.keys(this.devices).includes(serial)) {
        this.devices[serial] = device;
        this.emit("device added", device);


        if (device.isLock())
            this.mqttService.subscribeLock(device.getSerial());
    } else {
        throw new DuplicateDeviceError(`Device with this serial ${device.getSerial()} exists already and couldn't be added again!`);
    }
}

If the function throws a DuplicateDeviceError. This error is in sequence treated as a device itself (lines 509-511).
Since this error object doesn't have the needed methods for a device the following loadingDevices method will crash.

To reproduce

I have this issue in my quick and dirty tool (https://github.com/thieren/eufy-test-client) even though I can't quite tell why.
The first login works fine but any subsequent attempt to connect crashes.

The homebridge-eufy-security client which establishes the connection just the same works fine it seems.

Screenshots & Logfiles

TypeError: device.getStationSerial is not a function
    at /home/rene/dev/etc/node_modules/eufy-security-client/build/eufysecurity.js:477:56
    at Array.forEach (<anonymous>)
    at /home/rene/dev/etc/node_modules/eufy-security-client/build/eufysecurity.js:476:21
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async EufySecurity.getDevices (/home/rene/dev/etc/node_modules/eufy-security-client/build/eufysecurity.js:276:13)
    at async EufyPlatform.updateDevices (/home/rene/dev/etc/index.js:299:25)
    at async EufySecurity.<anonymous> (/home/rene/dev/etc/index.js:143:7)

Additional context

No response

[thieren]

Actually it seems to affect the homebridge plugin also.
I think the DuplicateDevicesError was already present in earlier versions. But it was not until the latest commit to fix #161 that caused this to crash the whole thing.

[bropat]

If the function throws a DuplicateDeviceError. This error is in sequence treated as a device itself (lines 509-511).
Since this error object doesn't have the needed methods for a device the following loadingDevices method will crash.

You're right about that. But the reason is also a race condition caused by this line in your test client. The EufySecurity class has already implemented a refresh mechanism itself. This is controlled by the parameter pollingIntervalMinutes of the EufySecurityConfig. In your example, the cloud is queried async twice and this bug occurs.

I will fix it soon, but as I said, if you remove your refresh mechanism, this problem should no longer occur.

[thieren]

Thank you for auditing.

Yeah. Didn't check if refresh was really necessary and copied it from other sources nevertheless.

As said quick and (very) dirty. But helps me a lot to test things and dive a little deeper.

[bropat]

Same thing here.

[thieren]

But the reason is also a race condition caused by this line in your test client.

I have another question regarding this, since this means that one can not retrieve the station and device list right away.
So, since you shouldn't use the refreshCloudData method in the first place, how are you supposed to get a stations and device list at the start of the program?
My assumption would be, that you'll consider an async approach and waiting on 'device added' and 'station added' events is best practice? Am I right?
I'd have to check if this is possible with the homebridge plugin, since the general way would be to register the accessories right at start up.

Alternatively, would it be feasible to emit an event after the first call to refreshCloudData has read all the devices?

Lastly: if calling this method yourself is bad practice, wouldn't it be better to restrict access by setting it to private? (of course this would be breaking, but maybe it's a consideration worth making for the future)

[bropat]

So, since you shouldn't use the refreshCloudData method in the first place, how are you supposed to get a stations and device list at the start of the program?

The connect event is now emitted after the internal refresh.

My assumption would be, that you'll consider an async approach and waiting on 'device added' and 'station added' events is best practice? Am I right?

Right.
There are also "old" methods that I would like to remove in the next major release.

Alternatively, would it be feasible to emit an event after the first call to refreshCloudData has read all the devices?

You could now also use the connect event ;)