From 26cd84c88c177ac685a0976377de9453a74af416 Mon Sep 17 00:00:00 2001 From: Christian Date: Fri, 2 Feb 2018 00:46:47 +0100 Subject: [PATCH] Fix generation of packet identifier. --- Build/MQTTnet.nuspec | 1 + .../MQTTnet.NetStandard/Client/MqttClient.cs | 17 ++++------ .../Client/MqttPacketIdentifierProvider.cs | 32 +++++++++++++++++++ .../MqttPacketIdentifierProviderTests.cs | 32 +++++++++++++++++++ 4 files changed, 71 insertions(+), 11 deletions(-) create mode 100644 Frameworks/MQTTnet.NetStandard/Client/MqttPacketIdentifierProvider.cs create mode 100644 Tests/MQTTnet.Core.Tests/MqttPacketIdentifierProviderTests.cs diff --git a/Build/MQTTnet.nuspec b/Build/MQTTnet.nuspec index 9603423..9c2e0d3 100644 --- a/Build/MQTTnet.nuspec +++ b/Build/MQTTnet.nuspec @@ -12,6 +12,7 @@ MQTTnet is a high performance .NET library for MQTT based communication. It provides a MQTT client and a MQTT server (broker). * [Core] Fixed some still thread blocking parts in the code (thanks to @kpreisser). * [Core] Updated 3rd-Party packages. +* [Core] Fixed wrong packet identifier calculation (thanks to @benpittoors). * [Core] Fixed an issue when reading the body of a package from a disconnected sender (thanks to @kpreisser). * [Core] Fixed wrong parsing of the body length (thanks to @kpreisser). * [Client] The client interfaces are now implementing _IDisposable_. diff --git a/Frameworks/MQTTnet.NetStandard/Client/MqttClient.cs b/Frameworks/MQTTnet.NetStandard/Client/MqttClient.cs index a4ec82f..f71ada1 100644 --- a/Frameworks/MQTTnet.NetStandard/Client/MqttClient.cs +++ b/Frameworks/MQTTnet.NetStandard/Client/MqttClient.cs @@ -14,13 +14,13 @@ namespace MQTTnet.Client { public class MqttClient : IMqttClient { + private readonly MqttPacketIdentifierProvider _packetIdentifierProvider = new MqttPacketIdentifierProvider(); private readonly IMqttClientAdapterFactory _adapterFactory; private readonly MqttPacketDispatcher _packetDispatcher; private readonly IMqttNetLogger _logger; private IMqttClientOptions _options; private bool _isReceivingPackets; - private int _latestPacketIdentifier; private CancellationTokenSource _cancellationTokenSource; private IMqttChannelAdapter _adapter; @@ -49,7 +49,7 @@ namespace MQTTnet.Client { _options = options; _cancellationTokenSource = new CancellationTokenSource(); - _latestPacketIdentifier = 0; + _packetIdentifierProvider.Reset(); _packetDispatcher.Reset(); _adapter = _adapterFactory.CreateClientAdapter(options.ChannelOptions, _logger); @@ -106,7 +106,7 @@ namespace MQTTnet.Client var subscribePacket = new MqttSubscribePacket { - PacketIdentifier = GetNewPacketIdentifier(), + PacketIdentifier = _packetIdentifierProvider.GetNewPacketIdentifier(), TopicFilters = topicFilters.ToList() }; @@ -128,7 +128,7 @@ namespace MQTTnet.Client var unsubscribePacket = new MqttUnsubscribePacket { - PacketIdentifier = GetNewPacketIdentifier(), + PacketIdentifier = _packetIdentifierProvider.GetNewPacketIdentifier(), TopicFilters = topicFilters.ToList() }; @@ -156,7 +156,7 @@ namespace MQTTnet.Client { foreach (var publishPacket in qosGroup) { - publishPacket.PacketIdentifier = GetNewPacketIdentifier(); + publishPacket.PacketIdentifier = _packetIdentifierProvider.GetNewPacketIdentifier(); await SendAndReceiveAsync(publishPacket).ConfigureAwait(false); } @@ -166,7 +166,7 @@ namespace MQTTnet.Client { foreach (var publishPacket in qosGroup) { - publishPacket.PacketIdentifier = GetNewPacketIdentifier(); + publishPacket.PacketIdentifier = _packetIdentifierProvider.GetNewPacketIdentifier(); var pubRecPacket = await SendAndReceiveAsync(publishPacket).ConfigureAwait(false); await SendAndReceiveAsync(pubRecPacket.CreateResponse()).ConfigureAwait(false); } @@ -340,11 +340,6 @@ namespace MQTTnet.Client return (TResponsePacket)await packetAwaiter.ConfigureAwait(false); } - private ushort GetNewPacketIdentifier() - { - return (ushort)Interlocked.Increment(ref _latestPacketIdentifier); - } - private async Task SendKeepAliveMessagesAsync(CancellationToken cancellationToken) { _logger.Info("Start sending keep alive packets."); diff --git a/Frameworks/MQTTnet.NetStandard/Client/MqttPacketIdentifierProvider.cs b/Frameworks/MQTTnet.NetStandard/Client/MqttPacketIdentifierProvider.cs new file mode 100644 index 0000000..dc09918 --- /dev/null +++ b/Frameworks/MQTTnet.NetStandard/Client/MqttPacketIdentifierProvider.cs @@ -0,0 +1,32 @@ +namespace MQTTnet.Client +{ + public class MqttPacketIdentifierProvider + { + private readonly object _syncRoot = new object(); + private ushort _value; + + public void Reset() + { + lock (_syncRoot) + { + _value = 0; + } + } + + public ushort GetNewPacketIdentifier() + { + lock (_syncRoot) + { + _value++; + + if (_value == 0) + { + // As per official MQTT documentation the package identifier should never be 0. + _value = 1; + } + + return _value; + } + } + } +} diff --git a/Tests/MQTTnet.Core.Tests/MqttPacketIdentifierProviderTests.cs b/Tests/MQTTnet.Core.Tests/MqttPacketIdentifierProviderTests.cs new file mode 100644 index 0000000..ab7070e --- /dev/null +++ b/Tests/MQTTnet.Core.Tests/MqttPacketIdentifierProviderTests.cs @@ -0,0 +1,32 @@ +using Microsoft.VisualStudio.TestTools .UnitTesting; +using MQTTnet.Client; + +namespace MQTTnet.Core.Tests +{ + [TestClass] + public class MqttPacketIdentifierProviderTests + { + [TestMethod] + public void Reset() + { + var p = new MqttPacketIdentifierProvider(); + Assert.AreEqual(1, p.GetNewPacketIdentifier()); + Assert.AreEqual(2, p.GetNewPacketIdentifier()); + p.Reset(); + Assert.AreEqual(1, p.GetNewPacketIdentifier()); + } + + [TestMethod] + public void ReachBoundaries() + { + var p = new MqttPacketIdentifierProvider(); + + for (ushort i = 0; i < ushort.MaxValue; i++) + { + Assert.AreEqual(i + 1, p.GetNewPacketIdentifier()); + } + + Assert.AreEqual(1, p.GetNewPacketIdentifier()); + } + } +}