From ad1c198e435385e9dde4586dd3abea16900bfc6b Mon Sep 17 00:00:00 2001 From: Johan x Lindqvist Date: Thu, 12 Sep 2019 09:05:08 +0200 Subject: [PATCH 1/5] Remove if clause that stopped handling of tasks with exceptions. Now the task is always awaited and thus the exception in the task is handled. --- Source/MQTTnet/Client/MqttClient.cs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/Source/MQTTnet/Client/MqttClient.cs b/Source/MQTTnet/Client/MqttClient.cs index 27b56ff..68045a2 100644 --- a/Source/MQTTnet/Client/MqttClient.cs +++ b/Source/MQTTnet/Client/MqttClient.cs @@ -149,7 +149,7 @@ namespace MQTTnet.Client Properties = new MqttAuthPacketProperties { // This must always be equal to the value from the CONNECT packet. So we use it here to ensure that. - AuthenticationMethod = Options.AuthenticationMethod, + AuthenticationMethod = Options.AuthenticationMethod, AuthenticationData = data.AuthenticationData, ReasonString = data.ReasonString, UserProperties = data.UserProperties @@ -567,7 +567,7 @@ namespace MQTTnet.Client }; await SendAsync(pubRecPacket, cancellationToken).ConfigureAwait(false); - } + } } else { @@ -633,11 +633,6 @@ namespace MQTTnet.Client return; } - if (task.IsCanceled || task.IsCompleted || task.IsFaulted) - { - return; - } - try { await task.ConfigureAwait(false); From ba9ceed7cec0b12132f9e3e8a638b266134fefbb Mon Sep 17 00:00:00 2001 From: Johan x Lindqvist Date: Mon, 16 Sep 2019 11:33:37 +0200 Subject: [PATCH 2/5] Use Task.WhenAll to handle errors in both tasks. Previously if there was an exception in the first task that is awaited the second task would not be awaited. --- Source/MQTTnet/Client/MqttClient.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Source/MQTTnet/Client/MqttClient.cs b/Source/MQTTnet/Client/MqttClient.cs index 68045a2..c5224cb 100644 --- a/Source/MQTTnet/Client/MqttClient.cs +++ b/Source/MQTTnet/Client/MqttClient.cs @@ -269,8 +269,10 @@ namespace MQTTnet.Client await _adapter.DisconnectAsync(Options.CommunicationTimeout, CancellationToken.None).ConfigureAwait(false); } - await WaitForTaskAsync(_packetReceiverTask, sender).ConfigureAwait(false); - await WaitForTaskAsync(_keepAlivePacketsSenderTask, sender).ConfigureAwait(false); + var receiverTask = WaitForTaskAsync(_packetReceiverTask, sender); + var keepAliveTask = WaitForTaskAsync(_keepAlivePacketsSenderTask, sender); + + await Task.WhenAll(receiverTask, keepAliveTask).ConfigureAwait(false); _logger.Verbose("Disconnected from adapter."); } From 5618c4c1f7f427d7672f3756c681860c1837dc91 Mon Sep 17 00:00:00 2001 From: Johan x Lindqvist Date: Wed, 18 Sep 2019 13:48:29 +0200 Subject: [PATCH 3/5] Always access the exception property on the task if it has status IsFaulted. By checking the Exception property it will consider the task exception as handled which means the finalizer won't throw an unhandled task exception. --- Source/MQTTnet/Client/MqttClient.cs | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/Source/MQTTnet/Client/MqttClient.cs b/Source/MQTTnet/Client/MqttClient.cs index c5224cb..6717f61 100644 --- a/Source/MQTTnet/Client/MqttClient.cs +++ b/Source/MQTTnet/Client/MqttClient.cs @@ -271,7 +271,7 @@ namespace MQTTnet.Client var receiverTask = WaitForTaskAsync(_packetReceiverTask, sender); var keepAliveTask = WaitForTaskAsync(_keepAlivePacketsSenderTask, sender); - + await Task.WhenAll(receiverTask, keepAliveTask).ConfigureAwait(false); _logger.Verbose("Disconnected from adapter."); @@ -628,10 +628,25 @@ namespace MQTTnet.Client return true; } - private static async Task WaitForTaskAsync(Task task, Task sender) + private async Task WaitForTaskAsync(Task task, Task sender) { - if (task == sender || task == null) + if (task == null) + { + return; + } + + if (task == sender) { + // Return here to avoid deadlocks, but first any eventual exception in the task + // must be handled to avoid not getting an unhandled task exception + if (!task.IsFaulted) + { + return; + } + + // By accessing the Exception property the exception is considered handled and will + // not result in an unhandled task exception later by the finalizer + _logger.Warning(task.Exception, "Exception when waiting for background task."); return; } From 61c3e02242826f8a9c775777976169ee2b575f91 Mon Sep 17 00:00:00 2001 From: Johan x Lindqvist Date: Thu, 19 Sep 2019 13:31:57 +0200 Subject: [PATCH 4/5] Updated exception message to be in line with other exception messages. --- Source/MQTTnet/Client/MqttClient.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/MQTTnet/Client/MqttClient.cs b/Source/MQTTnet/Client/MqttClient.cs index 6717f61..8520374 100644 --- a/Source/MQTTnet/Client/MqttClient.cs +++ b/Source/MQTTnet/Client/MqttClient.cs @@ -646,7 +646,7 @@ namespace MQTTnet.Client // By accessing the Exception property the exception is considered handled and will // not result in an unhandled task exception later by the finalizer - _logger.Warning(task.Exception, "Exception when waiting for background task."); + _logger.Warning(task.Exception, "Error while waiting for background task."); return; } From 699558e47a86d09511c1dee2c3fc53750e21ce24 Mon Sep 17 00:00:00 2001 From: Jimmy Rosenskog Date: Thu, 3 Oct 2019 09:46:40 +0200 Subject: [PATCH 5/5] Added exception handling to make sure all tasks are observed to avoid UnobservedTaskException. --- Source/MQTTnet/Client/MqttClient.cs | 37 ++++++++++++++++++++++------- 1 file changed, 28 insertions(+), 9 deletions(-) diff --git a/Source/MQTTnet/Client/MqttClient.cs b/Source/MQTTnet/Client/MqttClient.cs index 8520374..527a5cd 100644 --- a/Source/MQTTnet/Client/MqttClient.cs +++ b/Source/MQTTnet/Client/MqttClient.cs @@ -258,28 +258,34 @@ namespace MQTTnet.Client var clientWasConnected = IsConnected; TryInitiateDisconnect(); + IsConnected = false; try { - IsConnected = false; - if (_adapter != null) { _logger.Verbose("Disconnecting [Timeout={0}]", Options.CommunicationTimeout); await _adapter.DisconnectAsync(Options.CommunicationTimeout, CancellationToken.None).ConfigureAwait(false); } - var receiverTask = WaitForTaskAsync(_packetReceiverTask, sender); - var keepAliveTask = WaitForTaskAsync(_keepAlivePacketsSenderTask, sender); - - await Task.WhenAll(receiverTask, keepAliveTask).ConfigureAwait(false); - _logger.Verbose("Disconnected from adapter."); } catch (Exception adapterException) { _logger.Warning(adapterException, "Error while disconnecting from adapter."); } + + try + { + var receiverTask = WaitForTaskAsync(_packetReceiverTask, sender); + var keepAliveTask = WaitForTaskAsync(_keepAlivePacketsSenderTask, sender); + + await Task.WhenAll(receiverTask, keepAliveTask).ConfigureAwait(false); + } + catch (Exception e) + { + _logger.Warning(e, "Error while waiting for tasks."); + } finally { Dispose(); @@ -346,11 +352,24 @@ namespace MQTTnet.Client try { await _adapter.SendPacketAsync(requestPacket, Options.CommunicationTimeout, cancellationToken).ConfigureAwait(false); + } + catch (Exception e) + { + _logger.Warning(e, "Error when sending packet of type '{0}'.", typeof(TResponsePacket).Name); + packetAwaiter.Cancel(); + } + + try + { return await packetAwaiter.WaitOneAsync(Options.CommunicationTimeout).ConfigureAwait(false); } - catch (MqttCommunicationTimedOutException) + catch (Exception exception) { - _logger.Warning(null, "Timeout while waiting for packet of type '{0}'.", typeof(TResponsePacket).Name); + if (exception is MqttCommunicationTimedOutException) + { + _logger.Warning(null, "Timeout while waiting for packet of type '{0}'.", typeof(TResponsePacket).Name); + } + throw; } }