From a62ef67f62ded017d4b04094325137cf7c84acf2 Mon Sep 17 00:00:00 2001 From: Abhipsa Misra Date: Tue, 11 May 2021 15:13:29 -0700 Subject: [PATCH 01/19] fix(iot-device): Update client to not dispose user supplied IDisposable types --- iothub/device/src/InternalClient.cs | 1 - iothub/device/src/IotHubConnectionString.cs | 5 ++++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/iothub/device/src/InternalClient.cs b/iothub/device/src/InternalClient.cs index 5f4794c9c9..25a420da3c 100644 --- a/iothub/device/src/InternalClient.cs +++ b/iothub/device/src/InternalClient.cs @@ -1896,7 +1896,6 @@ public void Dispose() _fileUploadHttpTransportHandler?.Dispose(); _deviceReceiveMessageSemaphore?.Dispose(); _twinDesiredPropertySemaphore?.Dispose(); - IotHubConnectionString?.TokenRefresher?.Dispose(); } internal bool IsE2EDiagnosticSupportedProtocol() diff --git a/iothub/device/src/IotHubConnectionString.cs b/iothub/device/src/IotHubConnectionString.cs index 9bf87c27f8..fb40b08d01 100644 --- a/iothub/device/src/IotHubConnectionString.cs +++ b/iothub/device/src/IotHubConnectionString.cs @@ -25,7 +25,6 @@ public IotHubConnectionString(IotHubConnectionStringBuilder builder) : builder.GatewayHostName; SharedAccessKeyName = builder.SharedAccessKeyName; SharedAccessKey = builder.SharedAccessKey; - SharedAccessSignature = builder.SharedAccessSignature; IotHubName = builder.IotHubName; DeviceId = builder.DeviceId; ModuleId = builder.ModuleId; @@ -75,6 +74,10 @@ public IotHubConnectionString(IotHubConnectionStringBuilder builder) Debug.Assert(TokenRefresher != null); } + else if (!string.IsNullOrWhiteSpace(builder.SharedAccessSignature)) + { + SharedAccessSignature = builder.SharedAccessSignature; + } } public string IotHubName { get; private set; } From 7a82205fa143af2c4c0c4e5a6601a768321acf90 Mon Sep 17 00:00:00 2001 From: Abhipsa Misra Date: Tue, 11 May 2021 16:43:31 -0700 Subject: [PATCH 02/19] sdk should dispose if it created it --- iothub/device/src/AuthenticationWithTokenRefresh.cs | 2 ++ iothub/device/src/InternalClient.cs | 5 +++++ iothub/device/src/IotHubConnectionString.cs | 12 ++++++++++-- 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/iothub/device/src/AuthenticationWithTokenRefresh.cs b/iothub/device/src/AuthenticationWithTokenRefresh.cs index 1ec77bdcc6..453f2befb2 100644 --- a/iothub/device/src/AuthenticationWithTokenRefresh.cs +++ b/iothub/device/src/AuthenticationWithTokenRefresh.cs @@ -65,6 +65,8 @@ public AuthenticationWithTokenRefresh( /// public bool IsExpiring => (ExpiresOn - DateTime.UtcNow).TotalSeconds <= _bufferSeconds; + internal bool InstanceCreatedBySdk { get; set; } + /// /// Gets a snapshot of the security token associated with the device. This call is thread-safe. /// diff --git a/iothub/device/src/InternalClient.cs b/iothub/device/src/InternalClient.cs index 25a420da3c..92a6b4f537 100644 --- a/iothub/device/src/InternalClient.cs +++ b/iothub/device/src/InternalClient.cs @@ -1896,6 +1896,11 @@ public void Dispose() _fileUploadHttpTransportHandler?.Dispose(); _deviceReceiveMessageSemaphore?.Dispose(); _twinDesiredPropertySemaphore?.Dispose(); + + if ((IotHubConnectionString?.TokenRefresher?.InstanceCreatedBySdk).GetValueOrDefault()) + { + IotHubConnectionString?.TokenRefresher?.Dispose(); + } } internal bool IsE2EDiagnosticSupportedProtocol() diff --git a/iothub/device/src/IotHubConnectionString.cs b/iothub/device/src/IotHubConnectionString.cs index fb40b08d01..e6707db24d 100644 --- a/iothub/device/src/IotHubConnectionString.cs +++ b/iothub/device/src/IotHubConnectionString.cs @@ -52,7 +52,11 @@ public IotHubConnectionString(IotHubConnectionStringBuilder builder) { if (ModuleId.IsNullOrWhiteSpace()) { - TokenRefresher = new DeviceAuthenticationWithSakRefresh(DeviceId, this, builder.SasTokenTimeToLive, builder.SasTokenRenewalBuffer); + TokenRefresher = new DeviceAuthenticationWithSakRefresh(DeviceId, this, builder.SasTokenTimeToLive, builder.SasTokenRenewalBuffer) + { + InstanceCreatedBySdk = true, + }; + if (Logging.IsEnabled) { Logging.Info(this, $"{nameof(IAuthenticationMethod)} is {nameof(DeviceAuthenticationWithSakRefresh)}: {Logging.IdOf(TokenRefresher)}"); @@ -60,7 +64,11 @@ public IotHubConnectionString(IotHubConnectionStringBuilder builder) } else { - TokenRefresher = new ModuleAuthenticationWithSakRefresh(DeviceId, ModuleId, this, builder.SasTokenTimeToLive, builder.SasTokenRenewalBuffer); + TokenRefresher = new ModuleAuthenticationWithSakRefresh(DeviceId, ModuleId, this, builder.SasTokenTimeToLive, builder.SasTokenRenewalBuffer) + { + InstanceCreatedBySdk = true, + }; + if (Logging.IsEnabled) { Logging.Info(this, $"{nameof(IAuthenticationMethod)} is {nameof(ModuleAuthenticationWithSakRefresh)}: {Logging.IdOf(TokenRefresher)}"); From 35edd323bb3f018ded02ded8086eff47570870da Mon Sep 17 00:00:00 2001 From: Abhipsa Misra Date: Wed, 12 May 2021 16:21:59 -0700 Subject: [PATCH 03/19] one more instance created by sdk --- iothub/device/src/Edge/EdgeModuleClientFactory.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/iothub/device/src/Edge/EdgeModuleClientFactory.cs b/iothub/device/src/Edge/EdgeModuleClientFactory.cs index d4451e360d..5bb9764c41 100644 --- a/iothub/device/src/Edge/EdgeModuleClientFactory.cs +++ b/iothub/device/src/Edge/EdgeModuleClientFactory.cs @@ -98,7 +98,10 @@ private async Task CreateInternalClientFromEnvironmentAsync() int sasTokenRenewalBuffer = _options?.SasTokenRenewalBuffer ?? default; #pragma warning disable CA2000 // Dispose objects before losing scope - IDisposable ModuleAuthenticationWithHsm is disposed when the client is disposed. - var authMethod = new ModuleAuthenticationWithHsm(signatureProvider, deviceId, moduleId, generationId, sasTokenTimeToLive, sasTokenRenewalBuffer); + var authMethod = new ModuleAuthenticationWithHsm(signatureProvider, deviceId, moduleId, generationId, sasTokenTimeToLive, sasTokenRenewalBuffer) + { + InstanceCreatedBySdk = true, + }; #pragma warning restore CA2000 // Dispose objects before losing scope - IDisposable ModuleAuthenticationWithHsm is disposed when the client is disposed. Debug.WriteLine("EdgeModuleClientFactory setupTrustBundle from service"); From 336c4e00a73e9f2ebc37d3ba332b8f9639f06498 Mon Sep 17 00:00:00 2001 From: Abhipsa Misra Date: Thu, 13 May 2021 10:47:16 -0700 Subject: [PATCH 04/19] only final group sas token refresher pending --- iothub/device/src/AuthenticationWithTokenRefresh.cs | 2 ++ iothub/device/src/Edge/EdgeModuleClientFactory.cs | 1 + iothub/device/src/InternalClient.cs | 3 ++- iothub/device/src/IotHubConnectionString.cs | 2 ++ 4 files changed, 7 insertions(+), 1 deletion(-) diff --git a/iothub/device/src/AuthenticationWithTokenRefresh.cs b/iothub/device/src/AuthenticationWithTokenRefresh.cs index 453f2befb2..37c862f3df 100644 --- a/iothub/device/src/AuthenticationWithTokenRefresh.cs +++ b/iothub/device/src/AuthenticationWithTokenRefresh.cs @@ -67,6 +67,8 @@ public AuthenticationWithTokenRefresh( internal bool InstanceCreatedBySdk { get; set; } + internal bool IsIndividualSasAuthenticatedToken { get; set; } + /// /// Gets a snapshot of the security token associated with the device. This call is thread-safe. /// diff --git a/iothub/device/src/Edge/EdgeModuleClientFactory.cs b/iothub/device/src/Edge/EdgeModuleClientFactory.cs index 5bb9764c41..88b8978e8a 100644 --- a/iothub/device/src/Edge/EdgeModuleClientFactory.cs +++ b/iothub/device/src/Edge/EdgeModuleClientFactory.cs @@ -101,6 +101,7 @@ private async Task CreateInternalClientFromEnvironmentAsync() var authMethod = new ModuleAuthenticationWithHsm(signatureProvider, deviceId, moduleId, generationId, sasTokenTimeToLive, sasTokenRenewalBuffer) { InstanceCreatedBySdk = true, + IsIndividualSasAuthenticatedToken = true, }; #pragma warning restore CA2000 // Dispose objects before losing scope - IDisposable ModuleAuthenticationWithHsm is disposed when the client is disposed. diff --git a/iothub/device/src/InternalClient.cs b/iothub/device/src/InternalClient.cs index 92a6b4f537..295fe46647 100644 --- a/iothub/device/src/InternalClient.cs +++ b/iothub/device/src/InternalClient.cs @@ -1897,7 +1897,8 @@ public void Dispose() _deviceReceiveMessageSemaphore?.Dispose(); _twinDesiredPropertySemaphore?.Dispose(); - if ((IotHubConnectionString?.TokenRefresher?.InstanceCreatedBySdk).GetValueOrDefault()) + if ((IotHubConnectionString?.TokenRefresher?.InstanceCreatedBySdk).GetValueOrDefault() + && (IotHubConnectionString?.TokenRefresher?.IsIndividualSasAuthenticatedToken).GetValueOrDefault()) { IotHubConnectionString?.TokenRefresher?.Dispose(); } diff --git a/iothub/device/src/IotHubConnectionString.cs b/iothub/device/src/IotHubConnectionString.cs index e6707db24d..cf609babea 100644 --- a/iothub/device/src/IotHubConnectionString.cs +++ b/iothub/device/src/IotHubConnectionString.cs @@ -55,6 +55,7 @@ public IotHubConnectionString(IotHubConnectionStringBuilder builder) TokenRefresher = new DeviceAuthenticationWithSakRefresh(DeviceId, this, builder.SasTokenTimeToLive, builder.SasTokenRenewalBuffer) { InstanceCreatedBySdk = true, + IsIndividualSasAuthenticatedToken = builder.SharedAccessKeyName == null, }; if (Logging.IsEnabled) @@ -67,6 +68,7 @@ public IotHubConnectionString(IotHubConnectionStringBuilder builder) TokenRefresher = new ModuleAuthenticationWithSakRefresh(DeviceId, ModuleId, this, builder.SasTokenTimeToLive, builder.SasTokenRenewalBuffer) { InstanceCreatedBySdk = true, + IsIndividualSasAuthenticatedToken = builder.SharedAccessKeyName == null, }; if (Logging.IsEnabled) From 40c7cf88281249042ba73a07ad09bd568fcef806 Mon Sep 17 00:00:00 2001 From: Abhipsa Misra Date: Thu, 13 May 2021 13:41:55 -0700 Subject: [PATCH 05/19] delegate amqp group token refresher disposal to transport layer --- .../src/AuthenticationWithTokenRefresh.cs | 6 +- .../src/Edge/EdgeModuleClientFactory.cs | 4 +- iothub/device/src/InternalClient.cs | 3 +- iothub/device/src/IotHubConnectionString.cs | 12 +- .../Amqp/AmqpAuthenticationRefresher.cs | 5 +- .../Transport/Amqp/AmqpConnectionHolder.cs | 169 +++++++++--------- .../src/Transport/Amqp/AmqpConnectionPool.cs | 9 + .../AmqpIot/AmqpIotCbsTokenProvider.cs | 23 ++- 8 files changed, 135 insertions(+), 96 deletions(-) diff --git a/iothub/device/src/AuthenticationWithTokenRefresh.cs b/iothub/device/src/AuthenticationWithTokenRefresh.cs index 37c862f3df..b922242f92 100644 --- a/iothub/device/src/AuthenticationWithTokenRefresh.cs +++ b/iothub/device/src/AuthenticationWithTokenRefresh.cs @@ -65,9 +65,9 @@ public AuthenticationWithTokenRefresh( /// public bool IsExpiring => (ExpiresOn - DateTime.UtcNow).TotalSeconds <= _bufferSeconds; - internal bool InstanceCreatedBySdk { get; set; } - - internal bool IsIndividualSasAuthenticatedToken { get; set; } + // This internal property is used by the sdk to determine if the instance was created by the sdk, + // and thus, if it should be disposed by the sdk. + internal bool ShouldSdkDisposeInstance { get; set; } /// /// Gets a snapshot of the security token associated with the device. This call is thread-safe. diff --git a/iothub/device/src/Edge/EdgeModuleClientFactory.cs b/iothub/device/src/Edge/EdgeModuleClientFactory.cs index 88b8978e8a..80c58728ab 100644 --- a/iothub/device/src/Edge/EdgeModuleClientFactory.cs +++ b/iothub/device/src/Edge/EdgeModuleClientFactory.cs @@ -100,8 +100,8 @@ private async Task CreateInternalClientFromEnvironmentAsync() #pragma warning disable CA2000 // Dispose objects before losing scope - IDisposable ModuleAuthenticationWithHsm is disposed when the client is disposed. var authMethod = new ModuleAuthenticationWithHsm(signatureProvider, deviceId, moduleId, generationId, sasTokenTimeToLive, sasTokenRenewalBuffer) { - InstanceCreatedBySdk = true, - IsIndividualSasAuthenticatedToken = true, + // Since the sdk creates the instance of disposable ModuleAuthenticationWithHsm, the sdk needs to dispose it once the client is diposed. + ShouldSdkDisposeInstance = true, }; #pragma warning restore CA2000 // Dispose objects before losing scope - IDisposable ModuleAuthenticationWithHsm is disposed when the client is disposed. diff --git a/iothub/device/src/InternalClient.cs b/iothub/device/src/InternalClient.cs index 295fe46647..6f60cca57f 100644 --- a/iothub/device/src/InternalClient.cs +++ b/iothub/device/src/InternalClient.cs @@ -1897,8 +1897,7 @@ public void Dispose() _deviceReceiveMessageSemaphore?.Dispose(); _twinDesiredPropertySemaphore?.Dispose(); - if ((IotHubConnectionString?.TokenRefresher?.InstanceCreatedBySdk).GetValueOrDefault() - && (IotHubConnectionString?.TokenRefresher?.IsIndividualSasAuthenticatedToken).GetValueOrDefault()) + if ((IotHubConnectionString?.TokenRefresher?.ShouldSdkDisposeInstance).GetValueOrDefault()) { IotHubConnectionString?.TokenRefresher?.Dispose(); } diff --git a/iothub/device/src/IotHubConnectionString.cs b/iothub/device/src/IotHubConnectionString.cs index cf609babea..ba7dbd6838 100644 --- a/iothub/device/src/IotHubConnectionString.cs +++ b/iothub/device/src/IotHubConnectionString.cs @@ -54,8 +54,10 @@ public IotHubConnectionString(IotHubConnectionStringBuilder builder) { TokenRefresher = new DeviceAuthenticationWithSakRefresh(DeviceId, this, builder.SasTokenTimeToLive, builder.SasTokenRenewalBuffer) { - InstanceCreatedBySdk = true, - IsIndividualSasAuthenticatedToken = builder.SharedAccessKeyName == null, + // Clients initialized using group sas tokens will have an entry for "SharedAccessKeyName" in the supplied connection string. + // These clients use a connection-wide TokenRefresher for authenticating all clients under the same group. + // In these cases, the disposal of the TokenRefresher is delegated to the transport layer. + ShouldSdkDisposeInstance = builder.SharedAccessKeyName == null, }; if (Logging.IsEnabled) @@ -67,8 +69,10 @@ public IotHubConnectionString(IotHubConnectionStringBuilder builder) { TokenRefresher = new ModuleAuthenticationWithSakRefresh(DeviceId, ModuleId, this, builder.SasTokenTimeToLive, builder.SasTokenRenewalBuffer) { - InstanceCreatedBySdk = true, - IsIndividualSasAuthenticatedToken = builder.SharedAccessKeyName == null, + // Clients initialized using group sas tokens will have an entry for "SharedAccessKeyName" in the supplied connection string. + // These clients use a connection-wide TokenRefresher for authenticating all clients under the same group. + // In these cases, the disposal of the TokenRefresher is delegated to the transport layer. + ShouldSdkDisposeInstance = builder.SharedAccessKeyName == null, }; if (Logging.IsEnabled) diff --git a/iothub/device/src/Transport/Amqp/AmqpAuthenticationRefresher.cs b/iothub/device/src/Transport/Amqp/AmqpAuthenticationRefresher.cs index 6c844c036b..46c7e9102b 100644 --- a/iothub/device/src/Transport/Amqp/AmqpAuthenticationRefresher.cs +++ b/iothub/device/src/Transport/Amqp/AmqpAuthenticationRefresher.cs @@ -173,7 +173,10 @@ private void Dispose(bool disposing) if (disposing) { StopLoop(); - _cancellationTokenSource.Dispose(); + _cancellationTokenSource?.Dispose(); + _cancellationTokenSource = null; + + _amqpIotCbsTokenProvider?.Dispose(); } _disposed = true; diff --git a/iothub/device/src/Transport/Amqp/AmqpConnectionHolder.cs b/iothub/device/src/Transport/Amqp/AmqpConnectionHolder.cs index 78922bb8b0..20b50c5cf9 100644 --- a/iothub/device/src/Transport/Amqp/AmqpConnectionHolder.cs +++ b/iothub/device/src/Transport/Amqp/AmqpConnectionHolder.cs @@ -66,80 +66,6 @@ public AmqpUnit CreateAmqpUnit( return amqpUnit; } - private void OnConnectionClosed(object o, EventArgs args) - { - if (Logging.IsEnabled) - { - Logging.Enter(this, o, nameof(OnConnectionClosed)); - } - - if (_amqpIotConnection != null && ReferenceEquals(_amqpIotConnection, o)) - { - _amqpAuthenticationRefresher?.StopLoop(); - HashSet amqpUnits; - lock (_unitsLock) - { - amqpUnits = new HashSet(_amqpUnits); - } - foreach (AmqpUnit unit in amqpUnits) - { - unit.OnConnectionDisconnected(); - } - } - if (Logging.IsEnabled) - { - Logging.Exit(this, o, nameof(OnConnectionClosed)); - } - } - - public void Shutdown() - { - if (Logging.IsEnabled) - { - Logging.Enter(this, _amqpIotConnection, nameof(Shutdown)); - } - - _amqpAuthenticationRefresher?.StopLoop(); - _amqpIotConnection?.SafeClose(); - if (Logging.IsEnabled) - { - Logging.Exit(this, _amqpIotConnection, nameof(Shutdown)); - } - } - - public void Dispose() - { - Dispose(true); - GC.SuppressFinalize(this); - } - - private void Dispose(bool disposing) - { - if (_disposed) - { - return; - } - - if (Logging.IsEnabled) - { - Logging.Info(this, disposing, nameof(Dispose)); - } - - if (disposing) - { - _amqpIotConnection?.SafeClose(); - _lock?.Dispose(); - _amqpIotConnector?.Dispose(); - lock (_unitsLock) - { - _amqpUnits.Clear(); - } - _amqpAuthenticationRefresher?.Dispose(); - } - - _disposed = true; - } - public async Task CreateRefresherAsync(DeviceIdentity deviceIdentity, TimeSpan timeout) { if (Logging.IsEnabled) @@ -206,19 +132,17 @@ public async Task EnsureConnectionAsync(TimeSpan timeout) // Create AmqpConnection amqpIotConnection = await _amqpIotConnector.OpenConnectionAsync(timeout).ConfigureAwait(false); - if (_deviceIdentity.AuthenticationModel != AuthenticationModel.X509) + if (_deviceIdentity.AuthenticationModel == AuthenticationModel.SasGrouped) { - if (_deviceIdentity.AuthenticationModel == AuthenticationModel.SasGrouped) + if (Logging.IsEnabled) { - if (Logging.IsEnabled) - { - Logging.Info(this, "Creating connection width AmqpAuthenticationRefresher", nameof(EnsureConnectionAsync)); - } - - amqpAuthenticationRefresher = new AmqpAuthenticationRefresher(_deviceIdentity, amqpIotConnection.GetCbsLink()); - await amqpAuthenticationRefresher.InitLoopAsync(timeout).ConfigureAwait(false); + Logging.Info(this, "Creating connection wide AmqpAuthenticationRefresher", nameof(EnsureConnectionAsync)); } + + amqpAuthenticationRefresher = new AmqpAuthenticationRefresher(_deviceIdentity, amqpIotConnection.GetCbsLink()); + await amqpAuthenticationRefresher.InitLoopAsync(timeout).ConfigureAwait(false); } + _amqpIotConnection = amqpIotConnection; _amqpAuthenticationRefresher = amqpAuthenticationRefresher; _amqpIotConnection.Closed += OnConnectionClosed; @@ -271,5 +195,84 @@ public void RemoveAmqpUnit(AmqpUnit amqpUnit) Logging.Exit(this, amqpUnit, nameof(RemoveAmqpUnit)); } } + + public void Shutdown() + { + if (Logging.IsEnabled) + { + Logging.Enter(this, _amqpIotConnection, nameof(Shutdown)); + } + + _amqpAuthenticationRefresher?.StopLoop(); + _amqpIotConnection?.SafeClose(); + if (Logging.IsEnabled) + { + Logging.Exit(this, _amqpIotConnection, nameof(Shutdown)); + } + } + + public void Dispose() + { + Dispose(true); + GC.SuppressFinalize(this); + } + + private void Dispose(bool disposing) + { + if (_disposed) + { + return; + } + + if (Logging.IsEnabled) + { + Logging.Info(this, disposing, nameof(Dispose)); + } + + if (disposing) + { + _amqpIotConnection?.SafeClose(); + _lock?.Dispose(); + _amqpIotConnector?.Dispose(); + lock (_unitsLock) + { + _amqpUnits.Clear(); + } + _amqpAuthenticationRefresher?.Dispose(); + } + + _disposed = true; + } + + internal DeviceIdentity GetDeviceIdentityOfAuthenticationProvider() + { + return _deviceIdentity; + } + + private void OnConnectionClosed(object o, EventArgs args) + { + if (Logging.IsEnabled) + { + Logging.Enter(this, o, nameof(OnConnectionClosed)); + } + + if (_amqpIotConnection != null && ReferenceEquals(_amqpIotConnection, o)) + { + _amqpAuthenticationRefresher?.StopLoop(); + HashSet amqpUnits; + lock (_unitsLock) + { + amqpUnits = new HashSet(_amqpUnits); + } + foreach (AmqpUnit unit in amqpUnits) + { + unit.OnConnectionDisconnected(); + } + } + if (Logging.IsEnabled) + { + Logging.Exit(this, o, nameof(OnConnectionClosed)); + } + } } } diff --git a/iothub/device/src/Transport/Amqp/AmqpConnectionPool.cs b/iothub/device/src/Transport/Amqp/AmqpConnectionPool.cs index 61baf37613..a6ed95e36b 100644 --- a/iothub/device/src/Transport/Amqp/AmqpConnectionPool.cs +++ b/iothub/device/src/Transport/Amqp/AmqpConnectionPool.cs @@ -36,6 +36,15 @@ public AmqpUnit CreateAmqpUnit( { AmqpConnectionHolder[] amqpConnectionHolders = ResolveConnectionGroup(deviceIdentity); amqpConnectionHolder = ResolveConnectionByHashing(amqpConnectionHolders, deviceIdentity); + + // For group sas token authenticated devices over a multiplexed connection, the identity + // of the first device connecting will be used for generating the group sas tokens. + // The TokenRefresher of the subsequently connected device identities can be safely disposed. + if (!ReferenceEquals(amqpConnectionHolder.GetDeviceIdentityOfAuthenticationProvider(), deviceIdentity) + && deviceIdentity.IotHubConnectionString?.TokenRefresher != null) + { + deviceIdentity.IotHubConnectionString.TokenRefresher.Dispose(); + } } if (Logging.IsEnabled) diff --git a/iothub/device/src/Transport/AmqpIot/AmqpIotCbsTokenProvider.cs b/iothub/device/src/Transport/AmqpIot/AmqpIotCbsTokenProvider.cs index 6aaae2affa..3555ada46c 100644 --- a/iothub/device/src/Transport/AmqpIot/AmqpIotCbsTokenProvider.cs +++ b/iothub/device/src/Transport/AmqpIot/AmqpIotCbsTokenProvider.cs @@ -9,9 +9,10 @@ namespace Microsoft.Azure.Devices.Client.Transport.AmqpIot { - internal class AmqpIotCbsTokenProvider : ICbsTokenProvider + internal class AmqpIotCbsTokenProvider : ICbsTokenProvider, IDisposable { private readonly IotHubConnectionString _connectionString; + private bool _disposedValue; public AmqpIotCbsTokenProvider(IotHubConnectionString connectionString) { @@ -57,5 +58,25 @@ public async Task GetTokenAsync(Uri namespaceAddress, string appliesTo } } } + + protected virtual void Dispose(bool disposing) + { + if (!_disposedValue) + { + if (disposing) + { + _connectionString.TokenRefresher.Dispose(); + } + + _disposedValue = true; + } + } + + public void Dispose() + { + // Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method + Dispose(disposing: true); + GC.SuppressFinalize(this); + } } } From 74d2b37541d1557f14b2a0b3e4f97f3bc8afe996 Mon Sep 17 00:00:00 2001 From: Abhipsa Misra Date: Thu, 13 May 2021 15:20:06 -0700 Subject: [PATCH 06/19] dispose only for grouped sas --- .../src/DeviceAuthenticationWithTokenRefresh.cs | 4 ++-- .../src/Transport/Amqp/AmqpConnectionPool.cs | 15 +++++++++------ 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/iothub/device/src/DeviceAuthenticationWithTokenRefresh.cs b/iothub/device/src/DeviceAuthenticationWithTokenRefresh.cs index 401d6f7a7d..b40e8b8c42 100644 --- a/iothub/device/src/DeviceAuthenticationWithTokenRefresh.cs +++ b/iothub/device/src/DeviceAuthenticationWithTokenRefresh.cs @@ -11,8 +11,8 @@ namespace Microsoft.Azure.Devices.Client /// public abstract class DeviceAuthenticationWithTokenRefresh : AuthenticationWithTokenRefresh { - internal const int DefaultTimeToLiveSeconds = 1 * 60 * 60; - internal const int DefaultBufferPercentage = 15; + internal const int DefaultTimeToLiveSeconds = 1 * 60; + internal const int DefaultBufferPercentage = 50; /// /// Initializes a new instance of the class using default diff --git a/iothub/device/src/Transport/Amqp/AmqpConnectionPool.cs b/iothub/device/src/Transport/Amqp/AmqpConnectionPool.cs index a6ed95e36b..0ec3b9bd75 100644 --- a/iothub/device/src/Transport/Amqp/AmqpConnectionPool.cs +++ b/iothub/device/src/Transport/Amqp/AmqpConnectionPool.cs @@ -37,13 +37,16 @@ public AmqpUnit CreateAmqpUnit( AmqpConnectionHolder[] amqpConnectionHolders = ResolveConnectionGroup(deviceIdentity); amqpConnectionHolder = ResolveConnectionByHashing(amqpConnectionHolders, deviceIdentity); - // For group sas token authenticated devices over a multiplexed connection, the identity - // of the first device connecting will be used for generating the group sas tokens. - // The TokenRefresher of the subsequently connected device identities can be safely disposed. - if (!ReferenceEquals(amqpConnectionHolder.GetDeviceIdentityOfAuthenticationProvider(), deviceIdentity) - && deviceIdentity.IotHubConnectionString?.TokenRefresher != null) + if (deviceIdentity.AuthenticationModel == AuthenticationModel.SasGrouped) { - deviceIdentity.IotHubConnectionString.TokenRefresher.Dispose(); + // For group sas token authenticated devices over a multiplexed connection, the identity + // of the first device connecting will be used for generating the group sas tokens. + // The TokenRefresher of the subsequently connected device identities can be safely disposed. + if (!ReferenceEquals(amqpConnectionHolder.GetDeviceIdentityOfAuthenticationProvider(), deviceIdentity) + && deviceIdentity.IotHubConnectionString?.TokenRefresher != null) + { + deviceIdentity.IotHubConnectionString.TokenRefresher.Dispose(); + } } } From 78d57645c2276c0670076e7d5fae50368e964807 Mon Sep 17 00:00:00 2001 From: Abhipsa Misra Date: Thu, 13 May 2021 15:22:36 -0700 Subject: [PATCH 07/19] this should not have been checked in - ttl change --- iothub/device/src/DeviceAuthenticationWithTokenRefresh.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/iothub/device/src/DeviceAuthenticationWithTokenRefresh.cs b/iothub/device/src/DeviceAuthenticationWithTokenRefresh.cs index b40e8b8c42..401d6f7a7d 100644 --- a/iothub/device/src/DeviceAuthenticationWithTokenRefresh.cs +++ b/iothub/device/src/DeviceAuthenticationWithTokenRefresh.cs @@ -11,8 +11,8 @@ namespace Microsoft.Azure.Devices.Client /// public abstract class DeviceAuthenticationWithTokenRefresh : AuthenticationWithTokenRefresh { - internal const int DefaultTimeToLiveSeconds = 1 * 60; - internal const int DefaultBufferPercentage = 50; + internal const int DefaultTimeToLiveSeconds = 1 * 60 * 60; + internal const int DefaultBufferPercentage = 15; /// /// Initializes a new instance of the class using default From a86aa3797e3e58b22888be6b75d266f6f81d3bf4 Mon Sep 17 00:00:00 2001 From: Abhipsa Misra Date: Thu, 13 May 2021 20:38:21 -0700 Subject: [PATCH 08/19] revert method ordering changes --- .../Transport/Amqp/AmqpConnectionHolder.cs | 152 +++++++++--------- 1 file changed, 76 insertions(+), 76 deletions(-) diff --git a/iothub/device/src/Transport/Amqp/AmqpConnectionHolder.cs b/iothub/device/src/Transport/Amqp/AmqpConnectionHolder.cs index 20b50c5cf9..f2dc7d5a20 100644 --- a/iothub/device/src/Transport/Amqp/AmqpConnectionHolder.cs +++ b/iothub/device/src/Transport/Amqp/AmqpConnectionHolder.cs @@ -66,6 +66,80 @@ public AmqpUnit CreateAmqpUnit( return amqpUnit; } + private void OnConnectionClosed(object o, EventArgs args) + { + if (Logging.IsEnabled) + { + Logging.Enter(this, o, nameof(OnConnectionClosed)); + } + + if (_amqpIotConnection != null && ReferenceEquals(_amqpIotConnection, o)) + { + _amqpAuthenticationRefresher?.StopLoop(); + HashSet amqpUnits; + lock (_unitsLock) + { + amqpUnits = new HashSet(_amqpUnits); + } + foreach (AmqpUnit unit in amqpUnits) + { + unit.OnConnectionDisconnected(); + } + } + if (Logging.IsEnabled) + { + Logging.Exit(this, o, nameof(OnConnectionClosed)); + } + } + + public void Shutdown() + { + if (Logging.IsEnabled) + { + Logging.Enter(this, _amqpIotConnection, nameof(Shutdown)); + } + + _amqpAuthenticationRefresher?.StopLoop(); + _amqpIotConnection?.SafeClose(); + if (Logging.IsEnabled) + { + Logging.Exit(this, _amqpIotConnection, nameof(Shutdown)); + } + } + + public void Dispose() + { + Dispose(true); + GC.SuppressFinalize(this); + } + + private void Dispose(bool disposing) + { + if (_disposed) + { + return; + } + + if (Logging.IsEnabled) + { + Logging.Info(this, disposing, nameof(Dispose)); + } + + if (disposing) + { + _amqpIotConnection?.SafeClose(); + _lock?.Dispose(); + _amqpIotConnector?.Dispose(); + lock (_unitsLock) + { + _amqpUnits.Clear(); + } + _amqpAuthenticationRefresher?.Dispose(); + } + + _disposed = true; + } + public async Task CreateRefresherAsync(DeviceIdentity deviceIdentity, TimeSpan timeout) { if (Logging.IsEnabled) @@ -136,7 +210,7 @@ public async Task EnsureConnectionAsync(TimeSpan timeout) { if (Logging.IsEnabled) { - Logging.Info(this, "Creating connection wide AmqpAuthenticationRefresher", nameof(EnsureConnectionAsync)); + Logging.Info(this, "Creating connection width AmqpAuthenticationRefresher", nameof(EnsureConnectionAsync)); } amqpAuthenticationRefresher = new AmqpAuthenticationRefresher(_deviceIdentity, amqpIotConnection.GetCbsLink()); @@ -196,83 +270,9 @@ public void RemoveAmqpUnit(AmqpUnit amqpUnit) } } - public void Shutdown() - { - if (Logging.IsEnabled) - { - Logging.Enter(this, _amqpIotConnection, nameof(Shutdown)); - } - - _amqpAuthenticationRefresher?.StopLoop(); - _amqpIotConnection?.SafeClose(); - if (Logging.IsEnabled) - { - Logging.Exit(this, _amqpIotConnection, nameof(Shutdown)); - } - } - - public void Dispose() - { - Dispose(true); - GC.SuppressFinalize(this); - } - - private void Dispose(bool disposing) - { - if (_disposed) - { - return; - } - - if (Logging.IsEnabled) - { - Logging.Info(this, disposing, nameof(Dispose)); - } - - if (disposing) - { - _amqpIotConnection?.SafeClose(); - _lock?.Dispose(); - _amqpIotConnector?.Dispose(); - lock (_unitsLock) - { - _amqpUnits.Clear(); - } - _amqpAuthenticationRefresher?.Dispose(); - } - - _disposed = true; - } - internal DeviceIdentity GetDeviceIdentityOfAuthenticationProvider() { return _deviceIdentity; } - - private void OnConnectionClosed(object o, EventArgs args) - { - if (Logging.IsEnabled) - { - Logging.Enter(this, o, nameof(OnConnectionClosed)); - } - - if (_amqpIotConnection != null && ReferenceEquals(_amqpIotConnection, o)) - { - _amqpAuthenticationRefresher?.StopLoop(); - HashSet amqpUnits; - lock (_unitsLock) - { - amqpUnits = new HashSet(_amqpUnits); - } - foreach (AmqpUnit unit in amqpUnits) - { - unit.OnConnectionDisconnected(); - } - } - if (Logging.IsEnabled) - { - Logging.Exit(this, o, nameof(OnConnectionClosed)); - } - } } -} +} \ No newline at end of file From 76f9555890eaff9d3603153722e4beaf31bb78ac Mon Sep 17 00:00:00 2001 From: Abhipsa Misra Date: Thu, 13 May 2021 20:44:45 -0700 Subject: [PATCH 09/19] typo --- iothub/device/src/Transport/Amqp/AmqpConnectionHolder.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/iothub/device/src/Transport/Amqp/AmqpConnectionHolder.cs b/iothub/device/src/Transport/Amqp/AmqpConnectionHolder.cs index f2dc7d5a20..c82660aed5 100644 --- a/iothub/device/src/Transport/Amqp/AmqpConnectionHolder.cs +++ b/iothub/device/src/Transport/Amqp/AmqpConnectionHolder.cs @@ -210,7 +210,7 @@ public async Task EnsureConnectionAsync(TimeSpan timeout) { if (Logging.IsEnabled) { - Logging.Info(this, "Creating connection width AmqpAuthenticationRefresher", nameof(EnsureConnectionAsync)); + Logging.Info(this, "Creating connection wide AmqpAuthenticationRefresher", nameof(EnsureConnectionAsync)); } amqpAuthenticationRefresher = new AmqpAuthenticationRefresher(_deviceIdentity, amqpIotConnection.GetCbsLink()); From cbf832fc34deb2781838f163100b9bc6acc72a77 Mon Sep 17 00:00:00 2001 From: Abhipsa Misra Date: Thu, 13 May 2021 20:53:29 -0700 Subject: [PATCH 10/19] fix e2e test namespace --- e2e/test/iothub/SasCredentialAuthenticationTests.cs | 2 +- e2e/test/iothub/TokenCredentialAuthenticationTests.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/e2e/test/iothub/SasCredentialAuthenticationTests.cs b/e2e/test/iothub/SasCredentialAuthenticationTests.cs index 56eb80e9a1..09881b57bb 100644 --- a/e2e/test/iothub/SasCredentialAuthenticationTests.cs +++ b/e2e/test/iothub/SasCredentialAuthenticationTests.cs @@ -21,7 +21,7 @@ using ClientOptions = Microsoft.Azure.Devices.Client.ClientOptions; -namespace Microsoft.Azure.Devices.E2ETests.iothub +namespace Microsoft.Azure.Devices.E2ETests.Iothub.Service { /// /// Tests to ensure authentication using SAS credential succeeds in all the clients. diff --git a/e2e/test/iothub/TokenCredentialAuthenticationTests.cs b/e2e/test/iothub/TokenCredentialAuthenticationTests.cs index 20cd4c1fb4..22d4448106 100644 --- a/e2e/test/iothub/TokenCredentialAuthenticationTests.cs +++ b/e2e/test/iothub/TokenCredentialAuthenticationTests.cs @@ -21,7 +21,7 @@ using ClientOptions = Microsoft.Azure.Devices.Client.ClientOptions; -namespace Microsoft.Azure.Devices.E2ETests.iothub +namespace Microsoft.Azure.Devices.E2ETests.Iothub.Service { /// /// Tests to ensure authentication using Azure active directory succeeds in all the clients. From dda03d44c00af073bfec5950030cb61d398e94e4 Mon Sep 17 00:00:00 2001 From: Abhipsa Misra Date: Fri, 14 May 2021 15:07:51 -0700 Subject: [PATCH 11/19] update implementation to dispose at transport layer --- .../src/AuthenticationWithTokenRefresh.cs | 2 +- .../src/Edge/EdgeModuleClientFactory.cs | 2 +- iothub/device/src/InternalClient.cs | 5 ---- iothub/device/src/IotHubConnectionString.cs | 12 +++------ .../Transport/Amqp/AmqpConnectionHolder.cs | 4 +++ .../src/Transport/Amqp/AmqpConnectionPool.cs | 21 ++++++++------- iothub/device/src/Transport/Amqp/AmqpUnit.cs | 4 +++ .../AmqpIot/AmqpIotCbsTokenProvider.cs | 26 +++++++++++-------- .../device/src/Transport/HttpClientHelper.cs | 19 +++++++++++++- .../src/Transport/HttpTransportHandler.cs | 10 +++++-- .../src/Transport/Mqtt/MqttIotHubAdapter.cs | 7 +++++ .../src/Transport/TransportHandlerFactory.cs | 2 +- 12 files changed, 75 insertions(+), 39 deletions(-) diff --git a/iothub/device/src/AuthenticationWithTokenRefresh.cs b/iothub/device/src/AuthenticationWithTokenRefresh.cs index b922242f92..4bd6585646 100644 --- a/iothub/device/src/AuthenticationWithTokenRefresh.cs +++ b/iothub/device/src/AuthenticationWithTokenRefresh.cs @@ -67,7 +67,7 @@ public AuthenticationWithTokenRefresh( // This internal property is used by the sdk to determine if the instance was created by the sdk, // and thus, if it should be disposed by the sdk. - internal bool ShouldSdkDisposeInstance { get; set; } + internal bool InstanceCreatedBySdk { get; set; } /// /// Gets a snapshot of the security token associated with the device. This call is thread-safe. diff --git a/iothub/device/src/Edge/EdgeModuleClientFactory.cs b/iothub/device/src/Edge/EdgeModuleClientFactory.cs index 80c58728ab..07fa16be6c 100644 --- a/iothub/device/src/Edge/EdgeModuleClientFactory.cs +++ b/iothub/device/src/Edge/EdgeModuleClientFactory.cs @@ -101,7 +101,7 @@ private async Task CreateInternalClientFromEnvironmentAsync() var authMethod = new ModuleAuthenticationWithHsm(signatureProvider, deviceId, moduleId, generationId, sasTokenTimeToLive, sasTokenRenewalBuffer) { // Since the sdk creates the instance of disposable ModuleAuthenticationWithHsm, the sdk needs to dispose it once the client is diposed. - ShouldSdkDisposeInstance = true, + InstanceCreatedBySdk = true, }; #pragma warning restore CA2000 // Dispose objects before losing scope - IDisposable ModuleAuthenticationWithHsm is disposed when the client is disposed. diff --git a/iothub/device/src/InternalClient.cs b/iothub/device/src/InternalClient.cs index 6f60cca57f..25a420da3c 100644 --- a/iothub/device/src/InternalClient.cs +++ b/iothub/device/src/InternalClient.cs @@ -1896,11 +1896,6 @@ public void Dispose() _fileUploadHttpTransportHandler?.Dispose(); _deviceReceiveMessageSemaphore?.Dispose(); _twinDesiredPropertySemaphore?.Dispose(); - - if ((IotHubConnectionString?.TokenRefresher?.ShouldSdkDisposeInstance).GetValueOrDefault()) - { - IotHubConnectionString?.TokenRefresher?.Dispose(); - } } internal bool IsE2EDiagnosticSupportedProtocol() diff --git a/iothub/device/src/IotHubConnectionString.cs b/iothub/device/src/IotHubConnectionString.cs index ba7dbd6838..c26f8b3d0e 100644 --- a/iothub/device/src/IotHubConnectionString.cs +++ b/iothub/device/src/IotHubConnectionString.cs @@ -54,10 +54,8 @@ public IotHubConnectionString(IotHubConnectionStringBuilder builder) { TokenRefresher = new DeviceAuthenticationWithSakRefresh(DeviceId, this, builder.SasTokenTimeToLive, builder.SasTokenRenewalBuffer) { - // Clients initialized using group sas tokens will have an entry for "SharedAccessKeyName" in the supplied connection string. - // These clients use a connection-wide TokenRefresher for authenticating all clients under the same group. - // In these cases, the disposal of the TokenRefresher is delegated to the transport layer. - ShouldSdkDisposeInstance = builder.SharedAccessKeyName == null, + // Since the sdk creates the instance of disposable DeviceAuthenticationWithSakRefresh, the sdk needs to dispose it once the client is diposed. + InstanceCreatedBySdk = true, }; if (Logging.IsEnabled) @@ -69,10 +67,8 @@ public IotHubConnectionString(IotHubConnectionStringBuilder builder) { TokenRefresher = new ModuleAuthenticationWithSakRefresh(DeviceId, ModuleId, this, builder.SasTokenTimeToLive, builder.SasTokenRenewalBuffer) { - // Clients initialized using group sas tokens will have an entry for "SharedAccessKeyName" in the supplied connection string. - // These clients use a connection-wide TokenRefresher for authenticating all clients under the same group. - // In these cases, the disposal of the TokenRefresher is delegated to the transport layer. - ShouldSdkDisposeInstance = builder.SharedAccessKeyName == null, + // Since the sdk creates the instance of disposable ModuleAuthenticationWithSakRefresh, the sdk needs to dispose it once the client is diposed. + InstanceCreatedBySdk = true, }; if (Logging.IsEnabled) diff --git a/iothub/device/src/Transport/Amqp/AmqpConnectionHolder.cs b/iothub/device/src/Transport/Amqp/AmqpConnectionHolder.cs index c82660aed5..44ce677d4e 100644 --- a/iothub/device/src/Transport/Amqp/AmqpConnectionHolder.cs +++ b/iothub/device/src/Transport/Amqp/AmqpConnectionHolder.cs @@ -76,6 +76,8 @@ private void OnConnectionClosed(object o, EventArgs args) if (_amqpIotConnection != null && ReferenceEquals(_amqpIotConnection, o)) { _amqpAuthenticationRefresher?.StopLoop(); + _amqpAuthenticationRefresher?.Dispose(); + HashSet amqpUnits; lock (_unitsLock) { @@ -100,6 +102,8 @@ public void Shutdown() } _amqpAuthenticationRefresher?.StopLoop(); + _amqpAuthenticationRefresher?.Dispose(); + _amqpIotConnection?.SafeClose(); if (Logging.IsEnabled) { diff --git a/iothub/device/src/Transport/Amqp/AmqpConnectionPool.cs b/iothub/device/src/Transport/Amqp/AmqpConnectionPool.cs index 0ec3b9bd75..94b58c3dfc 100644 --- a/iothub/device/src/Transport/Amqp/AmqpConnectionPool.cs +++ b/iothub/device/src/Transport/Amqp/AmqpConnectionPool.cs @@ -37,16 +37,19 @@ public AmqpUnit CreateAmqpUnit( AmqpConnectionHolder[] amqpConnectionHolders = ResolveConnectionGroup(deviceIdentity); amqpConnectionHolder = ResolveConnectionByHashing(amqpConnectionHolders, deviceIdentity); - if (deviceIdentity.AuthenticationModel == AuthenticationModel.SasGrouped) + // For group sas token authenticated devices over a multiplexed connection, the TokenRefresher + // of the first client connecting will be used for generating the group sas tokens + // and will be associated with the connection itself. + // For this reason, if the device identity of the client is not the one associated with the + // connection, the associated TokenRefresher can be safely disposed. + // Note - This does not cause any identity related issues since the group sas tokens are generated + // against "{IoT hub name}.azure-devices.net/" as the intended audience (without the "device Id"). + if (deviceIdentity.AuthenticationModel == AuthenticationModel.SasGrouped + && !ReferenceEquals(amqpConnectionHolder.GetDeviceIdentityOfAuthenticationProvider(), deviceIdentity) + && deviceIdentity.IotHubConnectionString?.TokenRefresher != null + && deviceIdentity.IotHubConnectionString.TokenRefresher.InstanceCreatedBySdk) { - // For group sas token authenticated devices over a multiplexed connection, the identity - // of the first device connecting will be used for generating the group sas tokens. - // The TokenRefresher of the subsequently connected device identities can be safely disposed. - if (!ReferenceEquals(amqpConnectionHolder.GetDeviceIdentityOfAuthenticationProvider(), deviceIdentity) - && deviceIdentity.IotHubConnectionString?.TokenRefresher != null) - { - deviceIdentity.IotHubConnectionString.TokenRefresher.Dispose(); - } + deviceIdentity.IotHubConnectionString.TokenRefresher.Dispose(); } } diff --git a/iothub/device/src/Transport/Amqp/AmqpUnit.cs b/iothub/device/src/Transport/Amqp/AmqpUnit.cs index 359424eb1a..2193e41c8e 100644 --- a/iothub/device/src/Transport/Amqp/AmqpUnit.cs +++ b/iothub/device/src/Transport/Amqp/AmqpUnit.cs @@ -850,6 +850,10 @@ private void Dispose(bool disposing) _amqpConnectionHolder?.Dispose(); } + // For device sas authenticated clients the authentication refresher is associated with the AMQP unit itself, + // so it needs to be explicitly disposed. + _amqpAuthenticationRefresher?.Dispose(); + _sessionSemaphore?.Dispose(); _messageReceivingLinkSemaphore?.Dispose(); _messageReceivingCallbackSemaphore?.Dispose(); diff --git a/iothub/device/src/Transport/AmqpIot/AmqpIotCbsTokenProvider.cs b/iothub/device/src/Transport/AmqpIot/AmqpIotCbsTokenProvider.cs index 3555ada46c..047f2558b4 100644 --- a/iothub/device/src/Transport/AmqpIot/AmqpIotCbsTokenProvider.cs +++ b/iothub/device/src/Transport/AmqpIot/AmqpIotCbsTokenProvider.cs @@ -12,7 +12,7 @@ namespace Microsoft.Azure.Devices.Client.Transport.AmqpIot internal class AmqpIotCbsTokenProvider : ICbsTokenProvider, IDisposable { private readonly IotHubConnectionString _connectionString; - private bool _disposedValue; + private bool _isDisposed; public AmqpIotCbsTokenProvider(IotHubConnectionString connectionString) { @@ -59,24 +59,28 @@ public async Task GetTokenAsync(Uri namespaceAddress, string appliesTo } } + public void Dispose() + { + // Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method + Dispose(true); + GC.SuppressFinalize(this); + } + protected virtual void Dispose(bool disposing) { - if (!_disposedValue) + if (!_isDisposed) { if (disposing) { - _connectionString.TokenRefresher.Dispose(); + if (_connectionString?.TokenRefresher != null + && _connectionString.TokenRefresher.InstanceCreatedBySdk) + { + _connectionString.TokenRefresher.Dispose(); + } } - _disposedValue = true; + _isDisposed = true; } } - - public void Dispose() - { - // Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method - Dispose(disposing: true); - GC.SuppressFinalize(this); - } } } diff --git a/iothub/device/src/Transport/HttpClientHelper.cs b/iothub/device/src/Transport/HttpClientHelper.cs index 24c17cb92d..b93f38a63d 100644 --- a/iothub/device/src/Transport/HttpClientHelper.cs +++ b/iothub/device/src/Transport/HttpClientHelper.cs @@ -41,6 +41,7 @@ internal sealed class HttpClientHelper : IHttpClientHelper private HttpClientHandler _httpClientHandler; private bool _isDisposed; private readonly ProductInfo _productInfo; + private readonly bool _isClientPrimaryTransportHandler; public HttpClientHelper( Uri baseAddress, @@ -51,7 +52,8 @@ public HttpClientHelper( X509Certificate2 clientCert, HttpClientHandler httpClientHandler, ProductInfo productInfo, - IWebProxy proxy + IWebProxy proxy, + bool isClientPrimaryTransportHandler = false ) { _baseAddress = baseAddress; @@ -115,6 +117,7 @@ IWebProxy proxy preRequestActionForAllRequests?.Invoke(_httpClientObj); _productInfo = productInfo; + _isClientPrimaryTransportHandler = isClientPrimaryTransportHandler; } public Task GetAsync( @@ -538,6 +541,20 @@ public void Dispose() _httpClientHandler = null; } + // The associated TokenRefresher should be disposed by the http client helper only when the http client + // is the primary transport handler. + // For eg. we create HttpTransportHandler instances for file upload operations even though the client might be + // initialized via MQTT/ AMQP. In those scenarios, since the shared TokenRefresher resource would be primarily used by the + // corresponding transport layers (MQTT/ AMQP), the diposal should be delegated to them and it should not be disposed here. + // The only scenario where the TokenRefresher should be disposed here is when the client has been initialized using HTTP. + if (_isClientPrimaryTransportHandler + && _authenticationHeaderProvider is IotHubConnectionString iotHubConnectionString + && iotHubConnectionString.TokenRefresher != null + && iotHubConnectionString.TokenRefresher.InstanceCreatedBySdk) + { + iotHubConnectionString.TokenRefresher.Dispose(); + } + _isDisposed = true; } } diff --git a/iothub/device/src/Transport/HttpTransportHandler.cs b/iothub/device/src/Transport/HttpTransportHandler.cs index b57150cdc9..cee9c71665 100644 --- a/iothub/device/src/Transport/HttpTransportHandler.cs +++ b/iothub/device/src/Transport/HttpTransportHandler.cs @@ -60,7 +60,12 @@ internal sealed class HttpTransportHandler : TransportHandler private readonly string _deviceId; private readonly string _moduleId; - internal HttpTransportHandler(IPipelineContext context, IotHubConnectionString iotHubConnectionString, Http1TransportSettings transportSettings, HttpClientHandler httpClientHandler = null) + internal HttpTransportHandler( + IPipelineContext context, + IotHubConnectionString iotHubConnectionString, + Http1TransportSettings transportSettings, + HttpClientHandler httpClientHandler = null, + bool isClientPrimaryTransportHandler = false) : base(context, transportSettings) { ProductInfo productInfo = context.Get(); @@ -75,7 +80,8 @@ internal HttpTransportHandler(IPipelineContext context, IotHubConnectionString i transportSettings.ClientCertificate, httpClientHandler, productInfo, - transportSettings.Proxy); + transportSettings.Proxy, + isClientPrimaryTransportHandler); } public override Task OpenAsync(TimeoutHelper timeoutHelper) diff --git a/iothub/device/src/Transport/Mqtt/MqttIotHubAdapter.cs b/iothub/device/src/Transport/Mqtt/MqttIotHubAdapter.cs index af9a82fa73..93c322dca5 100644 --- a/iothub/device/src/Transport/Mqtt/MqttIotHubAdapter.cs +++ b/iothub/device/src/Transport/Mqtt/MqttIotHubAdapter.cs @@ -1017,6 +1017,13 @@ private async void ShutdownAsync(IChannelHandlerContext context) } finally { + if (_passwordProvider is IotHubConnectionString iotHubConnectionString + && iotHubConnectionString.TokenRefresher != null + && iotHubConnectionString.TokenRefresher.InstanceCreatedBySdk) + { + iotHubConnectionString.TokenRefresher.Dispose(); + } + if (Logging.IsEnabled) Logging.Exit(this, context.Name, nameof(ShutdownAsync)); } diff --git a/iothub/device/src/Transport/TransportHandlerFactory.cs b/iothub/device/src/Transport/TransportHandlerFactory.cs index 93969eabda..eb096a7dcd 100644 --- a/iothub/device/src/Transport/TransportHandlerFactory.cs +++ b/iothub/device/src/Transport/TransportHandlerFactory.cs @@ -37,7 +37,7 @@ public IDelegatingHandler Create(IPipelineContext context) new Func(onDeviceMessageReceivedCallback)); case TransportType.Http1: - return new HttpTransportHandler(context, connectionString, transportSetting as Http1TransportSettings); + return new HttpTransportHandler(context, connectionString, transportSetting as Http1TransportSettings, isClientPrimaryTransportHandler: true); case TransportType.Mqtt_Tcp_Only: case TransportType.Mqtt_WebSocket_Only: From 6e67431bf595d28880e0263229726fb9bb07970f Mon Sep 17 00:00:00 2001 From: Abhipsa Misra Date: Fri, 14 May 2021 21:08:41 -0700 Subject: [PATCH 12/19] another stop loop before disposing --- iothub/device/src/Transport/Amqp/AmqpUnit.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/iothub/device/src/Transport/Amqp/AmqpUnit.cs b/iothub/device/src/Transport/Amqp/AmqpUnit.cs index 2193e41c8e..5708b3eba3 100644 --- a/iothub/device/src/Transport/Amqp/AmqpUnit.cs +++ b/iothub/device/src/Transport/Amqp/AmqpUnit.cs @@ -852,6 +852,7 @@ private void Dispose(bool disposing) // For device sas authenticated clients the authentication refresher is associated with the AMQP unit itself, // so it needs to be explicitly disposed. + _amqpAuthenticationRefresher?.StopLoop(); _amqpAuthenticationRefresher?.Dispose(); _sessionSemaphore?.Dispose(); From 0428aea41057f4629de2cec9d44873d8f779b9ec Mon Sep 17 00:00:00 2001 From: Abhipsa Misra Date: Mon, 17 May 2021 09:07:07 -0700 Subject: [PATCH 13/19] test --- iothub/device/src/Transport/Amqp/AmqpConnectionHolder.cs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/iothub/device/src/Transport/Amqp/AmqpConnectionHolder.cs b/iothub/device/src/Transport/Amqp/AmqpConnectionHolder.cs index 44ce677d4e..c82660aed5 100644 --- a/iothub/device/src/Transport/Amqp/AmqpConnectionHolder.cs +++ b/iothub/device/src/Transport/Amqp/AmqpConnectionHolder.cs @@ -76,8 +76,6 @@ private void OnConnectionClosed(object o, EventArgs args) if (_amqpIotConnection != null && ReferenceEquals(_amqpIotConnection, o)) { _amqpAuthenticationRefresher?.StopLoop(); - _amqpAuthenticationRefresher?.Dispose(); - HashSet amqpUnits; lock (_unitsLock) { @@ -102,8 +100,6 @@ public void Shutdown() } _amqpAuthenticationRefresher?.StopLoop(); - _amqpAuthenticationRefresher?.Dispose(); - _amqpIotConnection?.SafeClose(); if (Logging.IsEnabled) { From bb113120f6029af6135664d71934bc5b97c18ae6 Mon Sep 17 00:00:00 2001 From: Abhipsa Misra Date: Mon, 17 May 2021 11:36:26 -0700 Subject: [PATCH 14/19] e2e tests --- ...enticationWithTokenRefreshDisposalTests.cs | 185 ++++++++++++++++++ 1 file changed, 185 insertions(+) create mode 100644 e2e/test/iothub/AuthenticationWithTokenRefreshDisposalTests.cs diff --git a/e2e/test/iothub/AuthenticationWithTokenRefreshDisposalTests.cs b/e2e/test/iothub/AuthenticationWithTokenRefreshDisposalTests.cs new file mode 100644 index 0000000000..a643d04b8b --- /dev/null +++ b/e2e/test/iothub/AuthenticationWithTokenRefreshDisposalTests.cs @@ -0,0 +1,185 @@ +// 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.Collections.Generic; +using System.Globalization; +using System.Net; +using System.Threading.Tasks; +using Microsoft.Azure.Devices.Client; +using Microsoft.Azure.Devices.E2ETests.Helpers; +using Microsoft.VisualStudio.TestTools.UnitTesting; + +namespace Microsoft.Azure.Devices.E2ETests.Iothub +{ + [TestClass] + [TestCategory("E2E")] + [TestCategory("IoTHub")] + public class AuthenticationWithTokenRefreshDisposalTests : E2EMsTestBase + { + private readonly string _devicePrefix = $"E2E_{nameof(AuthenticationWithTokenRefreshDisposalTests)}_"; + + [LoggedTestMethod] + public async Task DeviceSak_ReusableAuthenticationMethod_SingleDevicePerConnection_Amqp() + { + await ReuseAuthenticationMethod_SingleDevice(Client.TransportType.Amqp_Tcp_Only).ConfigureAwait(false); + } + + [LoggedTestMethod] + public async Task DeviceSak_ReusableAuthenticationMethod_SingleDevicePerConnection_AmqpWs() + { + await ReuseAuthenticationMethod_SingleDevice(Client.TransportType.Amqp_WebSocket_Only).ConfigureAwait(false); + } + + [LoggedTestMethod] + public async Task DeviceSak_ReusableAuthenticationMethod_SingleDevicePerConnection_Mqtt() + { + await ReuseAuthenticationMethod_SingleDevice(Client.TransportType.Mqtt_Tcp_Only).ConfigureAwait(false); + } + + [LoggedTestMethod] + public async Task DeviceSak_ReusableAuthenticationMethod_SingleDevicePerConnection_MqttWs() + { + await ReuseAuthenticationMethod_SingleDevice(Client.TransportType.Mqtt_WebSocket_Only).ConfigureAwait(false); + } + + [LoggedTestMethod] + public async Task DeviceSak_ReusableAuthenticationMethod_SingleDevicePerConnection_Http() + { + await ReuseAuthenticationMethod_SingleDevice(Client.TransportType.Http1).ConfigureAwait(false); + } + + [LoggedTestMethod] + public async Task DeviceSak_ReusableAuthenticationMethod_MuxedDevicesPerConnection_Amqp() + { + await ReuseAuthenticationMethod_MuxedDevices(Client.TransportType.Amqp_Tcp_Only, 2); + } + + [LoggedTestMethod] + public async Task DeviceSak_ReusableAuthenticationMethod_MuxedDevicesPerConnection_AmqpWs() + { + await ReuseAuthenticationMethod_MuxedDevices(Client.TransportType.Amqp_WebSocket_Only, 2); + } + + private async Task ReuseAuthenticationMethod_SingleDevice(Client.TransportType transport) + { + TestDevice testDevice = await TestDevice.GetTestDeviceAsync(Logger, _devicePrefix).ConfigureAwait(false); + var authenticationMethod = new DeviceAuthenticationSasToken(testDevice.ConnectionString); + + // Create an instance of the device client, send a test message and then close and dispose it. + DeviceClient deviceClient = DeviceClient.Create(testDevice.IoTHubHostName, authenticationMethod, transport); + await deviceClient.SendEventAsync(new Client.Message()).ConfigureAwait(false); + await deviceClient.CloseAsync(); + deviceClient.Dispose(); + Logger.Trace("Test with instance 1 completed"); + + // Perform the same steps again, reusing the previously created authentication method instance. + DeviceClient deviceClient2 = DeviceClient.Create(testDevice.IoTHubHostName, authenticationMethod, transport); + await deviceClient2.SendEventAsync(new Client.Message()).ConfigureAwait(false); + await deviceClient2.CloseAsync(); + deviceClient2.Dispose(); + Logger.Trace("Test with instance 2 completed"); + } + + private async Task ReuseAuthenticationMethod_MuxedDevices(Client.TransportType transport, int devicesCount) + { + IList testDevices = new List(); + IList authenticationMethods = new List(); + + // Set up amqp transport settings to multiplex all device sessions over the same amqp connection. + var amqpTransportSettings = new AmqpTransportSettings(transport) + { + AmqpConnectionPoolSettings = new AmqpConnectionPoolSettings + { + Pooling = true, + MaxPoolSize = 1, + }, + }; + + for (int i = 0; i < devicesCount; i++) + { + TestDevice testDevice = await TestDevice.GetTestDeviceAsync(Logger, _devicePrefix).ConfigureAwait(false); + var authenticationMethod = new DeviceAuthenticationSasToken(testDevice.ConnectionString); + + testDevices.Add(testDevice); + authenticationMethods.Add(authenticationMethod); + } + + // Create an instance of the device client, send a test message and then close and dispose it. + for (int i = 0; i < devicesCount; i++) + { + DeviceClient deviceClient = DeviceClient.Create(testDevices[i].IoTHubHostName, authenticationMethods[i], new ITransportSettings[] { amqpTransportSettings }); + await deviceClient.SendEventAsync(new Client.Message()).ConfigureAwait(false); + await deviceClient.CloseAsync(); + deviceClient.Dispose(); + Logger.Trace($"Test with client {i} completed."); + } + Logger.Trace($"Test run with instance 1 completed."); + + // Perform the same steps again, reusing the previously created authentication method instance. + for (int i = 0; i < devicesCount; i++) + { + DeviceClient deviceClient = DeviceClient.Create(testDevices[i].IoTHubHostName, authenticationMethods[i], new ITransportSettings[] { amqpTransportSettings }); + await deviceClient.SendEventAsync(new Client.Message()).ConfigureAwait(false); + await deviceClient.CloseAsync(); + deviceClient.Dispose(); + Logger.Trace($"Test with client {i} completed."); + } + Logger.Trace($"Test run with instance 2 completed."); + } + + private class DeviceAuthenticationSasToken : DeviceAuthenticationWithTokenRefresh + { + private const string SasTokenTargetFormat = "{0}/devices/{1}"; + private readonly IotHubConnectionStringBuilder _connectionStringBuilder; + + public DeviceAuthenticationSasToken( + string connectionString) + : base(GetDeviceIdFromConnectionString(connectionString)) + { + if (connectionString == null) + { + throw new ArgumentNullException(nameof(connectionString)); + } + + _connectionStringBuilder = IotHubConnectionStringBuilder.Create(connectionString); + } + + protected override Task SafeCreateNewToken(string iotHub, int suggestedTimeToLive) + { + var builder = new SharedAccessSignatureBuilder + { + Key = _connectionStringBuilder.SharedAccessKey, + TimeToLive = TimeSpan.FromSeconds(suggestedTimeToLive), + }; + + if (_connectionStringBuilder.SharedAccessKeyName == null) + { + builder.Target = string.Format( + CultureInfo.InvariantCulture, + SasTokenTargetFormat, + iotHub, + WebUtility.UrlEncode(DeviceId)); + } + else + { + builder.KeyName = _connectionStringBuilder.SharedAccessKeyName; + builder.Target = _connectionStringBuilder.HostName; + } + + return Task.FromResult(builder.ToSignature()); + } + + private static string GetDeviceIdFromConnectionString(string connectionString) + { + if (connectionString == null) + { + throw new ArgumentNullException(nameof(connectionString)); + } + + var builder = IotHubConnectionStringBuilder.Create(connectionString); + return builder.DeviceId; + } + } + } +} From cd62559fbcd8692d47a723c76e7a3491351debc9 Mon Sep 17 00:00:00 2001 From: Abhipsa Misra Date: Mon, 17 May 2021 13:44:58 -0700 Subject: [PATCH 15/19] update test --- .../Helpers/AmqpConnectionStatusChange.cs | 36 ++++++++ .../FaultInjectionPoolingOverAmqp.cs | 29 ------ ...enticationWithTokenRefreshDisposalTests.cs | 91 ++++++++++++++++--- 3 files changed, 115 insertions(+), 41 deletions(-) create mode 100644 e2e/test/Helpers/AmqpConnectionStatusChange.cs diff --git a/e2e/test/Helpers/AmqpConnectionStatusChange.cs b/e2e/test/Helpers/AmqpConnectionStatusChange.cs new file mode 100644 index 0000000000..0dd5c220b4 --- /dev/null +++ b/e2e/test/Helpers/AmqpConnectionStatusChange.cs @@ -0,0 +1,36 @@ +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Microsoft.Azure.Devices.Client; + +namespace Microsoft.Azure.Devices.E2ETests.Helpers +{ + public class AmqpConnectionStatusChange + { + private readonly string _deviceId; + private readonly MsTestLogger _logger; + + public AmqpConnectionStatusChange(string deviceId, MsTestLogger logger) + { + LastConnectionStatus = null; + LastConnectionStatusChangeReason = null; + ConnectionStatusChangeCount = 0; + _deviceId = deviceId; + _logger = logger; + } + + public void ConnectionStatusChangesHandler(ConnectionStatus status, ConnectionStatusChangeReason reason) + { + ConnectionStatusChangeCount++; + LastConnectionStatus = status; + LastConnectionStatusChangeReason = reason; + _logger.Trace($"{nameof(AmqpConnectionStatusChange)}.{nameof(ConnectionStatusChangesHandler)}: {_deviceId}: status={status} statusChangeReason={reason} count={ConnectionStatusChangeCount}"); + } + + public int ConnectionStatusChangeCount { get; set; } + + public ConnectionStatus? LastConnectionStatus { get; set; } + + public ConnectionStatusChangeReason? LastConnectionStatusChangeReason { get; set; } + } +} diff --git a/e2e/test/Helpers/templates/FaultInjectionPoolingOverAmqp.cs b/e2e/test/Helpers/templates/FaultInjectionPoolingOverAmqp.cs index d1dbb5fce2..2691508e85 100644 --- a/e2e/test/Helpers/templates/FaultInjectionPoolingOverAmqp.cs +++ b/e2e/test/Helpers/templates/FaultInjectionPoolingOverAmqp.cs @@ -217,34 +217,5 @@ public static async Task TestFaultInjectionPoolAmqpAsync( } } } - - public class AmqpConnectionStatusChange - { - private readonly string _deviceId; - private readonly MsTestLogger _logger; - - public AmqpConnectionStatusChange(string deviceId, MsTestLogger logger) - { - LastConnectionStatus = null; - LastConnectionStatusChangeReason = null; - ConnectionStatusChangeCount = 0; - _deviceId = deviceId; - _logger = logger; - } - - public void ConnectionStatusChangesHandler(ConnectionStatus status, ConnectionStatusChangeReason reason) - { - ConnectionStatusChangeCount++; - LastConnectionStatus = status; - LastConnectionStatusChangeReason = reason; - _logger.Trace($"{nameof(FaultInjectionPoolingOverAmqp)}.{nameof(ConnectionStatusChangesHandler)}: {_deviceId}: status={status} statusChangeReason={reason} count={ConnectionStatusChangeCount}"); - } - - public int ConnectionStatusChangeCount { get; set; } - - public ConnectionStatus? LastConnectionStatus { get; set; } - - public ConnectionStatusChangeReason? LastConnectionStatusChangeReason { get; set; } - } } } diff --git a/e2e/test/iothub/AuthenticationWithTokenRefreshDisposalTests.cs b/e2e/test/iothub/AuthenticationWithTokenRefreshDisposalTests.cs index a643d04b8b..5aa61e8368 100644 --- a/e2e/test/iothub/AuthenticationWithTokenRefreshDisposalTests.cs +++ b/e2e/test/iothub/AuthenticationWithTokenRefreshDisposalTests.cs @@ -3,20 +3,23 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.Globalization; using System.Net; using System.Threading.Tasks; +using FluentAssertions; using Microsoft.Azure.Devices.Client; using Microsoft.Azure.Devices.E2ETests.Helpers; using Microsoft.VisualStudio.TestTools.UnitTesting; -namespace Microsoft.Azure.Devices.E2ETests.Iothub +namespace Microsoft.Azure.Devices.E2ETests { [TestClass] [TestCategory("E2E")] [TestCategory("IoTHub")] public class AuthenticationWithTokenRefreshDisposalTests : E2EMsTestBase { + public static readonly TimeSpan MaxWaitTime = TimeSpan.FromSeconds(10); private readonly string _devicePrefix = $"E2E_{nameof(AuthenticationWithTokenRefreshDisposalTests)}_"; [LoggedTestMethod] @@ -79,12 +82,16 @@ private async Task ReuseAuthenticationMethod_SingleDevice(Client.TransportType t await deviceClient2.CloseAsync(); deviceClient2.Dispose(); Logger.Trace("Test with instance 2 completed"); + + authenticationMethod.Dispose(); } private async Task ReuseAuthenticationMethod_MuxedDevices(Client.TransportType transport, int devicesCount) { IList testDevices = new List(); + IList deviceClients = new List(); IList authenticationMethods = new List(); + IList amqpConnectionStatuses = new List(); // Set up amqp transport settings to multiplex all device sessions over the same amqp connection. var amqpTransportSettings = new AmqpTransportSettings(transport) @@ -105,27 +112,87 @@ private async Task ReuseAuthenticationMethod_MuxedDevices(Client.TransportType t authenticationMethods.Add(authenticationMethod); } - // Create an instance of the device client, send a test message and then close and dispose it. + // Initialize the client instances set the connection status change handler and open the connection. for (int i = 0; i < devicesCount; i++) { DeviceClient deviceClient = DeviceClient.Create(testDevices[i].IoTHubHostName, authenticationMethods[i], new ITransportSettings[] { amqpTransportSettings }); - await deviceClient.SendEventAsync(new Client.Message()).ConfigureAwait(false); - await deviceClient.CloseAsync(); - deviceClient.Dispose(); + + var amqpConnectionStatusChange = new AmqpConnectionStatusChange(testDevices[i].Id, Logger); + deviceClient.SetConnectionStatusChangesHandler(amqpConnectionStatusChange.ConnectionStatusChangesHandler); + amqpConnectionStatuses.Add(amqpConnectionStatusChange); + + await deviceClient.OpenAsync().ConfigureAwait(false); + deviceClients.Add(deviceClient); + } + + // Close and dispose client instance 1. + // The closed client should report a status of "disabled" while the rest of them should be connected. + + await deviceClients[0].CloseAsync().ConfigureAwait(false); + deviceClients[0].Dispose(); + + amqpConnectionStatuses[0].LastConnectionStatus.Should().Be(ConnectionStatus.Disabled); + + Logger.Trace($"{nameof(ReuseAuthenticationMethod_MuxedDevices)}: Confirming the rest of the multiplexed devices are online and operational."); + + bool notRecovered = true; + var sw = Stopwatch.StartNew(); + while (notRecovered && sw.Elapsed < MaxWaitTime) + { + notRecovered = false; + for (int i = 1; i < devicesCount; i++) + { + if (amqpConnectionStatuses[i].LastConnectionStatus != ConnectionStatus.Connected) + { + await Task.Delay(TimeSpan.FromSeconds(1)).ConfigureAwait(false); + notRecovered = true; + break; + } + } + } + + notRecovered.Should().BeFalse(); + + // Send a message through the rest of the multiplexed client instances. + for (int i = 1; i < devicesCount; i++) + { + await deviceClients[i].SendEventAsync(new Client.Message()).ConfigureAwait(false); Logger.Trace($"Test with client {i} completed."); } - Logger.Trace($"Test run with instance 1 completed."); - // Perform the same steps again, reusing the previously created authentication method instance. + // Close and dispose all of the client instances. + for (int i = 1; i < devicesCount; i++) + { + await deviceClients[i].CloseAsync().ConfigureAwait(false); + deviceClients[i].Dispose(); + } + + deviceClients.Clear(); + amqpConnectionStatuses.Clear(); + + // Initialize the client instances by reusing the created authentication methods and open the connection. for (int i = 0; i < devicesCount; i++) { DeviceClient deviceClient = DeviceClient.Create(testDevices[i].IoTHubHostName, authenticationMethods[i], new ITransportSettings[] { amqpTransportSettings }); - await deviceClient.SendEventAsync(new Client.Message()).ConfigureAwait(false); - await deviceClient.CloseAsync(); - deviceClient.Dispose(); - Logger.Trace($"Test with client {i} completed."); + + var amqpConnectionStatusChange = new AmqpConnectionStatusChange(testDevices[i].Id, Logger); + deviceClient.SetConnectionStatusChangesHandler(amqpConnectionStatusChange.ConnectionStatusChangesHandler); + amqpConnectionStatuses.Add(amqpConnectionStatusChange); + + await deviceClient.OpenAsync().ConfigureAwait(false); + deviceClients.Add(deviceClient); + } + + // Ensure that all clients are connected successfully, and the close and dispose the instances. + for (int i = 0; i < devicesCount; i++) + { + amqpConnectionStatuses[i].LastConnectionStatus.Should().Be(ConnectionStatus.Connected); + + await deviceClients[i].CloseAsync(); + deviceClients[i].Dispose(); + + amqpConnectionStatuses[i].LastConnectionStatus.Should().Be(ConnectionStatus.Disabled); } - Logger.Trace($"Test run with instance 2 completed."); } private class DeviceAuthenticationSasToken : DeviceAuthenticationWithTokenRefresh From 8c5b2c1f68a787216b6f403cafcfed1662f27ecc Mon Sep 17 00:00:00 2001 From: Abhipsa Misra Date: Tue, 25 May 2021 11:54:09 -0700 Subject: [PATCH 16/19] add IDisposable guide to dev doc --- doc/devguide.md | 5 +++++ .../iothub/AuthenticationWithTokenRefreshDisposalTests.cs | 2 +- iothub/device/src/Transport/Amqp/AmqpConnectionPool.cs | 2 +- 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/doc/devguide.md b/doc/devguide.md index bf66d30086..095dbffaa1 100644 --- a/doc/devguide.md +++ b/doc/devguide.md @@ -9,6 +9,11 @@ Before starting development on the Azure IoT SDK for C# you will need to install ### Design We are following the [Azure SDK design specification for C#](https://azuresdkspecs.z5.web.core.windows.net/DotNetSpec.html). To preserve backward compatibility, existing code will not change to follow these rules. +#### Using [`IDisposable`](https://docs.microsoft.com/en-us/dotnet/api/system.idisposable?view=net-5.0#implementing-idisposable) types: +- The sdk should dispose any `IDisposable` resource that it creates. +- The sdk should not dispose an `IDisposable` resource that is supplied by the calling application. This is because the caller might want to reuse the resource elsewhere in the application. The responsibility of disposal of caller-supplied `IDisposable` resource is on the caller. An example of such a resource would be an `X509Certificate` instance that is used for authenticating our clients. +- If the sdk implements `class A` that owns an `IDisposable` resource `X`, then `A` should also be `IDisposable`. In that case, resource `X` can be safely disposed when `class A` is disposed. + ### Code style Please read and apply our [coding style](coding-style.md) when proposing PRs against the Azure repository. When changing existing files, please apply changes to the entire file. Otherwise, maintain the same style. diff --git a/e2e/test/iothub/AuthenticationWithTokenRefreshDisposalTests.cs b/e2e/test/iothub/AuthenticationWithTokenRefreshDisposalTests.cs index 5aa61e8368..f93d034d4f 100644 --- a/e2e/test/iothub/AuthenticationWithTokenRefreshDisposalTests.cs +++ b/e2e/test/iothub/AuthenticationWithTokenRefreshDisposalTests.cs @@ -112,7 +112,7 @@ private async Task ReuseAuthenticationMethod_MuxedDevices(Client.TransportType t authenticationMethods.Add(authenticationMethod); } - // Initialize the client instances set the connection status change handler and open the connection. + // Initialize the client instances, set the connection status change handler and open the connection. for (int i = 0; i < devicesCount; i++) { DeviceClient deviceClient = DeviceClient.Create(testDevices[i].IoTHubHostName, authenticationMethods[i], new ITransportSettings[] { amqpTransportSettings }); diff --git a/iothub/device/src/Transport/Amqp/AmqpConnectionPool.cs b/iothub/device/src/Transport/Amqp/AmqpConnectionPool.cs index 94b58c3dfc..1916e4e4b0 100644 --- a/iothub/device/src/Transport/Amqp/AmqpConnectionPool.cs +++ b/iothub/device/src/Transport/Amqp/AmqpConnectionPool.cs @@ -43,7 +43,7 @@ public AmqpUnit CreateAmqpUnit( // For this reason, if the device identity of the client is not the one associated with the // connection, the associated TokenRefresher can be safely disposed. // Note - This does not cause any identity related issues since the group sas tokens are generated - // against "{IoT hub name}.azure-devices.net/" as the intended audience (without the "device Id"). + // against the hub host as the intended audience (without the "device Id"). if (deviceIdentity.AuthenticationModel == AuthenticationModel.SasGrouped && !ReferenceEquals(amqpConnectionHolder.GetDeviceIdentityOfAuthenticationProvider(), deviceIdentity) && deviceIdentity.IotHubConnectionString?.TokenRefresher != null From ed1084e245c821b6115f87283cacb1e895ee4c6e Mon Sep 17 00:00:00 2001 From: Abhipsa Misra Date: Tue, 25 May 2021 14:25:51 -0700 Subject: [PATCH 17/19] some more comments --- e2e/test/Helpers/AmqpConnectionStatusChange.cs | 12 ++++++------ .../AuthenticationWithTokenRefreshDisposalTests.cs | 5 +++-- iothub/device/src/IotHubConnectionString.cs | 7 +++++++ 3 files changed, 16 insertions(+), 8 deletions(-) diff --git a/e2e/test/Helpers/AmqpConnectionStatusChange.cs b/e2e/test/Helpers/AmqpConnectionStatusChange.cs index 0dd5c220b4..60c5e7a0b9 100644 --- a/e2e/test/Helpers/AmqpConnectionStatusChange.cs +++ b/e2e/test/Helpers/AmqpConnectionStatusChange.cs @@ -19,6 +19,12 @@ public AmqpConnectionStatusChange(string deviceId, MsTestLogger logger) _logger = logger; } + public int ConnectionStatusChangeCount { get; set; } + + public ConnectionStatus? LastConnectionStatus { get; set; } + + public ConnectionStatusChangeReason? LastConnectionStatusChangeReason { get; set; } + public void ConnectionStatusChangesHandler(ConnectionStatus status, ConnectionStatusChangeReason reason) { ConnectionStatusChangeCount++; @@ -26,11 +32,5 @@ public void ConnectionStatusChangesHandler(ConnectionStatus status, ConnectionSt LastConnectionStatusChangeReason = reason; _logger.Trace($"{nameof(AmqpConnectionStatusChange)}.{nameof(ConnectionStatusChangesHandler)}: {_deviceId}: status={status} statusChangeReason={reason} count={ConnectionStatusChangeCount}"); } - - public int ConnectionStatusChangeCount { get; set; } - - public ConnectionStatus? LastConnectionStatus { get; set; } - - public ConnectionStatusChangeReason? LastConnectionStatusChangeReason { get; set; } } } diff --git a/e2e/test/iothub/AuthenticationWithTokenRefreshDisposalTests.cs b/e2e/test/iothub/AuthenticationWithTokenRefreshDisposalTests.cs index f93d034d4f..e0ac4d5cf7 100644 --- a/e2e/test/iothub/AuthenticationWithTokenRefreshDisposalTests.cs +++ b/e2e/test/iothub/AuthenticationWithTokenRefreshDisposalTests.cs @@ -76,12 +76,13 @@ private async Task ReuseAuthenticationMethod_SingleDevice(Client.TransportType t deviceClient.Dispose(); Logger.Trace("Test with instance 1 completed"); - // Perform the same steps again, reusing the previously created authentication method instance. + // Perform the same steps again, reusing the previously created authentication method instance to ensure + // that the sdk did not dispose the user supplied authentication method instance. DeviceClient deviceClient2 = DeviceClient.Create(testDevice.IoTHubHostName, authenticationMethod, transport); await deviceClient2.SendEventAsync(new Client.Message()).ConfigureAwait(false); await deviceClient2.CloseAsync(); deviceClient2.Dispose(); - Logger.Trace("Test with instance 2 completed"); + Logger.Trace("Test with instance 2 completed, reused the previously created authentication method instance for the device client."); authenticationMethod.Dispose(); } diff --git a/iothub/device/src/IotHubConnectionString.cs b/iothub/device/src/IotHubConnectionString.cs index c26f8b3d0e..e52e01d243 100644 --- a/iothub/device/src/IotHubConnectionString.cs +++ b/iothub/device/src/IotHubConnectionString.cs @@ -84,6 +84,13 @@ public IotHubConnectionString(IotHubConnectionStringBuilder builder) Debug.Assert(TokenRefresher != null); } + // SharedAccessSignature should be set only if it is non-null and the authentication method of the device client is + // not of type AuthenticationWithTokenRefresh. + // Setting the sas value for an AuthenticationWithTokenRefresh authentication type will result in tokens not being renewed. + // This flow can be hit if the same authentication method is always used to initialize the client; + // as in, on disposal and reinitialization. This is because the value of the sas token computed is stored within the authentication method, + // and on reinitialization the client is incorrectly identified as a fixed-sas-token-initialized client, + // instead of being identified as a sas-token-refresh-enabled-client. else if (!string.IsNullOrWhiteSpace(builder.SharedAccessSignature)) { SharedAccessSignature = builder.SharedAccessSignature; From 6a677cfa7b9db6fea364cd7469044e372daecfee Mon Sep 17 00:00:00 2001 From: Abhipsa Misra Date: Tue, 25 May 2021 14:32:53 -0700 Subject: [PATCH 18/19] reorder --- doc/devguide.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/devguide.md b/doc/devguide.md index 095dbffaa1..9ea8998538 100644 --- a/doc/devguide.md +++ b/doc/devguide.md @@ -10,9 +10,9 @@ Before starting development on the Azure IoT SDK for C# you will need to install We are following the [Azure SDK design specification for C#](https://azuresdkspecs.z5.web.core.windows.net/DotNetSpec.html). To preserve backward compatibility, existing code will not change to follow these rules. #### Using [`IDisposable`](https://docs.microsoft.com/en-us/dotnet/api/system.idisposable?view=net-5.0#implementing-idisposable) types: +- If the sdk implements `class A` that owns an `IDisposable` resource `X`, then `A` should also be `IDisposable`. In that case, resource `X` can be safely disposed when `class A` is disposed. - The sdk should dispose any `IDisposable` resource that it creates. - The sdk should not dispose an `IDisposable` resource that is supplied by the calling application. This is because the caller might want to reuse the resource elsewhere in the application. The responsibility of disposal of caller-supplied `IDisposable` resource is on the caller. An example of such a resource would be an `X509Certificate` instance that is used for authenticating our clients. -- If the sdk implements `class A` that owns an `IDisposable` resource `X`, then `A` should also be `IDisposable`. In that case, resource `X` can be safely disposed when `class A` is disposed. ### Code style Please read and apply our [coding style](coding-style.md) when proposing PRs against the Azure repository. When changing existing files, please apply changes to the entire file. Otherwise, maintain the same style. From 75a3e9160176545bf103416a1f11c57957491a96 Mon Sep 17 00:00:00 2001 From: Abhipsa Misra Date: Tue, 25 May 2021 14:36:45 -0700 Subject: [PATCH 19/19] comment --- e2e/test/iothub/AuthenticationWithTokenRefreshDisposalTests.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/e2e/test/iothub/AuthenticationWithTokenRefreshDisposalTests.cs b/e2e/test/iothub/AuthenticationWithTokenRefreshDisposalTests.cs index e0ac4d5cf7..a686f9b321 100644 --- a/e2e/test/iothub/AuthenticationWithTokenRefreshDisposalTests.cs +++ b/e2e/test/iothub/AuthenticationWithTokenRefreshDisposalTests.cs @@ -128,6 +128,8 @@ private async Task ReuseAuthenticationMethod_MuxedDevices(Client.TransportType t // Close and dispose client instance 1. // The closed client should report a status of "disabled" while the rest of them should be connected. + // This is to ensure that disposal on one multiplexed device doesn't cause cascading failures + // in the rest of the devices on the same tcp connection. await deviceClients[0].CloseAsync().ConfigureAwait(false); deviceClients[0].Dispose();