1546 possible bug in server repeatedly sending the same message to client subscribed with cleansession=false (#1569)

Source/MQTTnet/Server/Internal/MqttClient.cs

@@ -507,15 +507,15 @@ namespace MQTTnet.Server
                     try
                     {
                         await SendPacketAsync(packetBusItem.Packet, cancellationToken).ConfigureAwait(false);
-                        packetBusItem.MarkAsDelivered();
+                        packetBusItem.Complete();
                     }
                     catch (OperationCanceledException)
                     {
-                        packetBusItem.MarkAsCancelled();
+                        packetBusItem.Cancel();
                     }
                     catch (Exception exception)
                     {
-                        packetBusItem.MarkAsFailed(exception);
+                        packetBusItem.Fail(exception);
                     }
                     finally
                     {
@@ -563,7 +563,7 @@ namespace MQTTnet.Server
             catch (ObjectDisposedException)
             {
                 // This can happen when connections are created and dropped very quickly.
-                // It is not an issue if the cancellation token cannot be cancelled multiple times.
+                // It is not an issue if the cancellation token cannot be canceled multiple times.
             }
         }
 

Source/MQTTnet/Server/Internal/MqttSession.cs

@@ -4,7 +4,6 @@
 
 using System;
 using System.Collections;
-using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.Linq;
 using System.Threading;
@@ -25,7 +24,8 @@ namespace MQTTnet.Server
         readonly MqttServerOptions _serverOptions;
         readonly MqttClientSubscriptionsManager _subscriptionsManager;
 
-        readonly ConcurrentDictionary<ushort, MqttPublishPacket> _unacknowledgedPublishPackets = new ConcurrentDictionary<ushort, MqttPublishPacket>();
+        // Do not use a dictionary in order to keep the ordering of the messages.
+        readonly List<MqttPublishPacket> _unacknowledgedPublishPackets = new List<MqttPublishPacket>();
 
         // Bookkeeping to know if this is a subscribing client; lazy initialize later.
         HashSet<string> _subscribedTopics;
@@ -72,7 +72,14 @@ namespace MQTTnet.Server
 
         public MqttPublishPacket AcknowledgePublishPacket(ushort packetIdentifier)
         {
-            _unacknowledgedPublishPackets.TryRemove(packetIdentifier, out var publishPacket);
+            MqttPublishPacket publishPacket;
+
+            lock (_unacknowledgedPublishPackets)
+            {
+                publishPacket = _unacknowledgedPublishPackets.FirstOrDefault(p => p.PacketIdentifier.Equals(packetIdentifier));
+                _unacknowledgedPublishPackets.Remove(publishPacket);
+            }
+
             return publishPacket;
         }
 
@@ -130,7 +137,10 @@ namespace MQTTnet.Server
             {
                 publishPacket.PacketIdentifier = _packetIdentifierProvider.GetNextPacketIdentifier();
 
-                _unacknowledgedPublishPackets[publishPacket.PacketIdentifier] = publishPacket;
+                lock (_unacknowledgedPublishPackets)
+                {
+                    _unacknowledgedPublishPackets.Add(publishPacket);
+                }
             }
 
             _packetBus.EnqueueItem(packetBusItem, MqttPacketBusPartition.Data);
@@ -145,8 +155,10 @@ namespace MQTTnet.Server
         {
             // This will only return the matching PUBLISH packet but does not remove it.
             // This is required for QoS 2.
-            _unacknowledgedPublishPackets.TryGetValue(packetIdentifier, out var publishPacket);
-            return publishPacket;
+            lock (_unacknowledgedPublishPackets)
+            {
+                return _unacknowledgedPublishPackets.FirstOrDefault(p => p.PacketIdentifier.Equals(packetIdentifier));
+            }
         }
 
         public void Recover()
@@ -154,9 +166,33 @@ namespace MQTTnet.Server
             // TODO: Keep the bus and only insert pending items again.
             // TODO: Check if packet identifier must be restarted or not.
             // TODO: Recover package identifier.
+            
+            /*
+                The Session state in the Client consists of:
+                ·         QoS 1 and QoS 2 messages which have been sent to the Server, but have not been completely acknowledged.
+                ·         QoS 2 messages which have been received from the Server, but have not been completely acknowledged. 
+
+                The Session state in the Server consists of:
+                ·         The existence of a Session, even if the rest of the Session state is empty.
+                ·         The Client’s subscriptions.
+                ·         QoS 1 and QoS 2 messages which have been sent to the Client, but have not been completely acknowledged.
+                ·         QoS 1 and QoS 2 messages pending transmission to the Client.
+                ·         QoS 2 messages which have been received from the Client, but have not been completely acknowledged.
+                ·         Optionally, QoS 0 messages pending transmission to the Client. 
+             */
+
+            // Create a copy of all currently unacknowledged publish packets and clear the storage.
+            // We must re-enqueue them in order to trigger other code.
+            List<MqttPublishPacket> unacknowledgedPublishPackets;
+            lock (_unacknowledgedPublishPackets)
+            {
+                unacknowledgedPublishPackets = _unacknowledgedPublishPackets.ToList();
+                _unacknowledgedPublishPackets.Clear();    
+            }
+
             _packetBus.Clear();
 
-            foreach (var publishPacket in _unacknowledgedPublishPackets.Values.ToList())
+            foreach (var publishPacket in unacknowledgedPublishPackets)
             {
                 EnqueueDataPacket(new MqttPacketBusItem(publishPacket));
             }

Source/MQTTnet/Server/MqttServerExtensions.cs

@@ -3,29 +3,89 @@
 // See the LICENSE file in the project root for more information.
 
 using System;
+using System.Text;
 using System.Threading.Tasks;
+using MQTTnet.Internal;
 using MQTTnet.Packets;
+using MQTTnet.Protocol;
 
 namespace MQTTnet.Server
 {
     public static class MqttServerExtensions
     {
+        public static Task InjectApplicationMessage(
+            this MqttServer server,
+            string topic,
+            string payload = null,
+            MqttQualityOfServiceLevel qualityOfServiceLevel = MqttQualityOfServiceLevel.AtMostOnce,
+            bool retain = false)
+        {
+            if (server == null)
+            {
+                throw new ArgumentNullException(nameof(server));
+            }
+
+            if (topic == null)
+            {
+                throw new ArgumentNullException(nameof(topic));
+            }
+
+            var payloadBuffer = EmptyBuffer.Array;
+            if (payload is string stringPayload)
+            {
+                payloadBuffer = Encoding.UTF8.GetBytes(stringPayload);
+            }
+
+            return server.InjectApplicationMessage(
+                new InjectedMqttApplicationMessage(
+                    new MqttApplicationMessage
+                    {
+                        Topic = topic,
+                        Payload = payloadBuffer,
+                        QualityOfServiceLevel = qualityOfServiceLevel,
+                        Retain = retain
+                    }));
+        }
+
         public static Task SubscribeAsync(this MqttServer server, string clientId, params MqttTopicFilter[] topicFilters)
         {
-            if (server == null) throw new ArgumentNullException(nameof(server));
-            if (clientId == null) throw new ArgumentNullException(nameof(clientId));
-            if (topicFilters == null) throw new ArgumentNullException(nameof(topicFilters));
+            if (server == null)
+            {
+                throw new ArgumentNullException(nameof(server));
+            }
+
+            if (clientId == null)
+            {
+                throw new ArgumentNullException(nameof(clientId));
+            }
+
+            if (topicFilters == null)
+            {
+                throw new ArgumentNullException(nameof(topicFilters));
+            }
 
             return server.SubscribeAsync(clientId, topicFilters);
         }
-        
+
         public static Task SubscribeAsync(this MqttServer server, string clientId, string topic)
         {
-            if (server == null) throw new ArgumentNullException(nameof(server));
-            if (clientId == null) throw new ArgumentNullException(nameof(clientId));
-            if (topic == null) throw new ArgumentNullException(nameof(topic));
+            if (server == null)
+            {
+                throw new ArgumentNullException(nameof(server));
+            }
 
-            return server.SubscribeAsync(clientId, new MqttTopicFilterBuilder().WithTopic(topic).Build());
+            if (clientId == null)
+            {
+                throw new ArgumentNullException(nameof(clientId));
+            }
+
+            if (topic == null)
+            {
+                throw new ArgumentNullException(nameof(topic));
+            }
+
+            var topicFilters = new MqttTopicFilterBuilder().WithTopic(topic).Build();
+            return server.SubscribeAsync(clientId, topicFilters);
         }
     }
-}
+}

Source/MQTTnet/Server/Status/MqttSessionStatus.cs

@@ -45,7 +45,7 @@ namespace MQTTnet.Server
             var packetBusItem = new MqttPacketBusItem(publishPacketFactory.Create(applicationMessage));
             _session.EnqueueDataPacket(packetBusItem);
 
-            return packetBusItem.WaitForDeliveryAsync();
+            return packetBusItem.WaitAsync();
         }
         
         public Task DeleteAsync()