search

eclipse-paho / paho.mqtt.golang

Other
2.79k stars 534 forks source link

disabling AutoReconnect causes spurious error to be logged #697

Open rittneje opened 3 days ago

rittneje commented 3 days ago

We set AutoReconnect to false in our ClientOptions. When the connection to the broker is lost unexpectedly, we see the following error come from this library:

[client] BUG BUG BUG reconnection function is nil

Tracing through the code, this is because disDone is assigned like so. https://github.com/eclipse-paho/paho.mqtt.golang/blob/714f7c0231294ec4144f0e0e5fc5b43a6d430d2f/client.go#L517

Since c.options.AutoReconnect is false, that will pass false to c.status.ConnectionLost. Thus the callback that it returns will itself return nil, nil. https://github.com/eclipse-paho/paho.mqtt.golang/blob/714f7c0231294ec4144f0e0e5fc5b43a6d430d2f/status.go#L278-L280

This in turn triggers the log that "should never happen". https://github.com/eclipse-paho/paho.mqtt.golang/blob/714f7c0231294ec4144f0e0e5fc5b43a6d430d2f/client.go#L547-L552

I believe this regression was introduced in v1.4.2. It prevents us from upgrading from v1.3.5.

stoxx commented 3 days ago

I've assumed it's harmless.

MattBrittan commented 3 days ago

The library has quite a few options and I was concerned that I may have missed some possible combinations when rearchitecting the status handling (it was a bit of a mess), hence the "BUG BUG" log entry. Have had a quick look and the log entry should be supressed if c.options.AutoReconnect == false (probably best to add something like reconnectExpected := c.options.AutoReconnect && c.status.ConnectionStatus() > connecting at the top, and then use that value). Having said that the code looks like it will work OK, it's just an incorrect log entry. Will fix when I get a chance (or would welcome a PR).