Skip to content
New issue

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

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

[Core] Improve OperationHelpers used by our LROs #19105

Merged
merged 48 commits into from
May 14, 2021
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
57ae23f
OperationInternal
kinelski Apr 19, 2021
5b901ce
Updated OperationInternal
kinelski Apr 20, 2021
40d6450
Updated FormRecognizer
kinelski Apr 20, 2021
e7046fd
Added KeyVault Keys
kinelski Apr 20, 2021
e8cc4d4
Updated OperationInternal with AddAttributes
kinelski Apr 20, 2021
adfb122
Updated FormRecognizer
kinelski Apr 20, 2021
a5f4284
Added Synapse
kinelski Apr 20, 2021
7727103
Added TextAnalytics
kinelski Apr 20, 2021
2664ce8
Updated FR to new format
kinelski Apr 27, 2021
90b8f44
Updated KV Keys to new format
kinelski Apr 27, 2021
d2efbb2
Updated TA to new format
kinelski Apr 27, 2021
cf75dd6
Update Synapse to new format
kinelski Apr 27, 2021
43bc320
Added RetryAfter header handling
kinelski Apr 27, 2021
d88ea8a
Updated OperationInternal
kinelski Apr 27, 2021
1ebb67f
Merge branch 'master' into eng-lro
kinelski May 3, 2021
d3b2e32
Addressed part of comments
kinelski May 3, 2021
88a8b55
Added readonly struct approach
kinelski May 3, 2021
e8b467e
Merged GetResponse and UpdateState into single method
kinelski May 3, 2021
bcd9e65
Added UpdateStatus tests
kinelski May 4, 2021
f9681d8
Added WaitForCompletion testS
kinelski May 4, 2021
ce7f2bf
Updated tests
kinelski May 4, 2021
177728e
Updated Synapse and KeyVault to new model
kinelski May 5, 2021
1c6c8ad
Fixed null ref test bug
kinelski May 6, 2021
c367892
Added ms retry header support
kinelski May 6, 2021
7265d61
Added op failed default exception
kinelski May 6, 2021
91744d5
Moved custom type name to ctor
kinelski May 6, 2021
30bed6a
Merge branch 'master' into eng-lro
kinelski May 6, 2021
3ec6997
Made Value and HasCompleted not settable
kinelski May 6, 2021
10e2d11
Flow exception out instead of wrapping it
kinelski May 6, 2021
c24a61f
Added rawResponse to ctor
kinelski May 7, 2021
d693567
Update sdk/keyvault/Azure.Security.KeyVault.Keys/src/RecoverDeletedKe…
kinelski May 7, 2021
cd68031
Update sdk/core/Azure.Core/src/Shared/OperationInternal.cs
kinelski May 7, 2021
c13cc96
Update sdk/core/Azure.Core/tests/OperationInternalTests.cs
kinelski May 7, 2021
db816ea
Updates (tests, max delay)
kinelski May 7, 2021
0bba0d0
Merge branch 'master' into eng-lro
kinelski May 7, 2021
caed979
Removed KeyVault
kinelski May 7, 2021
3ee4d45
No more var
kinelski May 11, 2021
2bf26c7
Removed var everywhere
kinelski May 11, 2021
7fb7bce
Removed bool?
kinelski May 11, 2021
41de045
Merged master
kinelski May 13, 2021
02fe9d8
Removed dictionary instantiation
kinelski May 13, 2021
9b55b43
Updated tests
kinelski May 13, 2021
6943a0d
Added too many comments
kinelski May 14, 2021
4fdaeb6
Added OperationState tests
kinelski May 14, 2021
bebbbbd
Added null validation to OperationState
kinelski May 14, 2021
eb2401e
Merge branch 'master' into eng-lro
kinelski May 14, 2021
63262b7
Fixed docstring issues
kinelski May 14, 2021
4a4fc76
Removed TA and Synapse
kinelski May 14, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
212 changes: 212 additions & 0 deletions sdk/core/Azure.Core/src/Shared/OperationInternal.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,212 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System;
using System.Collections.Generic;
using System.Threading;
using System.Threading.Tasks;
using Azure.Core.Pipeline;

