eclipse-paho / paho.mqtt.golang

Other
2.77k stars 534 forks source link

Autoreconnect seems to set clean session flag? #612

Closed eldondevat closed 10 months ago

eldondevat commented 2 years ago

We have configured our clients with autoreconnect, and customized various intervals and timeouts. It seems to me, although I haven't build a comprehensive test setup, that the CleanSession flag will be set from client options regardless of whether we are in an automatic reconnection or an initial connection. This had the unexpected consequence that, after a network outage we recovered from, our subscriptions made on the initial connection seemed no longer to be active. It occurs to me that a client automatic reconnect should set a CleanSession to 0 to preserve subscriptions. Perhaps if a clean session is the expected behavior, documentation could be added to the autoreconnect functionality.

MattBrittan commented 2 years ago

The handling of Cleansession is tested here; as far as I'm aware it works (in fact my production code relies upon messages being sent after multi-day network outages). Having said that this does depend upon a range of things (e.g. QOS0 messages will not be resent, subscriptions must be QOS1+, you must use the same client ID etc).

If you believe there is an issue then I'll need considerably more information than you posted; please see the relevant section of the readme. It should be fairly easy to use the docker example to replicate your setup (the readme in that example even has a section on "Simulating Network Connection Loss" which was written when I was testing your exact use-case :-) ).

eldondevat commented 2 years ago

@MattBrittan Thanks much for your response. Perhaps this is simply a usability or documentation topic. I think the crux of my confusion around the AutoReconnect functionality is the need for this bit. The combination of AutoReconnect and the default of having CleanSession set implies that one must resubscribe to all previous subscriptions each time a connection happens, which was unexpected to me.

I can simply set CleanSession to false in our application code, but it seemed odd to me that NewClientOptions would set both AutoReconnect and CleanSession by default. In our application we spin out a number of goroutines performing different tasks after connecting to AWS IoT, and so building resubscribe logic into each of them would likely add complexity. Currently we rely on the MQTT broker to manage whether or not we are subscribed to '/thing/job/12345678/events`, for example. This got me thinking how long AWS keeps these around for, which appears to default to 1 hour, and max out at 7 days.

I am still trying to grok the breadth of use-cases here. The requirement to re-subscribe has surprised me, maybe a doc change would help to prevent surprises for others? Is resubscribing each time a best practice due to broker quirks? Thank you for your help, I gave a look at the docs and tests you referenced, please let me know if I have still missed something.

MattBrittan commented 2 years ago

As you say if CleanSession is set to true (the default) then the broker will not store subscriptions if the connection is lost.

There is some logic when reconnection is active that retains stuff (e.g. this). However this is never something I've looked into in any real detail (I suspect it was put in place so messages submitted when the connection was down are transmitted when the connection comes back up); I suspect there are issues with this (but as I always set CleanSession(false) its not something I've put any time into.

The requirement to re-subscribe has surprised me

This only really applies if CleanSession is set to true. Having said that I think subscribing every time is safer (otherwise you will need special logic for the first connection and there may be circumstances when the broker loses your session info).

Unfortunately the defaults predate my involvement with the project and I've been reluctant to change them (as that would be a breaking change). Due to the age of this library there is quite a bit of old code that has been retained solely to avoid breaking changes (I really don't know what the aim was when it was written).

eldondevat commented 2 years ago

but as I always set CleanSession(false) its not something I've put any time into.

This is great information, thank you! It's always good to understand the primary use case for the folks core to the project.

solely to avoid breaking changes

Yes, of course something deployed in the field like we have here instead of the cloud benefits from extra care with backwards compatibility. I hope to open a PR expanding the docs on some of the client options bits to address this if that is reasonable.

MattBrittan commented 2 years ago

I hope to open a PR expanding the docs on some of the client options bits to address this if that is reasonable.

Absolutely - always happy to see improvements to the documentation (and having other view points is always helpful!). Unfortunately a log of users do struggle to understand MQTT persistence before you add in the peculiarities of this package.

MattBrittan commented 10 months ago

Closing this due to age/inactivity - as mentioned above any improvements to the documentation would be welcomed.