Update client to not dispose user supplied IDisposable types #1954
Conversation
abhipsaMisra
requested review from
azabbasi,
barustum,
bikamani,
drwill-ms,
jamdavi,
timtay-microsoft and
vinagesh
as code owners
| @@ -65,6 +65,10 @@ public abstract class AuthenticationWithTokenRefresh : IAuthenticationMethod, ID | |||
| /// </summary> | |||
| public bool IsExpiring => (ExpiresOn - DateTime.UtcNow).TotalSeconds <= _bufferSeconds; | |||
|
|
|||
| // 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; } | |||
abhipsaMisra
force-pushed
the
abmisr/tokenRefresh
branch
from
b2f1425 to
4379eaa
Compare
|
TODO: E2E tests with fault injection? |
| @@ -21,7 +21,7 @@ | |||
|
|
|||
| using ClientOptions = Microsoft.Azure.Devices.Client.ClientOptions; | |||
|
|
|||
| namespace Microsoft.Azure.Devices.E2ETests.iothub | |||
| namespace Microsoft.Azure.Devices.E2ETests.Iothub.Service | |||
There was a problem hiding this comment.
minor tangential change - this test was placed under an incorrect namespace
| _cancellationTokenSource?.Dispose(); | ||
| _cancellationTokenSource = null; | ||
|
|
||
| _amqpIotCbsTokenProvider?.Dispose(); |
There was a problem hiding this comment.
the token provider is disposed when the token refresher is disposed
| @@ -206,19 +206,17 @@ public async Task<AmqpIotConnection> EnsureConnectionAsync(TimeSpan timeout) | |||
| // Create AmqpConnection | |||
| amqpIotConnection = await _amqpIotConnector.OpenConnectionAsync(timeout).ConfigureAwait(false); | |||
|
|
|||
| if (_deviceIdentity.AuthenticationModel != AuthenticationModel.X509) | |||
| if (_deviceIdentity.AuthenticationModel == AuthenticationModel.SasGrouped) | |||
There was a problem hiding this comment.
_deviceIdentity.AuthenticationModel is an enum, so we can simplify this check
There was a problem hiding this comment.
What about AuthenticationModel.SasIndividual ? Does it not apply to that?
There was a problem hiding this comment.
for sas individual authenticated devices, the token refresher is associated with the amqp unit: https://github.com/Azure/azure-iot-sdk-csharp/blob/master/iothub/device/src/Transport/Amqp/AmqpUnit.cs#L119
sas individual => amqp unit
sas group => amqp connection
x509 => no token refresher (and also, no multiplexing)
| // 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(); |
There was a problem hiding this comment.
stop and dispose the authentication refresher on disposal of an amqp unit
| @@ -9,9 +9,10 @@ | |||
|
|
|||
| namespace Microsoft.Azure.Devices.Client.Transport.AmqpIot | |||
| { | |||
| internal class AmqpIotCbsTokenProvider : ICbsTokenProvider | |||
| internal class AmqpIotCbsTokenProvider : ICbsTokenProvider, IDisposable | |||
There was a problem hiding this comment.
AmqpIotCbsTokenProvider contains an IDisposable type, so making it IDisposable as well
| && iotHubConnectionString.TokenRefresher != null | ||
| && iotHubConnectionString.TokenRefresher.InstanceCreatedBySdk) | ||
| { | ||
| iotHubConnectionString.TokenRefresher.Dispose(); |
There was a problem hiding this comment.
if applicable, dispose the token refresher when the http client helper is disposed
| && iotHubConnectionString.TokenRefresher != null | ||
| && iotHubConnectionString.TokenRefresher.InstanceCreatedBySdk) | ||
| { | ||
| iotHubConnectionString.TokenRefresher.Dispose(); |
There was a problem hiding this comment.
if applicable, dispose the token refresher when the mqtt iot hub adapter shuts down.
Note that even though we have a token refresher associated with the mqtt connection, it isn't a "true" refresher in the sense that it doesn't monitor token expiration and doesn't initiate a preemptive renewal. The token here is refreshed only on CONNECT (if expired).
abhipsaMisra
force-pushed
the
abmisr/tokenRefresh
branch
from
9c0bc12 to
cd62559
Compare
| // 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"). |
There was a problem hiding this comment.
Not if US Gov't cloud, or in the future with vanity domains.
There was a problem hiding this comment.
In that case, I'll edit to to simply say "hub hostname".
| deviceClients[0].Dispose(); | ||
|
|
||
| amqpConnectionStatuses[0].LastConnectionStatus.Should().Be(ConnectionStatus.Disabled); | ||
|
|
There was a problem hiding this comment.
Could also add amqpConnectionStatuses[1].LastConnectionStatus.Should().Be(ConnectionStatus.Connected);
There was a problem hiding this comment.
we actually check the status of the entire list of client instances right below
| // 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 the hub host as the intended audience (without the "device Id"). | ||
| if (deviceIdentity.AuthenticationModel == AuthenticationModel.SasGrouped |
There was a problem hiding this comment.
Is there no way to avoid creating these for mux scenarios instead of disposing the unnecessary ones? Looks like all we need is a 1SAS token for the connection irrespective of the devices in the mux.
There was a problem hiding this comment.
There isn't, no. The client is "set up" in a different layer (in internal client, before the whole concept of transport pipeline comes into picture), and this is where the token refresher is set up. This layer doesn't have any visibility about multiplexing, which is a transport-level construct.
|
|
||
| var builder = IotHubConnectionStringBuilder.Create(connectionString); | ||
| return builder.DeviceId; | ||
| } |
There was a problem hiding this comment.
These tests are doing a lot more than just checking for reusing the disposable created external to the SDK. Is that because we don't have tests that cover these mux scenarios? If they already exist, it would be nice simplify to just testing the reusability.
There was a problem hiding this comment.
We have tests that cover basic mux and pooling scenarios (all operations), and that cover fault injection when devices have opted for multiplexing (and pooling).
These new tests are designed to reuse the authentication method, while still testing that disposal on one multiplexed device doesn't cause cascading failures in the rest of the devices on the same tcp connection.
There was a problem hiding this comment.
This seems more explanatory. Can you just add this in comments?
There was a problem hiding this comment.
sure, I'll add this comment in
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This PR fixes two issues:
IotHubConnectionString.TokenRefresheris anIDispoableIAuthenticationMethod.AuthenticationWithTokenRefresh, which can be user-supplied. So, we shouldn't dispose it within our client, since they might want to reuse it.Reference: https://docs.microsoft.com/en-us/dotnet/api/system.idisposable?view=net-5.0#implementing-idisposable
"You should implement IDisposable only if your type uses unmanaged resources directly. The consumers of your type can call your IDisposable.Dispose implementation to free resources when the instance is no longer needed."
IotHubConnectionString.SharedAccessSignatureshould be set only if it is non-null and the authentication method of the device client is not of typeAuthenticationWithTokenRefresh. Setting the sas value for anAuthenticationWithTokenRefreshauthentication 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. (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.)Fix for #1911
Dependency code map of
IAuthenticationMethod.AuthenticationWithTokenRefresh: