Auto-ack of messages in router.go renders API call mqtt.Message.Ack() useless

[tonygunter]

I attempted to verify that I could consume a message and pass it to a worker for processing. If the processing successfully completes, only then do I want to ack the message. However, I notice that the messages are acked during consumption by router.go in three places inside the matchAndDispatch function.

I believe this is a bug. If the intended purpose is to ack the message only after the message has been completely processed, then these internal acks are bypassing that functionality. If the intended purpose is to ack the message immediately after consumption but before processing, then exposing mqtt.Message.Ack() serves no purpose and executes no code (because it wraps a once.Do({}) clause).

[tonygunter]

This bug is similar to #396, but the description of the problem is different. I'm consuming my messages then passing them off to worker processes on a go channel, and expecting to be able to Ack() them there. He's requesting custom consumers or error-based execution of auto-ack. I suspect that automatic acks by router.go are the defect, not the solution.

[MattBrittan]

I think the two issues are essentially the same. Unfortunately the automatic call to Ack is required because removing it would silently break existing code (that is what happened when the change was initially released). I don't think that adding a return value to MessageHandler is an option (would break existing code) so I see the options as follows:

• Add a new option (say NoAutoAck which defaults to false).
• Add an interface (say AutoAcker with the function AutoAckOff()) that PublishPacket implements - calling this would deactivate the automatic ack for the particular message (a slightly messy approach but avoids adding yet another option).

[alsm]

This is an interesting one, because by the spec the original behaviour is correct. Once the client library has successfully received the message it should acknowledge it, the resend mechanism isn't designed as a feature to be used by applications, it's to ensure that messages successfully pass the network.

[tonygunter]

@alsm I think you're correct. Receiver initiates the onward delivery of the application message, discards the packet identifier, and issues PUBCOMP. I suppose that means mqtt.Message.Ack() should not be exposed to the public interface?

[h12w]

Manual Ack is not about correctness but the efficiency of the message processing. Most message processing involves persisting the results into a database or another message queuing system, where high throughput could be only achieved by batches/transactions with a reasonable size. Processing message one by one is correct but very inefficient.

With automatic ack always enabled, the only way to work around this seems to send the messages to a fast in-memory queue like Redis, automatically ack and start the real processing from there.

[MattBrittan]

Based on the comments on the v5 repo I'm happy to accept a PR for this. Any implementation will need to allow for the possibility of the connection dropping and reconnecting before the ACK is requested (in that case the broker should resend the packet when the connection comes up again so the ACK should probably just be dropped). Note that the proposed changes to the V5 client ensure the the ACK packets are sent in the correct order; while this is a requirement of the MQTT spec its not something I'd insist on (its not currently guaranteed and could cause issues when one ACK is delayed and there is a limit to the number of messages in flight).

[shivamkm07]

Based on the comments on the v5 repo I'm happy to accept a PR for this. Any implementation will need to allow for the possibility of the connection dropping and reconnecting before the ACK is requested (in that case the broker should resend the packet when the connection comes up again so the ACK should probably just be dropped). Note that the proposed changes to the V5 client ensure the the ACK packets are sent in the correct order; while this is a requirement of the MQTT spec its not something I'd insist on (its not currently guaranteed and could cause issues when one ACK is delayed and there is a limit to the number of messages in flight).

@MattBrittan PR for the same is opened for review. Thanks!

[MattBrittan]

Thanks for the PR (#578 ) @shivamkm07. I added the below comments to that PR and thought I'd copy them here as I suspect they will reach more people and I'm really keen to get input on this. These notes/issues 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 (with #578) 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). I think a comment is needed to ensure users are aware of this.

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.

If the consensus is that the approach in the PR works for everyone then I'm happy to move forward with it (subject to the addition of some comments covering the above issues).

[shivamkm07]

Updated the associated PR with adding an option in ClientOptions for disabling the auto-ack.