From 731c89d2e90b4f409daa54116fbce42ebd911ccd Mon Sep 17 00:00:00 2001 From: logicaloud <47704763+logicaloud@users.noreply.github.com> Date: Mon, 29 Nov 2021 00:51:29 +1300 Subject: [PATCH] Fix MQTT5 'IsPersisted' session flag handling (#1300) --- .../Server/Internal/MqttClientSession.cs | 13 +- .../Internal/MqttClientSessionsManager.cs | 4 +- .../MQTTnet.Core.Tests/MQTTv5/Server_Tests.cs | 129 +++++++++++++++--- 3 files changed, 118 insertions(+), 28 deletions(-) diff --git a/Source/MQTTnet/Server/Internal/MqttClientSession.cs b/Source/MQTTnet/Server/Internal/MqttClientSession.cs index 291b3e5a..a943f5ad 100644 --- a/Source/MQTTnet/Server/Internal/MqttClientSession.cs +++ b/Source/MQTTnet/Server/Internal/MqttClientSession.cs @@ -8,11 +8,6 @@ namespace MQTTnet.Server.Internal { readonly DateTime _createdTimestamp = DateTime.UtcNow; - /// - /// Session should persist if CleanSession was set to false (Mqtt3) or if SessionExpiryInterval != 0 (Mqtt5) - /// - readonly bool _isPersistent; - public MqttClientSession( string clientId, IDictionary items, @@ -24,17 +19,19 @@ namespace MQTTnet.Server.Internal { ClientId = clientId ?? throw new ArgumentNullException(nameof(clientId)); Items = items ?? throw new ArgumentNullException(nameof(items)); - + IsPersistent = isPersistent; SubscriptionsManager = new MqttClientSubscriptionsManager(this, serverOptions, eventDispatcher, retainedMessagesManager); ApplicationMessagesQueue = new MqttClientSessionApplicationMessagesQueue(serverOptions); - _isPersistent = isPersistent; } public string ClientId { get; } public bool IsCleanSession { get; set; } = true; - public bool IsPersistent => _isPersistent; + /// + /// Session should persist if CleanSession was set to false (Mqtt3) or if SessionExpiryInterval != 0 (Mqtt5) + /// + public bool IsPersistent { get; set; } public MqttApplicationMessage WillMessage { get; set; } diff --git a/Source/MQTTnet/Server/Internal/MqttClientSessionsManager.cs b/Source/MQTTnet/Server/Internal/MqttClientSessionsManager.cs index 8831a05b..471e9df8 100644 --- a/Source/MQTTnet/Server/Internal/MqttClientSessionsManager.cs +++ b/Source/MQTTnet/Server/Internal/MqttClientSessionsManager.cs @@ -507,7 +507,7 @@ namespace MQTTnet.Server.Internal // in each time it connects. // Persist if SessionExpiryInterval != 0, but may start with a clean session - sessionShouldPersist = context.SessionExpiryInterval != 0; + sessionShouldPersist = context.SessionExpiryInterval.GetValueOrDefault() != 0; } else { @@ -540,6 +540,8 @@ namespace MQTTnet.Server.Internal else { _logger.Verbose("Reusing existing session of client '{0}'.", connectPacket.ClientId); + // Session persistence could change for MQTT 5 clients that reconnect with different SessionExpiryInterval + session.IsPersistent = sessionShouldPersist; connAckPacket.IsSessionPresent = true; } } diff --git a/Tests/MQTTnet.Core.Tests/MQTTv5/Server_Tests.cs b/Tests/MQTTnet.Core.Tests/MQTTv5/Server_Tests.cs index 18fde518..ce015b18 100644 --- a/Tests/MQTTnet.Core.Tests/MQTTv5/Server_Tests.cs +++ b/Tests/MQTTnet.Core.Tests/MQTTv5/Server_Tests.cs @@ -51,15 +51,8 @@ namespace MQTTnet.Tests.MQTTv5 const string ClientId = "Client1"; // Create client without clean session and long session expiry interval - - var client1 = await testEnvironment.ConnectClient(o => o - .WithProtocolVersion(MqttProtocolVersion.V500) - .WithTcpServer("127.0.0.1", testEnvironment.ServerPort) - .WithSessionExpiryInterval(9999) - .WithCleanSession(false) - .WithClientId(ClientId) - .Build() - ); + var options1 = CreateClientOptions(testEnvironment, ClientId, false, 9999); + var client1 = await testEnvironment.ConnectClient(options1); // Disconnect; empty session should remain on server @@ -72,16 +65,8 @@ namespace MQTTnet.Tests.MQTTv5 // Reconnect the same client ID to existing session var client2 = testEnvironment.CreateClient(); - var options = testEnvironment.Factory.CreateClientOptionsBuilder() - .WithProtocolVersion(MqttProtocolVersion.V500) - .WithTcpServer("127.0.0.1", testEnvironment.ServerPort) - .WithSessionExpiryInterval(9999) - .WithCleanSession(false) - .WithClientId(ClientId) - .Build(); - - - var result = await client2.ConnectAsync(options).ConfigureAwait(false); + var options2 = CreateClientOptions(testEnvironment, ClientId, false, 9999); + var result = await client2.ConnectAsync(options2).ConfigureAwait(false); await client2.DisconnectAsync(); @@ -90,5 +75,111 @@ namespace MQTTnet.Tests.MQTTv5 Assert.IsTrue(result.IsSessionPresent, "Session not present"); } } + + [TestMethod] + public async Task Connect_with_Undefined_SessionExpiryInterval() + { + using (var testEnvironment = new TestEnvironment(TestContext)) + { + // Create server with persistent sessions enabled + + await testEnvironment.StartServer(o => o.WithPersistentSessions()); + + const string ClientId = "Client1"; + + // Create client without clean session and NO session expiry interval, + // that means, the session should not persist + + var options1 = CreateClientOptions(testEnvironment, ClientId, false, null); + var client1 = await testEnvironment.ConnectClient(options1); + + // Disconnect; no session should remain on server because the session expiry interval was undefined + + await client1.DisconnectAsync(); + + // Simulate some time delay between connections + + await Task.Delay(1000); + + // Reconnect the same client ID to existing session + + var client2 = testEnvironment.CreateClient(); + var options2 = CreateClientOptions(testEnvironment, ClientId, false, 9999); + var result = await client2.ConnectAsync(options2).ConfigureAwait(false); + + await client2.DisconnectAsync(); + + // Session should not be present + + Assert.IsTrue(!result.IsSessionPresent, "Session is present when it should not"); + } + } + + + [TestMethod] + public async Task Reconnect_with_different_SessionExpiryInterval() + { + using (var testEnvironment = new TestEnvironment(TestContext)) + { + // Create server with persistent sessions enabled + + await testEnvironment.StartServer(o => o.WithPersistentSessions()); + + const string ClientId = "Client1"; + + // Create client with clean session and session expiry interval > 0 + + var options = CreateClientOptions(testEnvironment, ClientId, true, 9999); + var client1 = await testEnvironment.ConnectClient(options); + + // Disconnect; session should remain on server + + await client1.DisconnectAsync(); + + // Simulate some time delay between connections + + await Task.Delay(1000); + + // Reconnect the same client ID to the existing session but leave session expiry interval undefined this time. + // Session should be present because the client1 connection had SessionExpiryInterval > 0 + + var client2 = testEnvironment.CreateClient(); + var options2 = CreateClientOptions(testEnvironment, ClientId, false, null); + var result2 = await client2.ConnectAsync(options2).ConfigureAwait(false); + + await client2.DisconnectAsync(); + + Assert.IsTrue(result2.IsSessionPresent, "Session is not present when it should"); + + // Simulate some time delay between connections + + await Task.Delay(1000); + + // Reconnect the same client ID. + // No session should be present because the previous session expiry interval was undefined for the client2 connection + + var client3 = testEnvironment.CreateClient(); + var options3 = CreateClientOptions(testEnvironment, ClientId, false, null); + var result3 = await client2.ConnectAsync(options3).ConfigureAwait(false); + + await client3.DisconnectAsync(); + + // Session should be present + + Assert.IsTrue(!result3.IsSessionPresent, "Session is present when it should not"); + + } + } + + IMqttClientOptions CreateClientOptions(TestEnvironment testEnvironment, string clientId, bool cleanSession, uint? sessionExpiryInterval) + { + return testEnvironment.Factory.CreateClientOptionsBuilder() + .WithProtocolVersion(MqttProtocolVersion.V500) + .WithTcpServer("127.0.0.1", testEnvironment.ServerPort) + .WithSessionExpiryInterval(sessionExpiryInterval) + .WithCleanSession(cleanSession) + .WithClientId(clientId) + .Build(); + } } }