namespace Azure.Core
{
internal class OperationInternal<TResult>
Copy link
Member Author

Choose a reason for hiding this comment

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

Naming it OperationInternal for now. We can't use OperationHelpers because we still have some clients using the name.

Copy link
Member Author

Choose a reason for hiding this comment

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

We currently have: OperationInternal<TResult> and OperationInternal<TResult, TResponseType>.

  • OperationInternal<TResult> receives a Response from the service, and exposes the Value property as a TResult.
  • OperationInternal<TResult, TResponseType> receives a Response<ResponseType> from the service, and exposes the Value property as a TResult.

Copy link
Member

Choose a reason for hiding this comment

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

We can't use OperationHelpers because we still have some clients using the name.

Is there a plan to move everyone so eventually OperationHelpers class can be deleted and we avoid confusion?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. That's the plan. We'll probably keep the OperationHelpers name as well.

{
private const string RetryAfterHeaderName = "Retry-After";
kinelski marked this conversation as resolved.
Show resolved Hide resolved
private const string RetryAfterMsHeaderName = "retry-after-ms";
private const string XRetryAfterMsHeaderName = "x-ms-retry-after-ms";

private readonly IOperation<TResult> _operation;

private readonly ClientDiagnostics _diagnostics;

private readonly string _updateStatusScopeName;

private TResult _value;

private RequestFailedException _operationFailedException;

public OperationInternal(ClientDiagnostics clientDiagnostics, IOperation<TResult> operation, Response rawResponse, string operationTypeName = null)
{
operationTypeName ??= operation.GetType().Name;

_operation = operation;
_diagnostics = clientDiagnostics;
_updateStatusScopeName = $"{operationTypeName}.UpdateStatus";
RawResponse = rawResponse;
DefaultPollingInterval = TimeSpan.FromSeconds(1);
pakrym marked this conversation as resolved.
Show resolved Hide resolved
ScopeAttributes = new Dictionary<string, string>();
}

public IDictionary<string, string> ScopeAttributes { get; }

public bool HasValue { get; private set; }

public bool HasCompleted { get; private set; }

public TResult Value
{
get
{
if (HasValue)
{
return _value;
}
else if (_operationFailedException != null)
{
throw _operationFailedException;
}
else
{
throw new InvalidOperationException("The operation has not completed yet.");
}
}
private set
{
if (value is null)
{
throw new ArgumentNullException(nameof(Value));
}

_value = value;
HasValue = true;
}
}

public Response RawResponse { get; set; }
kinelski marked this conversation as resolved.
Show resolved Hide resolved

public TimeSpan DefaultPollingInterval { get; set; }

public async ValueTask<Response> UpdateStatusAsync(CancellationToken cancellationToken) =>
await UpdateStatusAsync(async: true, cancellationToken).ConfigureAwait(false);

public Response UpdateStatus(CancellationToken cancellationToken) =>
UpdateStatusAsync(async: false, cancellationToken).EnsureCompleted();

public async ValueTask<Response<TResult>> WaitForCompletionAsync(CancellationToken cancellationToken) =>
await WaitForCompletionAsync(DefaultPollingInterval, cancellationToken).ConfigureAwait(false);

public async ValueTask<Response<TResult>> WaitForCompletionAsync(TimeSpan pollingInterval, CancellationToken cancellationToken)
{
while (true)
{
Response response = await UpdateStatusAsync(cancellationToken).ConfigureAwait(false);

if (HasCompleted)
{
return Response.FromValue(Value, response);
}

var serverDelay = GetServerDelay(response);

var delay = serverDelay > pollingInterval
kinelski marked this conversation as resolved.
Show resolved Hide resolved
? serverDelay : pollingInterval;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var delay = serverDelay > pollingInterval
? serverDelay : pollingInterval;
var delay = Math.Max(serverDelay, pollingInterval);

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, it seems there's no overload for TimeSpan.

Copy link
Member

Choose a reason for hiding this comment

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

You can use TimeSpan.TotalMilliseconds.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there an optimization reason to prefer Math.Max over the ternary operator? This would imply some other changes in tests.


await WaitAsync(delay, cancellationToken).ConfigureAwait(false);
}
}

protected virtual async Task WaitAsync(TimeSpan delay, CancellationToken cancellationToken) =>
Copy link
Member

Choose a reason for hiding this comment

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

Why is this virtual and nothing else is? If anything, other methods like UpdateStatus/Async make more sense as virtual methods than simply waiting.

Copy link
Member Author

Choose a reason for hiding this comment

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

We're making it virtual for testing only. The current design does not expect developers to derive from OperationInternal, but instantiate it internally in their operations.

await Task.Delay(delay, cancellationToken).ConfigureAwait(false);

private async ValueTask<Response> UpdateStatusAsync(bool async, CancellationToken cancellationToken)
Copy link
Member Author

Choose a reason for hiding this comment

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

Flow of UpdateStatus in multiple clients goes as follows:

  • Check if already completed. If not, poll.
  • If operation succeeded, parse response.
  • If failed, throw (we could still throw with code 2xx depending on the content of the response).

Copy link
Member

Choose a reason for hiding this comment

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

Add as a comment?

Copy link
Member

Choose a reason for hiding this comment

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

In general, I'd like to see more comments on internal types/members anyway - especially those in shared source like in Core/Shared. What exceptions might we expect? What parameters are actually required? What is the behavior, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added (a lot of) docstrings to the methods we expect developers to use. We still intend to add a usage guide in our docs in another PR.

{
using DiagnosticScope scope = _diagnostics.CreateScope(_updateStatusScopeName);

foreach (KeyValuePair<string, string> attribute in ScopeAttributes)
kinelski marked this conversation as resolved.
Show resolved Hide resolved
{
scope.AddAttribute(attribute.Key, attribute.Value);
}

scope.Start();

try
{
return await UpdateStateAsync(async, cancellationToken).ConfigureAwait(false);
}
catch (Exception e)
{
scope.Failed(e);
throw;
}
}

private async ValueTask<Response> UpdateStateAsync(bool async, CancellationToken cancellationToken)
{
var state = await _operation.UpdateStateAsync(async, cancellationToken).ConfigureAwait(false);
kinelski marked this conversation as resolved.
Show resolved Hide resolved

RawResponse = state.RawResponse;

if (state.Succeeded == true)
kinelski marked this conversation as resolved.
Show resolved Hide resolved
{
Value = state.Value;
HasCompleted = true;
}
else if (state.Succeeded == false)
{
_operationFailedException = state.OperationFailedException ?? (async
? await _diagnostics.CreateRequestFailedExceptionAsync(state.RawResponse).ConfigureAwait(false)
: _diagnostics.CreateRequestFailedException(state.RawResponse));
HasCompleted = true;

throw _operationFailedException;
}

return state.RawResponse;
}

private static TimeSpan GetServerDelay(Response response)
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth sharing this implementation with the RetryPolicy corresponding method so our behavior stays in sync?

Copy link
Member Author

@kinelski kinelski May 14, 2021

Choose a reason for hiding this comment

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

RetryPolicy takes the number of attempts and the RetryMode into account as well, so I don't think it's worth the trouble.

Edit: took another look and GetServerDelay in fact is the same thing. I still think it's not worth the trouble, though, since it's a short method. I'll consult with Pavel and check if there's a convenient place to put this method in Core.

{
if (response.Headers.TryGetValue(RetryAfterMsHeaderName, out string retryAfterValue)
|| response.Headers.TryGetValue(XRetryAfterMsHeaderName, out retryAfterValue))
{
if (int.TryParse(retryAfterValue, out int serverDelayInMilliseconds))
{
return TimeSpan.FromMilliseconds(serverDelayInMilliseconds);
}
}

if (response.Headers.TryGetValue(RetryAfterHeaderName, out retryAfterValue))
{
if (int.TryParse(retryAfterValue, out int serverDelayInSeconds))
{
return TimeSpan.FromSeconds(serverDelayInSeconds);
}
}

return TimeSpan.Zero;
}
}

internal interface IOperation<TResult>
{
ValueTask<OperationState<TResult>> UpdateStateAsync(bool async, CancellationToken cancellationToken);
}

internal readonly struct OperationState<TResult>
{
private OperationState(Response rawResponse, bool? succeeded, TResult value, RequestFailedException operationFailedException)
{
RawResponse = rawResponse;
Succeeded = succeeded;
Value = value;
OperationFailedException = operationFailedException;
}

public Response RawResponse { get; }

public bool? Succeeded { get; }
Copy link
Member

Choose a reason for hiding this comment

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

Seeing this now (given my previous comment), if you don't actually care about the null state, you could handle that here in the helper e.g.

public bool Succeeded => _succeeded.HasValue && _succeeded.Value;

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to differentiate because we have three possible states:

  • If true: operation has completed successfully.
  • If false: operation has completed with errors.
  • If null: operation is still running.

Copy link
Member

Choose a reason for hiding this comment

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

In general, I get it. But I only remember seeing this property compared to true or false. Another property could do so as well and at least consolidate the logic I mentioned elsewhere about checking for HasValue first to avoid boxing (if the newer compiles aren't doing that automatically for Nullable<T> comparisons).

Copy link
Member

Choose a reason for hiding this comment

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

If true: operation has completed successfully.
If false: operation has completed with errors.
If null: operation is still running.

If this is what we are aiming for, I second Chris here that this should be documented, commented, etc so people don't have to figure out what each value means

Copy link
Member Author

Choose a reason for hiding this comment

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

But I only remember seeing this property compared to true or false.

if (state.Succeeded == true)
{
Value = state.Value;
HasCompleted = true;
}
else if (state.Succeeded == false)
{
_operationFailedException = state.OperationFailedException ?? (async
? await _diagnostics.CreateRequestFailedExceptionAsync(state.RawResponse).ConfigureAwait(false)
: _diagnostics.CreateRequestFailedException(state.RawResponse));
HasCompleted = true;
throw _operationFailedException;
}

If it's true, we change the state. If it's false, we change it to something else. If it's null, we keep the state we had previously. We can't achieve the same behavior if we have a non-nullable bool given that we have three possible outcomes.

I'll make Succeeded a non-nullable bool and add a new bool called HasCompleted so we can avoid the problems mentioned.

Copy link
Member Author

@kinelski kinelski May 11, 2021

Choose a reason for hiding this comment

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

If this is what we are aiming for, I second Chris here that this should be documented, commented, etc so people don't have to figure out what each value means

We'll have proper documentation and a migration guide, but developers won't need to understand these values directly. An OperationState needs to be instantiated by one of its static methods:

  • OperationState<T>.Pending(Response)
  • OperationState<T>.Success(Response, T)
  • OperationState<T>.Failure(Response, RequestFailedException)

Its properties, such as Succeeded, are set by these static methods and are used only by the OperationInternal class.


public TResult Value { get; }

public RequestFailedException OperationFailedException { get; }

public static OperationState<TResult> Success(Response rawResponse, TResult value) =>
new OperationState<TResult>(rawResponse, true, value, default);

public static OperationState<TResult> Failure(Response rawResponse, RequestFailedException operationFailedException = null) =>
new OperationState<TResult>(rawResponse, false, default, operationFailedException);

public static OperationState<TResult> Pending(Response rawResponse) =>
new OperationState<TResult>(rawResponse, default, default, default);
}
}
1 change: 1 addition & 0 deletions sdk/core/Azure.Core/tests/Azure.Core.Tests.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
<Compile Include="..\src\Shared\NoBodyResponseOfT.cs" LinkBase="Shared" />
<Compile Include="..\src\Shared\ObjectNotDisposedException.cs" LinkBase="Shared" />
<Compile Include="..\src\Shared\OperationHelpers.cs" LinkBase="Shared" />
<Compile Include="..\src\Shared\OperationInternal.cs" LinkBase="Shared" />
<Compile Include="..\src\Shared\PageResponseEnumerator.cs" LinkBase="Shared" />
<Compile Include="..\src\Shared\RetriableStream.cs" LinkBase="Shared" />
<Compile Include="..\src\Shared\RequestRequestContent.cs" LinkBase="Shared" />
Expand Down
Loading