Zwavejs2mqtt: [bug] Zwavejs2mqtt is listening to more topics than needed

[LordMike]

Version

Build/Run method

• [ ] Docker
• [ ] PKG
• [x] Manually built (git clone - npm install - npm run build )

zwavejs2mqtt version: 1.0.0-rc.1+423a7e7.423a7e7
zwavejs version: 6.1.0-5d49c45

Describe the bug

I was playing with an API, and noticed that the debug log showed the API response as received by zwavejs2mqtt itself. This seems excessive, and unecessary. So I think this app is subscribing to too many/broad topics.

Example log where I publish a message to a */set topic, after which the API response is received by the app.

2021-01-26 23:22:31.264 INFO MQTT: Message received on zwavejs2mqtt/_CLIENTS/ZWAVE_GATEWAY-HomeMQTT/api/pollValue/set, 'blah'
2021-01-26 23:22:31.266 INFO ZWAVE: Cannot read property 'nodeId' of undefined pollValue
2021-01-26 23:22:31.268 INFO MQTT: Message received on zwavejs2mqtt/_CLIENTS/ZWAVE_GATEWAY-HomeMQTT/api/pollValue, `{"success":false,"message":"Cannot read property 'nodeId' of undefined","args":[]}`

To Reproduce

Publish a message to zwavejs2mqtt/_CLIENTS/ZWAVE_GATEWAY-HomeMQTT/api/pollValue and observe the app reacting to it.

Also, publishing to ../set produces two received messages - both the command, and the response itself.

Expected behavior

Zwavejs2mqtt does _not_ subscribe to topics it doesn't need.

Additional context

N/A

[LordMike]

I found this code, which should always subscribe to a /set variant, so I'm not sure why a non-/set is seen.

https://github.com/zwave-js/zwavejs2mqtt/blob/ade28bd5c1ea8d525786b30b771ca5d3a7b72568/lib/MqttClient.js#L324-L332

Also. Isn't there a bug here?.. If the client is _not_ connected, the topic is pushed to the toSubscribe list, but that list is itself iterated when connected:

https://github.com/zwave-js/zwavejs2mqtt/blob/ade28bd5c1ea8d525786b30b771ca5d3a7b72568/lib/MqttClient.js#L127-L131

So if a connection is dropped _while_ subscribing, the remaining topics are added to the list itself, potentially creating an infinite loop.

[LordMike]

Oh - I found the place.

https://github.com/zwave-js/zwavejs2mqtt/blob/ade28bd5c1ea8d525786b30b771ca5d3a7b72568/lib/MqttClient.js#L137-L143

So the /api/ topics are too broadly subscribed to. As ACTIONS are broadcast and api, you could change this to:

this.client.subscribe([this.config.prefix, CLIENTS_PREFIX, this.clientID, 'api', '+', 'set'].join('/');
this.client.subscribe([this.config.prefix, CLIENTS_PREFIX, this.clientID, 'broadcast', '+', 'set'].join('/');
this.client.subscribe([this.config.prefix, CLIENTS_PREFIX, this.clientID, 'broadcast', '+', '+', 'set'].join('/');
this.client.subscribe([this.config.prefix, CLIENTS_PREFIX, this.clientID, 'broadcast', '+', '+', '+', 'set'].join('/');

[LordMike]

There's potentially another bug. The toSubscribe is cleaned after the connected event, which is fine, because the next time the client connects (after a disconnect), the broker will maintain subscriptions .... if the session is persistent (non-clean).

https://github.com/zwave-js/zwavejs2mqtt/blob/ade28bd5c1ea8d525786b30b771ca5d3a7b72568/lib/MqttClient.js#L59-L70

But clean can be set via the config, so if a user sets config.clean to false, and the broker connection is lost - I suspect no commands will be received by zwavejs2mqtt, as no new subscriptions are made.

[robertsLando]

So the /api/ topics are too broadly subscribed to. As ACTIONS are broadcast and api, you could change this to:

This wouldn't cover the case when the gateway is in 'manual' mode. The topic could have infinite levels. Having a # subscribe isn't that bed as messages without /set are simply ignored

[robertsLando]

I suspect no commands will be received by zwavejs2mqtt, as no new subscriptions are made.

Why this? The subscriptions are restored both if clean is true or false. The only difference is that if clean is false it will also receive messages while it was offline as the session is persistent

[robertsLando]

So if a connection is dropped while subscribing, the remaining topics are added to the list itself, potentially creating an infinite loop.

That's almost impossible as everything is sync in that part but I could fix it by deep copy the subscribe object

[LordMike]

I suspect no commands will be received by zwavejs2mqtt, as no new subscriptions are made.

Why this? The subscriptions are restored both if clean is true or false. The only difference is that if clean is false it will also receive messages while it was offline as the session is persistent

As I understand it, in a clean session:

1. Connection is made
2. No subscriptions are kept, broker expects client to subscribe
3. Z2m will _not_ resubscribe, as toSubscribe is now empty

I just tested on mosquitto, that clean sessions mean the subscriptions are disregarded. Whereas a non-clean session will keep subscriptions (without resubscribing).

I'll see if I can reproduce this in z2m.

[LordMike]

It's odd. Clean sessions are enabled in my settings page, but when I restarted my broker and pushed a message - it was received after the reconnect as well.. I'm not entirely sure why this works - but it's probably fine. :O

2021-01-27 18:34:40.189 INFO MQTT: Message received on zwavejs2mqtt/_CLIENTS/ZWAVE_GATEWAY-HomeMQTT/api/blah, 'test'
2021-01-27 18:34:59.441 INFO MQTT: MQTT client offline
2021-01-27 18:34:59.441 INFO MQTT: MQTT client closed
2021-01-27 18:35:04.441 INFO MQTT: MQTT client reconnecting
2021-01-27 18:35:04.455 INFO MQTT: MQTT client connected
2021-01-27 18:35:07.234 INFO MQTT: Message received on zwavejs2mqtt/_CLIENTS/ZWAVE_GATEWAY-HomeMQTT/api/blah, 'test'

[LordMike]

Aha!.. Node-js mqtt:

From v2.0.0, subscriptions are restored upon reconnection if clean: true

We're good. :)