-
Notifications
You must be signed in to change notification settings - Fork 493
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.
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
feat(provisioning-device, prov-amqp, prov-mqtt, prov-https): Add support for timespan timeouts to provisioning device client #2041
Changes from all commits
72dca89
811a414
3fec16f
a66ca2a
2b930c1
ea002d9
6510d7d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
|
||
using Microsoft.Azure.Devices.Provisioning.Client.Transport; | ||
using Microsoft.Azure.Devices.Shared; | ||
using System; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
|
||
|
@@ -63,52 +64,87 @@ private ProvisioningDeviceClient( | |
/// <summary> | ||
/// Registers the current device using the Device Provisioning Service and assigns it to an IoT Hub. | ||
/// </summary> | ||
/// <param name="timeout">The maximum amount of time to allow this operation to run for before timing out.</param> | ||
/// <remarks> | ||
/// Due to the AMQP library used by this library uses not accepting cancellation tokens, this overload and <see cref="RegisterAsync(ProvisioningRegistrationAdditionalData, TimeSpan)"/> | ||
/// are the only overloads for this method that allow for a specified timeout to be respected in the middle of an AMQP operation such as opening | ||
/// the AMQP connection. MQTT and HTTPS connections do not share that same limitation, though. | ||
/// </remarks> | ||
/// <returns>The registration result.</returns> | ||
public Task<DeviceRegistrationResult> RegisterAsync() | ||
public Task<DeviceRegistrationResult> RegisterAsync(TimeSpan timeout) | ||
{ | ||
return RegisterAsync(CancellationToken.None); | ||
return RegisterAsync(null, timeout); | ||
} | ||
|
||
/// <summary> | ||
/// Registers the current device using the Device Provisioning Service and assigns it to an IoT Hub. | ||
/// </summary> | ||
/// <param name="data">The optional additional data.</param> | ||
/// <param name="data"> | ||
/// The optional additional data that is passed through to the custom allocation policy webhook if | ||
/// a custom allocation policy webhook is setup for this enrollment. | ||
/// </param> | ||
/// <param name="timeout">The maximum amount of time to allow this operation to run for before timing out.</param> | ||
/// <remarks> | ||
/// Due to the AMQP library used by this library uses not accepting cancellation tokens, this overload and <see cref="RegisterAsync(TimeSpan)"/> | ||
/// are the only overloads for this method that allow for a specified timeout to be respected in the middle of an AMQP operation such as opening | ||
/// the AMQP connection. MQTT and HTTPS connections do not share that same limitation, though. | ||
/// </remarks> | ||
/// <returns>The registration result.</returns> | ||
public Task<DeviceRegistrationResult> RegisterAsync(ProvisioningRegistrationAdditionalData data) | ||
public Task<DeviceRegistrationResult> RegisterAsync(ProvisioningRegistrationAdditionalData data, TimeSpan timeout) | ||
{ | ||
return RegisterAsync(data, CancellationToken.None); | ||
Logging.RegisterAsync(this, _globalDeviceEndpoint, _idScope, _transport, _security); | ||
|
||
var request = new ProvisioningTransportRegisterMessage(_globalDeviceEndpoint, _idScope, _security, data?.JsonData) | ||
{ | ||
ProductInfo = ProductInfo, | ||
}; | ||
|
||
return _transport.RegisterAsync(request, timeout); | ||
} | ||
|
||
/// <summary> | ||
/// Registers the current device using the Device Provisioning Service and assigns it to an IoT Hub. | ||
/// </summary> | ||
/// <param name="cancellationToken">The cancellation token.</param> | ||
/// <remarks> | ||
/// Due to the AMQP library used by this library uses not accepting cancellation tokens, the provided cancellation token will only be checked | ||
/// for cancellation in between AMQP operations, and not during. In order to have a timeout for this operation that is checked during AMQP operations | ||
/// (such as opening the connection), you must use <see cref="RegisterAsync(TimeSpan)"/> instead. MQTT and HTTPS connections do not have the same | ||
/// behavior as AMQP connections in this regard. MQTT and HTTPS connections will check this cancellation token for cancellation during their protocol level operations. | ||
/// </remarks> | ||
/// <returns>The registration result.</returns> | ||
public Task<DeviceRegistrationResult> RegisterAsync(CancellationToken cancellationToken) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure why we didn't use optional parameters here before, but I'm assuming public Task<DeviceRegistrationResult> RegisterAsync(CancellationToken cancellationToken = default) is equivalent to public Task<DeviceRegistrationResult> RegisterAsync();
public Task<DeviceRegistrationResult> RegisterAsync(CancellationToken cancellationToken); When CancellationToken.None is passed as the token from RegisterAsync() to RegisterAsync(CancellationToken) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is, but not binary compatible. Could be seen as a breaking change. I'd be okay with it, though. Simply requires recompile. |
||
public Task<DeviceRegistrationResult> RegisterAsync(CancellationToken cancellationToken = default) | ||
{ | ||
Logging.RegisterAsync(this, _globalDeviceEndpoint, _idScope, _transport, _security); | ||
|
||
var request = new ProvisioningTransportRegisterMessage(_globalDeviceEndpoint, _idScope, _security) | ||
{ | ||
ProductInfo = ProductInfo | ||
}; | ||
return _transport.RegisterAsync(request, cancellationToken); | ||
return RegisterAsync(null, cancellationToken); | ||
} | ||
|
||
/// <summary> | ||
/// Registers the current device using the Device Provisioning Service and assigns it to an IoT Hub. | ||
/// </summary> | ||
/// <param name="data">The custom content.</param> | ||
/// <param name="data"> | ||
/// The optional additional data that is passed through to the custom allocation policy webhook if | ||
/// a custom allocation policy webhook is setup for this enrollment. | ||
/// </param> | ||
/// <param name="cancellationToken">The cancellation token.</param> | ||
/// <remarks> | ||
/// Due to the AMQP library used by this library uses not accepting cancellation tokens, the provided cancellation token will only be checked | ||
/// for cancellation in between AMQP operations, and not during. In order to have a timeout for this operation that is checked during AMQP operations | ||
/// (such as opening the connection), you must use <see cref="RegisterAsync(ProvisioningRegistrationAdditionalData, TimeSpan)">this overload</see> instead. | ||
/// MQTT and HTTPS connections do not have the same behavior as AMQP connections in this regard. MQTT and HTTPS connections will check this cancellation | ||
/// token for cancellation during their protocol level operations. | ||
/// </remarks> | ||
/// <returns>The registration result.</returns> | ||
public Task<DeviceRegistrationResult> RegisterAsync(ProvisioningRegistrationAdditionalData data, CancellationToken cancellationToken) | ||
public Task<DeviceRegistrationResult> RegisterAsync(ProvisioningRegistrationAdditionalData data, CancellationToken cancellationToken = default) | ||
{ | ||
Logging.RegisterAsync(this, _globalDeviceEndpoint, _idScope, _transport, _security); | ||
|
||
var request = new ProvisioningTransportRegisterMessage(_globalDeviceEndpoint, _idScope, _security, data?.JsonData) | ||
{ | ||
ProductInfo = ProductInfo, | ||
}; | ||
|
||
return _transport.RegisterAsync(request, cancellationToken); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was tempted to make the TimeSpan an optional parameter here like the cancellation token is, but we can't have two overloads with the same first parameter and different optional parameters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm
Could always add TimeSpan as another optional parameter to the one that has a CancellationToken. Would that be less weird?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could go either way on this suggestion. If we ever get cancellation token support for our AMQP library, then we can just deprecate all the APIs that take TimeSpans, but if we start mixing TimeSpans and Cancellation tokens then it gets messy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to keep the timespan and cancellation token APIs separate. Since both are ways to signal an API that it should stop the in-progress operation gracefully, I don't think we should mix them together.