There isn't any code in MQTTnet that actually uses a cancellation token in WaitAsync, so this is more of a preventative thing than a bug fix. The original code just checks if the task was completed, not whether it was cancelled. If the process is cancelled immediately before the call to WaitAsync, it'll return as normal (https://referencesource.microsoft.com/#mscorlib/system/threading/SemaphoreSlim.cs,612) rather than throw a cancellation exception. This change will ensure we only return the releaser if the wait actually ran to completion rather than exited early due to cancellation. I've tested this, and it properly throws a cancellation exception later on.
Sometimes, TryPublishQueuedMessageAsync would try to remove a message from the storage queue before PublishAsync added it to the storage queue, resulting in a message being stuck in the storage queue forever. Switched the message queue lock to an async lock and synchronized the storage queue updates with the message queue updates.
We had been seeing an issue in which the queue could grow larger than the configured cap. I examined the code and saw that this could happen if _mqttClient.PublishAsync() throws an exception, in which case a message can be re-enqueued without honoring the cap. Furthermore, I saw that it was possible for the DropOldestQueuedMessage strategy to drop messages that were not actually the oldest ones, because when re-enqueueing the messages in the queue are no longer ordered by the original time they entered the queue. It made sense to us to peek at the message when publishing rather than dequeue it, so that when re-enqueueing after an exception 1) the cap is still honored and 2) the order of queued messages isn't altered. It's ok if another thread removes the message that's currently being published from the queue due to the cap, because all we have to do then is check if it's already been removed before removing it ourselves.
This might be a little controversial, but it worked for us to correct a problem in which messages get stuck in the managed client storage queue (and are thrown out of the regular message queue without being published!) in the case of a failed connection. What we were seeing was that ManagedMqttClient.TryPublishQueuedMessage() was discarding a dequeued message without ever removing it from the storage queue because of an OperationCancelledException thrown by MqttClient.PublishAsync(). Tracing the code back, we found that when the connection is interrupted, after the timeout period MqttClient.InitiateDisconnect() would set the cancellation token, and the managed client would continue to try to publish, eventually calling through to MqttClient.SendAndReceiveAsync(), which would throw because the cancellation token is set. Looking back over the code, we saw that MqttClient.PublishAsync() has a call to ThrowIfNotConnected() at the top, which told us that the intent was to not allow this function to be called after a disconnect. But the disconnect was still pending, and the function wasn't behaving correctly in this state, so we reasoned that it's best to throw if the disconnect is pending.
Paul Fake