Mqttnet: AtLeastOnce is incorrectly sending PubAck on exceptions

[ramonsmits]

The method FireApplicationMessageReceivedEvent is swallowing exceptions instead of throwing. This results in the PubAck to always be send. This AFAIK shouldn't be done on exceptions making sure that the message eventually will be processed.

https://github.com/chkr1011/MQTTnet/blob/195357835b258c8c043ed32fa10ad30e12d6a45f/Source/MQTTnet/Client/MqttClient.cs#L459-L463

https://github.com/chkr1011/MQTTnet/blob/195357835b258c8c043ed32fa10ad30e12d6a45f/Source/MQTTnet/Client/MqttClient.cs#L516-L527

I think the exception handling should not be occurring in FireApplicationMessageReceivedEvent but in ProcessReceivedPublishPacketAsync where if qos IS NOT equal to MqttQualityOfServiceLevel.AtMostOnce to throw or to have FireApplicationMessageReceivedEvent return a bool so that ProcessReceivedPublishPacketAsync could be:

c# if (publishPacket.QualityOfServiceLevel == MqttQualityOfServiceLevel.AtLeastOnce) { if(FireApplicationMessageReceivedEvent(publishPacket)) { return SendAsync(new MqttPubAckPacket { PacketIdentifier = publishPacket.PacketIdentifier }, cancellationToken); { return Task.CompletedTask; } }

[chkr1011]

Hi,
thank you for sharing this issue. You are right. This has to be changed. I fixed it via moving the exception handler as you suggested. This will return from the method without sending the ACK.
Best regards
Christian

[ramonsmits]

As this is a critical bug, any idea when this is going to be released? The current version on Nuget doesn't have it.

[SeppPenner]

@chkr1011 Does version 3 already contain this fix?