Koenkk / zigbee-herdsman

A Node.js Zigbee library
MIT License
485 stars 300 forks source link

fix: Clear the loaded device database entries when the controller is stopped #1131

Closed matt-oakes closed 4 months ago

matt-oakes commented 4 months ago

Hello! thanks again for all the work on this great library.

I have hit an edge case with the way we were using the library which was causing some strange issues for us.

Steps to reproduce

The issue stems from the fact that the Devices model is storing a cache of the loaded device database entries in the devices static member. This was causing an issue when we are:

  1. Stopping the zigbee-herdsman controller with controller.stop()
  2. Manually deleting a device from the database.
  3. Then later reconnecting the controller with controller.start

We're manually removing it because we need to allow users to remove devices even when the adapter is not connected. In situations where the adapter is connected we allow zigbee-herdsman to handle it.

All this is happening in an Electron program which is not closed between these steps.

Current behaviour

At the point of reconnecting the controller, we ask it for the list of devices. At that point Device.devices still contains the cached version so we are being returned devices that are no longer in the database.

For example, if the steps are:

  1. Start controller
  2. Add "device 1"
  3. Add "device 2"
  4. Stop the controller
  5. Delete "device 2" from the database manually
  6. Start the controller

At this point we are asking for the devices and getting back both "Device 1" and "Device 2".

Expected behaviour

I would expect that once the controller stops and restarts it essentially starts from a clean slate. This would allow us to restart the controller and know that it's going to respect the current state of the database.

In the example above, this would mean responding with just "Device 1".

What the fix does

I have resolved this in this PR by adding an additional step to clear the Device.devices cache when the controller stops. This measn the next time Device.loadFromDatabaseIfNecessary is called it will now load from the database again and get the correct new values.

Nerivec commented 4 months ago

Ok to incorporate this in https://github.com/Koenkk/zigbee-herdsman/pull/1130? Would need to do the same for Device.deletedDevices and Group.groups for consistent behavior across the board.

matt-oakes commented 4 months ago

Sure! Are you going to incorporate it or are you asking me to do that?

Related but much bigger potential change

In an ideal world for me the entire database concept would moved from zigbee-herdsman and into zigbee2mqtt.

I would replace it with an interface where herdsman can ask for the list of stored devices and pass through updates that should be saved.

This would make this library much easier to integrate with other systems as it is currently a little odd to have a library writing to the disk itself.

I understand why you’ve built it this way though, and know that’s a much bigger change, so I understand if your preference is to stick with the current design.

Nerivec commented 4 months ago

I'll do it 👍

matt-oakes commented 4 months ago

Ok. Thank you!

And thanks again for all the work on the library