feature,fix (device-client) Handle Twin failures using Amqp #1796

bikamani · 2021-02-18T22:38:07Z

Checklist

I have read the contribution guidelines.
I added or modified the existing tests to cover the change (we do not allow our test coverage to go down).
This pull-request is submitted against the master branch.

Description of the changes

This PR includes following changes
Add code to handle failures for Twin scenario for Get Twin
Fix a bug where SDK was mapping to wrong exception "Service Response is Null" for reported properties ( currently with this code client does not throw any exception due to limitations of update reported property API)
Improve naming of internal methods

Reference/Link to the issue solved with this PR (if any)

#1505

There will be follow up PRs to fix API implementation to handle failure for updating reported properties.

bikamani · 2021-02-18T22:38:30Z

/azp run

azure-pipelines · 2021-02-18T22:38:41Z

Azure Pipelines successfully started running 1 pipeline(s).

bikamani · 2021-02-19T01:00:09Z

/azp run

azure-pipelines · 2021-02-19T01:00:50Z

Azure Pipelines successfully started running 1 pipeline(s).

e2e/test/iothub/twin/TwinE2ETests.cs

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

bikamani · 2021-02-19T02:02:19Z

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

@@ -405,10 +406,6 @@ private async Task<Twin> RoundTripTwinMessage(AmqpTwinMessageType amqpTwinMessag
                    // Consider that the task may have faulted or been canceled.
                    // We re-await the task so that any exceptions/cancellation is rethrown.
                    response = await receivingTask.ConfigureAwait(false);
-                    if (response == null)


This is unnecessary since we can get to this point 2 ways

Get Twin response

Updating reported properties response

The check here was assuming the response coming here can never be null (one more assumption that it is only get) and we were throwing a wrong exception for (2)

bikamani · 2021-02-19T02:02:59Z

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

@@ -522,19 +519,23 @@ private async Task DisposeMessageAsync(string lockToken, AmqpIoTDisposeActions o

        private void TwinMessageListener(Twin twin, string correlationId, TwinCollection twinCollection)
        {
-            if (correlationId != null)
+            if (correlationId == null)


changing logic here to first handle desired property updates and then look at the type of message.

iothub/device/src/Transport/AmqpIoT/AmqpIoTReceivingLink.cs

bikamani · 2021-02-19T03:58:41Z

/azp run

azure-pipelines · 2021-02-19T03:58:52Z

Azure Pipelines successfully started running 1 pipeline(s).

iothub/device/src/Transport/AmqpIoT/AmqpIoTReceivingLink.cs

azure-pipelines · 2021-02-20T03:05:01Z

Azure Pipelines successfully started running 1 pipeline(s).

iothub/device/src/Transport/AmqpIoT/AmqpIoTReceivingLink.cs

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

iothub/device/src/Transport/AmqpIoT/AmqpIoTReceivingLink.cs

bikamani · 2021-02-25T19:11:52Z

/azp run

azure-pipelines · 2021-02-25T19:12:02Z

Azure Pipelines successfully started running 1 pipeline(s).

bikamani · 2021-02-27T01:08:28Z

/azp run

azure-pipelines · 2021-02-27T01:08:38Z

Azure Pipelines successfully started running 1 pipeline(s).

* fix: samples readme links were out-of-date (#1784) * fix: samples arguments and remove 1 more dead link (#1785) * fix(readme): Update the location of ConsoleEventListener in our readme * doc(service-client) - Updating readme (#1799) * fix(doc): Update amqp transport exception doc to have detailed description for quota exceeded error mapping * feature(device-client): Make the DeviceClient and ModuleClient extensible (#1802) * doc(service-client): Added extra comments to clarify true and false in dispose (#1805) * feature,fix (device-client) Handle Twin failures using Amqp (#1796) * fix(iot-service): Update xml comments for ServiceClient * fix(iot-device): Update MqttTransportHandler to not use SemaphoreSlim.WaitAsync(TimeSpan, CancellationToken) * fix(iot-device): Update dotnetty task calls to use ConfigureAwait(true) * fix(iot-device): Fix MqttTransportHandler to not await on user supplied C2D callback * IoTHub Exception for Get and Patch Twin failures (#1815) * fix(edge): UnixDomainSocketEndPoint is available in .NET 2.1 and greater (#1816) * UnixDomainSocketEndPoint has been standard since 2.1. Allowing later versions to use the correct class The edge client HSM provider uses UnixDomainSockets (UDS) for communication. Before .NET 2.1 to implement a Unix Socket you had to create your own class to do so. Since 2.1 there has been a native UnixDomainSocketEndPoint class in the runtime. In 2.1 and 3.1 there is no issue. However in 5.0 there are some changes to the way the Socket class handles the native UnixDomainSocketEndPoint class. I didn't dig down extremely deep, but I suspect it's due to the way the endpoint handles the SocketAddress and the string manipulation there seeing as how there is a specific implementation for Windows and for Unix. * feat(e2e) - Enabling soft delete when creating keyvaults (#1820) * fix(e2e) - Change event logging to opt in to specific events only. (#1824) * (service-client): Design for IoT hub AAD authentication * (service-client: Refactor and add implementation for token credential input) (#1781) * (service-client): Refactor and add sas credential (#1786) * (service-client): Add constructors in service client to accept aad and sas tokens. (#1787) * (service-client): Add constructors in registry manager to accept aad and sas tokens. (#1788) * (service-client): Add constructors in job client to accept aad and sas tokens. (#1789) * (service-client): Add constructors to accept aad and sas tokens for digital twins client. (#1790) * fix(service-client): Support for AzureSasCredential for a better user experience (#1797) * doc(service-client): Update readme about the differnt client and operations (#1798) * tests(service-client): E2E tests for aad auth on all our clients (#1800) * test(service-client): Adding e2e tests for sas credential auth for IoT hub. (#1806) * feature(service-client): Adding chaching for aad tokens. (#1807) * fix(service-client)- Add IoT hub token scope. (#1812) Co-authored-by: David R. Williamson <drwill@microsoft.com> Co-authored-by: Abhipsa Misra <abhipsa.misra@microsoft.com> Co-authored-by: bikamani <41314966+bikamani@users.noreply.github.com> Co-authored-by: jamdavi <73593426+jamdavi@users.noreply.github.com>

…1796)" This reverts commit 2c01037.

* fix: samples readme links were out-of-date (#1784) * fix: samples arguments and remove 1 more dead link (#1785) * fix(readme): Update the location of ConsoleEventListener in our readme * doc(service-client) - Updating readme (#1799) * fix(doc): Update amqp transport exception doc to have detailed description for quota exceeded error mapping * feature(device-client): Make the DeviceClient and ModuleClient extensible (#1802) * doc(service-client): Added extra comments to clarify true and false in dispose (#1805) * feature,fix (device-client) Handle Twin failures using Amqp (#1796) * fix(iot-service): Update xml comments for ServiceClient * fix(iot-device): Update MqttTransportHandler to not use SemaphoreSlim.WaitAsync(TimeSpan, CancellationToken) * fix(iot-device): Update dotnetty task calls to use ConfigureAwait(true) * fix(iot-device): Fix MqttTransportHandler to not await on user supplied C2D callback * IoTHub Exception for Get and Patch Twin failures (#1815) * fix(edge): UnixDomainSocketEndPoint is available in .NET 2.1 and greater (#1816) * UnixDomainSocketEndPoint has been standard since 2.1. Allowing later versions to use the correct class The edge client HSM provider uses UnixDomainSockets (UDS) for communication. Before .NET 2.1 to implement a Unix Socket you had to create your own class to do so. Since 2.1 there has been a native UnixDomainSocketEndPoint class in the runtime. In 2.1 and 3.1 there is no issue. However in 5.0 there are some changes to the way the Socket class handles the native UnixDomainSocketEndPoint class. I didn't dig down extremely deep, but I suspect it's due to the way the endpoint handles the SocketAddress and the string manipulation there seeing as how there is a specific implementation for Windows and for Unix. * feat(e2e) - Enabling soft delete when creating keyvaults (#1820) * fix(e2e) - Change event logging to opt in to specific events only. (#1824) * (service-client): Design for IoT hub AAD authentication * (service-client: Refactor and add implementation for token credential input) (#1781) * (service-client): Refactor and add sas credential (#1786) * (service-client): Add constructors in service client to accept aad and sas tokens. (#1787) * (service-client): Add constructors in registry manager to accept aad and sas tokens. (#1788) * (service-client): Add constructors in job client to accept aad and sas tokens. (#1789) * (service-client): Add constructors to accept aad and sas tokens for digital twins client. (#1790) * fix(service-client): Support for AzureSasCredential for a better user experience (#1797) * doc(service-client): Update readme about the differnt client and operations (#1798) * tests(service-client): E2E tests for aad auth on all our clients (#1800) * test(service-client): Adding e2e tests for sas credential auth for IoT hub. (#1806) * feature(service-client): Adding chaching for aad tokens. (#1807) * fix(service-client)- Add IoT hub token scope. (#1812) Co-authored-by: David R. Williamson <drwill@microsoft.com> Co-authored-by: Abhipsa Misra <abhipsa.misra@microsoft.com> Co-authored-by: bikamani <41314966+bikamani@users.noreply.github.com> Co-authored-by: jamdavi <73593426+jamdavi@users.noreply.github.com>

initial changes to handle failures

9a22f2e

fix and ignore few tests

71b8a2e

bikamani commented Feb 19, 2021

View reviewed changes

e2e/test/iothub/twin/TwinE2ETests.cs Show resolved Hide resolved

bikamani commented Feb 19, 2021

View reviewed changes

iothub/device/src/Transport/Amqp/AmqpTransportHandler.cs Show resolved Hide resolved

bikamani commented Feb 19, 2021

View reviewed changes

bikamani changed the title ~~[do not review]~~ [do not review] Handle Twin Failures using Amqp Feb 19, 2021

change logic

8a3cd01

bikamani commented Feb 19, 2021

View reviewed changes

iothub/device/src/Transport/AmqpIoT/AmqpIoTReceivingLink.cs Outdated Show resolved Hide resolved

bikamani commented Feb 19, 2021

View reviewed changes

iothub/device/src/Transport/AmqpIoT/AmqpIoTReceivingLink.cs Show resolved Hide resolved

bikamani changed the title ~~[do not review] Handle Twin Failures using Amqp~~ feature,fix (device-client) Handle Twin failures using Amqp Feb 19, 2021

bikamani marked this pull request as ready for review February 19, 2021 03:20

bikamani requested review from abhipsaMisra, azabbasi, barustum, drwill-ms, jamdavi, timtay-microsoft and vinagesh as code owners February 19, 2021 03:20

barustum reviewed Feb 19, 2021

View reviewed changes

iothub/device/src/Transport/AmqpIoT/AmqpIoTReceivingLink.cs Show resolved Hide resolved

barustum reviewed Feb 19, 2021

View reviewed changes

iothub/device/src/Transport/AmqpIoT/AmqpIoTReceivingLink.cs Outdated Show resolved Hide resolved

barustum reviewed Feb 19, 2021

View reviewed changes

iothub/device/src/Transport/AmqpIoT/AmqpIoTReceivingLink.cs Outdated Show resolved Hide resolved

barustum reviewed Feb 19, 2021

View reviewed changes

iothub/device/src/Transport/AmqpIoT/AmqpIoTReceivingLink.cs Outdated Show resolved Hide resolved

barustum reviewed Feb 19, 2021

View reviewed changes

iothub/device/src/Transport/AmqpIoT/AmqpIoTReceivingLink.cs Outdated Show resolved Hide resolved

drwill-ms reviewed Feb 22, 2021

View reviewed changes

iothub/device/src/Transport/AmqpIoT/AmqpIoTReceivingLink.cs Outdated Show resolved Hide resolved

drwill-ms approved these changes Feb 22, 2021

View reviewed changes

barustum approved these changes Feb 25, 2021

View reviewed changes

abhipsaMisra reviewed Feb 25, 2021

View reviewed changes

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

abhipsaMisra reviewed Feb 25, 2021

View reviewed changes

iothub/device/src/Transport/AmqpIoT/AmqpIoTReceivingLink.cs Outdated Show resolved Hide resolved

abhipsaMisra reviewed Feb 25, 2021

View reviewed changes

iothub/device/src/Transport/AmqpIoT/AmqpIoTReceivingLink.cs Show resolved Hide resolved

abhipsaMisra approved these changes Feb 25, 2021

View reviewed changes

bikamani added 2 commits February 25, 2021 11:09

Merge branch 'master' into bikamani/correlationId

31ce55f

resolve more comments

009f4a0

bikamani added 2 commits February 26, 2021 17:03

Merge branch 'master' into bikamani/correlationId

49fdcc3

small change

56c7989

bikamani merged commit 5fcd166 into master Feb 27, 2021

bikamani deleted the bikamani/correlationId branch February 27, 2021 01:24

abhipsaMisra pushed a commit that referenced this pull request Mar 19, 2021

feature,fix (device-client) Handle Twin failures using Amqp (#1796)

2c01037

abhipsaMisra added a commit that referenced this pull request Mar 19, 2021

Revert "feature,fix (device-client) Handle Twin failures using Amqp (#…

e3ff7e7

…1796)" This reverts commit 2c01037.

abhipsaMisra added a commit that referenced this pull request Mar 19, 2021

Revert "feature,fix (device-client) Handle Twin failures using Amqp (#…

2886502

…1796)" This reverts commit 2c01037.

jamdavi mentioned this pull request Mar 19, 2021

release: Bump versions for latest release #1842

Merged

abhipsaMisra added a commit that referenced this pull request Mar 19, 2021

Revert "feature,fix (device-client) Handle Twin failures using Amqp (#…

e65e5f4

…1796)" This reverts commit 2c01037.

vinagesh pushed a commit that referenced this pull request Mar 22, 2021

feature,fix (device-client) Handle Twin failures using Amqp (#1796)

7285202

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature,fix (device-client) Handle Twin failures using Amqp #1796

feature,fix (device-client) Handle Twin failures using Amqp #1796

bikamani commented Feb 18, 2021 •

edited

bikamani commented Feb 18, 2021

azure-pipelines bot commented Feb 18, 2021

bikamani commented Feb 19, 2021

azure-pipelines bot commented Feb 19, 2021

bikamani Feb 19, 2021

bikamani Feb 19, 2021

bikamani commented Feb 19, 2021

azure-pipelines bot commented Feb 19, 2021

azure-pipelines bot commented Feb 20, 2021

bikamani commented Feb 25, 2021

azure-pipelines bot commented Feb 25, 2021

bikamani commented Feb 27, 2021

azure-pipelines bot commented Feb 27, 2021

feature,fix (device-client) Handle Twin failures using Amqp #1796

feature,fix (device-client) Handle Twin failures using Amqp #1796

Conversation

bikamani commented Feb 18, 2021 • edited

Checklist

Description of the changes

Reference/Link to the issue solved with this PR (if any)

bikamani commented Feb 18, 2021

azure-pipelines bot commented Feb 18, 2021

bikamani commented Feb 19, 2021

azure-pipelines bot commented Feb 19, 2021

bikamani Feb 19, 2021

Choose a reason for hiding this comment

bikamani Feb 19, 2021

Choose a reason for hiding this comment

bikamani commented Feb 19, 2021

azure-pipelines bot commented Feb 19, 2021

azure-pipelines bot commented Feb 20, 2021

bikamani commented Feb 25, 2021

azure-pipelines bot commented Feb 25, 2021

bikamani commented Feb 27, 2021

azure-pipelines bot commented Feb 27, 2021

bikamani commented Feb 18, 2021 •

edited