Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix synchronization of semaphore in retry delegating handler #3161

Closed
wants to merge 16 commits into from
Closed

Fix synchronization of semaphore in retry delegating handler #3161

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
676e933
separate out callback semaphores from open-close
abhipsaMisra Mar 14, 2023
89ec067
rename clientOpenSemaphore, update isOpen bool to enum
abhipsaMisra Mar 14, 2023
9b58d7d
update?
abhipsaMisra Mar 15, 2023
e68b87c
minor
abhipsaMisra Mar 15, 2023
64297fa
closing
abhipsaMisra Mar 15, 2023
6a7e3ae
closing
abhipsaMisra Mar 15, 2023
88e44a1
tests1
abhipsaMisra Mar 15, 2023
3594791
wip
abhipsaMisra Mar 21, 2023
72d3283
fixes
abhipsaMisra Mar 21, 2023
e81cab4
wip2
abhipsaMisra Mar 23, 2023
3b04a6b
rest
abhipsaMisra Mar 23, 2023
0e6b486
consolidate
abhipsaMisra Mar 23, 2023
0ca63bd
remove
abhipsaMisra Mar 23, 2023
f9002a1
reorder for logging
abhipsaMisra Mar 24, 2023
971cb6f
Remove need for ExceptionAssertion.cs
Mar 24, 2023
20e5d16
pr1
abhipsaMisra Mar 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions azureiot.sln
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Solution Items", "Solution
SDK v2 migration guide.md = SDK v2 migration guide.md
supported_platforms.md = supported_platforms.md
test.runsettings = test.runsettings
build.ps1 = build.ps1
EndProjectSection
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.Azure.Devices.Provisioning.Client.UnitTests", "provisioning\device\tests\Microsoft.Azure.Devices.Provisioning.Client.UnitTests.csproj", "{8797C176-398A-4347-B8E9-38FAF5746EC4}"
Expand Down
2 changes: 1 addition & 1 deletion build.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ Function BuildPackage($path, $message)
SignDotNetBinary $filesToSign
}

& dotnet pack --verbosity $verbosity --configuration $configuration --no-build --include-symbols --include-source --output $localPackages
& dotnet pack --verbosity $verbosity --configuration $configuration --no-build --include-symbols --include-source --property:PackageOutputPath=$localPackages

if ($LASTEXITCODE -ne 0)
{
Expand Down
2 changes: 1 addition & 1 deletion e2e/test/iothub/device/FileUploadE2ETests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
using System.Threading.Tasks;
using FluentAssertions;
using Microsoft.Azure.Devices.Client;
using Microsoft.Azure.Devices.Client.Transport;
using Microsoft.Azure.Devices.E2ETests.Helpers;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using Microsoft.WindowsAzure.Storage.Blob;
Expand Down Expand Up @@ -81,6 +80,7 @@ public async Task FileUpload_SmallFile_GranularSteps_x509()

[TestMethod]
[Timeout(LongRunningTestTimeoutMilliseconds)]
[TestCategory("Proxy")]
[TestCategory("LongRunning")]
public async Task FileUpload_SmallFile_GranularSteps_Proxy()
{
Expand Down
1 change: 1 addition & 0 deletions e2e/test/iothub/service/DevicesClientE2ETests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,7 @@ public async Task DevicesClient_RemoveDevicesAsync_Works()

[TestMethod]
[Timeout(TestTimeoutMilliseconds)]
[TestCategory("Proxy")]
public async Task DevicesClient_AddDeviceWithProxy()
{
string deviceId = _idPrefix + Guid.NewGuid();
Expand Down
6 changes: 4 additions & 2 deletions iothub/device/src/Pipeline/DefaultDelegatingHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,10 @@ namespace Microsoft.Azure.Devices.Client.Transport
{
internal abstract class DefaultDelegatingHandler : IDelegatingHandler
{
private volatile IDelegatingHandler _nextHandler;
protected const string ClientDisposedMessage = "The client has been disposed and is no longer usable.";

protected volatile bool _isDisposed;
private volatile IDelegatingHandler _nextHandler;

protected DefaultDelegatingHandler(PipelineContext context, IDelegatingHandler nextHandler)
{
Expand Down Expand Up @@ -170,7 +172,7 @@ protected private void ThrowIfDisposed()
{
if (_isDisposed)
{
throw new ObjectDisposedException("IoT Client");
throw new ObjectDisposedException("IoT client", ClientDisposedMessage);
}
}

Expand Down
414 changes: 285 additions & 129 deletions iothub/device/src/Pipeline/RetryDelegatingHandler.cs
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It occurs to me that this file which is supposed to be about retrying, and it looks like it does a lot more than that, especially with the addition of reconnection and overall connection (sort of) management.

Should checking the connection, reconnection attempts, etc. be a separate delegating handler, so this class doesn't get quite so full of mixed concerns?

Large diffs are not rendered by default.

28 changes: 28 additions & 0 deletions iothub/device/tests/ExceptionAssertions.cs
View file
Open in desktop
drwill-ms marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System;
using System.Threading.Tasks;
using FluentAssertions;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace Microsoft.Azure.Devices.Client.Tests
{
internal static class ExceptionAssertions
{
public static async Task<TException> ExpectedAsync<TException>(this Task action) where TException : Exception
{
try
{
await action.ConfigureAwait(false);
}
catch (Exception e)
{
e.Should().BeAssignableTo<TException>(e.ToString());
return (TException)e;
}

throw new AssertFailedException($"An exception of type \"{typeof(TException)}\" was expected, but none was thrown.");
}
}
}
Loading