Support for AutoAckOff

[shivamkm07]

This is to support disabling of Automatic Acking of messages by mqtt client. A similar solution has been merged in V5 client(eclipse-paho/paho.golang@a47276e). While V5 solution also takes care of Acks being sent in order, this doesn't seem a necessity as discussed here.

Issue Resolved: #459

[MattBrittan]

Thanks for the PR - before I review it can you please sign the ECA (please let me know if you have any issues). Unfortunately I cannot accept a PR without a signed ECA (the aim being to ensure that all contributions are appropriately licensed).

[shivamkm07]

Thanks for the PR - before I review it can you please sign the ECA (please let me know if you have any issues). Unfortunately I cannot accept a PR without a signed ECA (the aim being to ensure that all contributions are appropriately licensed).

Done!

[MattBrittan]

Thanks very much for the PR. I have taken a quick look today and list a few concerns below; please note that these may seem fussy but I think its important that we get this right (once live it is very difficult to change the API and I want to minimise future issues caused by users not understanding how this all works).

It's probably possible to address these concerns by documenting the Message interface to ensure users are aware of the potential pitfalls. Given we don't want to make this a breaking change I think that documenting the potential issues may all we can do unless an alternative approach is adopted.

My concerns are:

• Multiple handlers may be fired when a message is received; in this case:
  • If SetOrderMatters(true) (default) then the ACK will be sent if the first handler called does not call AutoAckOff() (regardless of the actions of any further handlers).
  • If SetOrderMatters(false) (recommended in the docs) then there is a potential race condition (handlers are called in go routines).
• If the MessageHandler calls AutoAckOff(), and then returns after starting a go routine that eventually calls ACK() then this will lead to a deadlock/unexpected results (just need a warning that ACK must not be called after the handler has returned).
• Where a message is not acknowledged the broker will often only resend it when the connection is re-established (the v3 spec does not specify this but it's what Mosquitto does and what the v5 spec states).

Adding this feature is likely to result in longer running handlers so I feel that its important to note:

• Users need to be aware that long running handlers can cause issues (particularly when SetOrderMatters(true)). For example the client will not reconnect until all handlers exit (and I suspect that keep alives may not be sent).
• Due to the way this library handles QOS2 messages duplicates are possible (particularly with long running handlers).

Given these concerns I did consider whether we are going about this in the right way; alternatives would include:

• A ClientOptions that turns off all automatic ACK's globally. This would very simple to implement but removes the safety net that automatic ACKs provide.
• Provide a way to flag a route such that messages received on it are not automatically acknowledged. The API for this would require some thought.

Note: I'll copy this to the issue because it would be good to get feedback from users on this.

[Volcore]

Hi @MattBrittan and @shivamkm07,

thanks for the PR! I've been running into a similar issue, and stumbled over the auto-ack. Coming from other message passing platforms, like Kafka and PubSub, I was very surprised that auto-ack'ing is even a thing (afaicr it's not even an option for Google's official PubSub library), especially since the Message.Ack() is essentially pointless because of that (at least to the end user of the library).

My preferred option would be to have a ClientOption to at least turn it off (but ideally, not even auto-ack to begin with, but I understand this will create compatibility issues). I'm happy to give a PR for this a go if you two are busy.

One crucial piece that MQTT brokers might be missing to make this really useful is some kind of backoff feature to delay re-delivering messages longer and longer if writes fail (so that we aren't spamming all of our server instances constantly with the same message upon failure), but that's beyond the scope of the client library I think.

[shivamkm07]

Hi @MattBrittan and @Volcore, Thanks for reviewing the PR.

I agree adding AutoAckOff in Message could lead to race issues, and also it would be redundant to do it for every message. Adding it in ClientOptions would disable the AutoAck once globally and then users would be able to Ack the messages as per the handler logic. Therefore, I have updated the PR with AutoAck as an option in ClientOptions. By default, it is set to true, and the user could set it to false for disabling the automated acking. Let me know if this looks good.

[MattBrittan]

Thanks very much for implementing this (sorry it took a few attempts!).

[shivamkm07]

@MattBrittan Is there any expected timeline at which the next patch release is scheduled? That would be better than using the latest master commit.

[MattBrittan]

Done - I had been holding off because of the major changes to status handling but as they have been in @master for a couple of months without any issues being raised I felt it was worth making a release.

[shivamkm07]

Thanks @MattBrittan!!!