Add M2M message failure fixes to lts_2021_03 #3006

schoims · 2022-12-06T17:25:52Z

Bringing in

Fixed M2M message (MQTT) failure after receiving Twin response or Direct method (Fixed M2M message (MQTT) failure after receiving Twin response or Direct method #2938)

iothub/device/src/Transport/Mqtt/OrderedTwoPhaseWorkQueue.cs

schoims · 2022-12-07T19:00:40Z

iothub/device/src/Transport/Mqtt/MqttIotHubAdapter.cs

+                        if (((PublishPacket)packet).QualityOfService == QualityOfService.AtLeastOnce &&
+                            ((PublishPacket)packet).TopicName.StartsWith(MqttTransportHandler.MethodPostTopicPrefix, StringComparison.OrdinalIgnoreCase))
+                        {
+                            await AcknowledgeAsync(context, ((PublishPacket)packet).PacketId.ToString(CultureInfo.InvariantCulture)).ConfigureAwait(false);


ToString method required IFormatProvider to be specified, so added CultureInfo.InvariantCulture

iothub/device/src/Transport/Amqp/AmqpAuthenticationRefresher.cs

iothub/device/src/Transport/Mqtt/MqttIotHubAdapter.cs

iothub/device/src/Transport/Mqtt/OrderedTwoPhaseWorkQueue.cs

schoims · 2022-12-07T22:29:55Z

/azp run

azure-pipelines · 2022-12-07T22:30:03Z

Azure Pipelines successfully started running 1 pipeline(s).

schoims · 2022-12-08T00:06:30Z

/azp run

azure-pipelines · 2022-12-08T00:06:38Z

Azure Pipelines successfully started running 1 pipeline(s).

schoims · 2022-12-08T00:44:05Z

/azp run

azure-pipelines · 2022-12-08T00:44:13Z

Azure Pipelines successfully started running 1 pipeline(s).

tmahmood-microsoft · 2022-12-08T01:10:07Z

/azp run

azure-pipelines · 2022-12-08T01:10:16Z

Azure Pipelines successfully started running 1 pipeline(s).

drwill-ms · 2022-12-08T02:05:47Z

iothub/device/src/Transport/Amqp/AmqpAuthenticationRefresher.cs

            {
-                Logging.Info(this, $"{nameof(StopLoop)}");
+                _refresherCancellationTokenSource?.Cancel();
+            }


should it wait for the loop to end? We have cached the Task from it, right?

We cannot wait for the loop to end since this is an infinite loop. Control will exit the loop only when the cancellation token is signaled for cancellation.

This an unmonitored loop running on a separate thread.

Right, but you just canceled it on line 181, right?

I thought awaiting on the loop in here might help us catch an OperationCancelledException in the RefreshLoopAsync running unmonitored, but it turns out the only place we refer to the cancellation token is when we check it manually for cancellation. So, the ~~API~~ method should not encounter OperationCancellationException.

Would there be any advantage to us awaiting this loop then?

Awaiting for any exception and logging it.

If we don't await it, let's make sure that method does not throw, and logs anything it catches.

Sure, updated accordingly.

schoims · 2022-12-09T18:16:11Z

/azp run

azure-pipelines · 2022-12-09T18:16:21Z

Azure Pipelines successfully started running 1 pipeline(s).

schoims · 2022-12-09T18:19:40Z

/azp run

azure-pipelines · 2022-12-09T18:19:48Z

Azure Pipelines successfully started running 1 pipeline(s).

drwill-ms · 2022-12-09T20:25:30Z

iothub/device/src/Transport/Amqp/AmqpAuthenticationRefresher.cs

+        // StopLoop ensures that the resources related to token refresher are cancelled and ready for cleanup.
+        // It will cancel the associated cancellation token source and will await completion of the refresh loop task.
+        // Any exceptions thrown as a result of these operations are caught and logged.
+        // StopLoop will not generate any exceptions so it safe for us to declare this as async void.


Still don't do this. It hurts nothing to return a Task.

I agree. I would have liked to return a Task but the way this call chains up prevents us from returning a Task.
This is invoked from within Dispose() and certain event handlers that return void, so we can either makie this fire-forget or block on this task within the caller.
Since this method isn't expected to have any undesireable side-effect, making this fire-forget seemed like the better option.

The call chain doesn't prevent this from returning a task. It just prevents the callers from awaiting it.

Also, we can choose to use IAsyncDisposable, if we wish.

drwill-ms · 2022-12-09T20:26:32Z

iothub/device/src/Transport/Amqp/AmqpAuthenticationRefresher.cs

+                catch (Exception ex)
+                {
+                    if (Logging.IsEnabled)
+                        Logging.Error(this, $"Exception caught while canceling SAS token refresh loop: {ex}", nameof(StopLoop));
+                }


This try/catch should cover the entire method, including logging and the CTS cancel.

drwill-ms · 2022-12-09T20:26:58Z

iothub/device/src/Transport/Amqp/AmqpAuthenticationRefresher.cs

+                catch (Exception ex)
+                {
+                    if (Logging.IsEnabled)
+                        Logging.Error(this, $"Exception caught while canceling SAS token refresh loop: {ex}", nameof(StopLoop));


Suggested change

Logging.Error(this, $"Exception caught while canceling SAS token refresh loop: {ex}", nameof(StopLoop));

Logging.Error(this, $"Exception caught while waiting for SAS token refresh loop to exit: {ex}", nameof(StopLoop));

schoims · 2022-12-12T21:36:10Z

/azp run

azure-pipelines · 2022-12-12T21:36:20Z

Azure Pipelines successfully started running 1 pipeline(s).

schoims commented Dec 7, 2022

View reviewed changes

iothub/device/src/Transport/Mqtt/OrderedTwoPhaseWorkQueue.cs Show resolved Hide resolved

schoims commented Dec 7, 2022

View reviewed changes

schoims marked this pull request as ready for review December 7, 2022 19:01

schoims requested review from drwill-ms, timtay-microsoft and abhipsaMisra as code owners December 7, 2022 19:01

drwill-ms reviewed Dec 7, 2022

View reviewed changes

iothub/device/src/Transport/Amqp/AmqpAuthenticationRefresher.cs Outdated Show resolved Hide resolved

drwill-ms reviewed Dec 7, 2022

View reviewed changes

iothub/device/src/Transport/Mqtt/MqttIotHubAdapter.cs Outdated Show resolved Hide resolved

drwill-ms reviewed Dec 7, 2022

View reviewed changes

iothub/device/src/Transport/Mqtt/OrderedTwoPhaseWorkQueue.cs Outdated Show resolved Hide resolved

drwill-ms reviewed Dec 7, 2022

View reviewed changes

iothub/device/src/Transport/Mqtt/OrderedTwoPhaseWorkQueue.cs Outdated Show resolved Hide resolved

schoims force-pushed the choisophia/add_fixes branch from a1cb21d to 2bec37a Compare December 8, 2022 00:43

drwill-ms reviewed Dec 8, 2022

View reviewed changes

drwill-ms approved these changes Dec 8, 2022

View reviewed changes

tmahmood-microsoft approved these changes Dec 8, 2022

View reviewed changes

schoims changed the title ~~Add fixes to lts~~ Add AMQP token refresher cancellation and M2M message failure fixes to lts Dec 9, 2022

schoims force-pushed the choisophia/add_fixes branch from 627dde3 to def7607 Compare December 9, 2022 18:19

schoims changed the title ~~Add AMQP token refresher cancellation and M2M message failure fixes to lts~~ Add AMQP token refresher cancellation and M2M message failure fixes to lts_2021_03 Dec 9, 2022

abhipsaMisra approved these changes Dec 9, 2022

View reviewed changes

drwill-ms reviewed Dec 9, 2022

View reviewed changes

drwill-ms approved these changes Dec 9, 2022

View reviewed changes

schoims changed the title ~~Add AMQP token refresher cancellation and M2M message failure fixes to lts_2021_03~~ Add M2M message failure fixes to lts_2021_03 Dec 12, 2022

schoims force-pushed the choisophia/add_fixes branch from 5ebfc54 to 9e4844c Compare December 12, 2022 21:35

Add M2M message failure fixes

2d5e1de

schoims force-pushed the choisophia/add_fixes branch from 9e4844c to 2d5e1de Compare December 12, 2022 21:35

schoims merged commit ca70b27 into lts_2021_03 Dec 12, 2022

schoims deleted the choisophia/add_fixes branch December 12, 2022 21:54

This was referenced Jan 9, 2023

Bump version numbers for LTS patch release #3058

Merged

Bump versions of remaining packages since shared package was updated #3060

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add M2M message failure fixes to lts_2021_03 #3006

Add M2M message failure fixes to lts_2021_03 #3006

schoims commented Dec 6, 2022 •

edited

schoims Dec 7, 2022

schoims commented Dec 7, 2022

azure-pipelines bot commented Dec 7, 2022

schoims commented Dec 8, 2022

azure-pipelines bot commented Dec 8, 2022

schoims commented Dec 8, 2022

azure-pipelines bot commented Dec 8, 2022

tmahmood-microsoft commented Dec 8, 2022

azure-pipelines bot commented Dec 8, 2022

drwill-ms Dec 8, 2022

schoims Dec 8, 2022

schoims Dec 8, 2022

drwill-ms Dec 8, 2022

abhipsaMisra Dec 8, 2022 •

edited

drwill-ms Dec 9, 2022

schoims Dec 9, 2022

schoims commented Dec 9, 2022

azure-pipelines bot commented Dec 9, 2022

schoims commented Dec 9, 2022

azure-pipelines bot commented Dec 9, 2022

drwill-ms Dec 9, 2022

abhipsaMisra Dec 9, 2022

drwill-ms Dec 9, 2022

drwill-ms Dec 9, 2022

drwill-ms Dec 9, 2022

schoims commented Dec 12, 2022

azure-pipelines bot commented Dec 12, 2022

	Logging.Error(this, $"Exception caught while canceling SAS token refresh loop: {ex}", nameof(StopLoop));
	Logging.Error(this, $"Exception caught while waiting for SAS token refresh loop to exit: {ex}", nameof(StopLoop));

Add M2M message failure fixes to lts_2021_03 #3006

Add M2M message failure fixes to lts_2021_03 #3006

Conversation

schoims commented Dec 6, 2022 • edited

Choose a reason for hiding this comment

schoims commented Dec 7, 2022

azure-pipelines bot commented Dec 7, 2022

schoims commented Dec 8, 2022

azure-pipelines bot commented Dec 8, 2022

schoims commented Dec 8, 2022

azure-pipelines bot commented Dec 8, 2022

tmahmood-microsoft commented Dec 8, 2022

azure-pipelines bot commented Dec 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhipsaMisra Dec 8, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schoims commented Dec 9, 2022

azure-pipelines bot commented Dec 9, 2022

schoims commented Dec 9, 2022

azure-pipelines bot commented Dec 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schoims commented Dec 12, 2022

azure-pipelines bot commented Dec 12, 2022

schoims commented Dec 6, 2022 •

edited

abhipsaMisra Dec 8, 2022 •

edited