Synchronize OperationInternalBase calls #27639

AlexanderSher · 2022-03-18T16:51:51Z

This is a draft of the change to resolve several issues:

When two WaitForCompletion calls are made concurrently, RawResponse property may contain response that is not from the final state. For details, see: [Core] Improve OperationHelpers used by our LROs #19105 (comment)
Because of the problem above, right now we have a workaround that every call for WaitForCompletion makes at least one call even if operation is completed, including cases when operation was created as completed
OperationInternal<T>.Value may contain value that can't be recreated from RawResponse
OperationInternal<T>.HasValue may be true while RawResponse contains "pending" response

AlexanderSher · 2022-03-18T16:53:49Z

sdk/core/Azure.Core/src/Shared/AsyncLockWithValue.cs

+    /// Primitive that combines async lock and value cache
+    /// </summary>
+    /// <typeparam name="T"></typeparam>
+    internal sealed class AsyncLockWithValue<T>


This type is a copy of type from Azure.Identity with added HasValue property and TryGetValue method.

Assuming we make this change, we should change Identity's reference to this shared source version and delete its copy?

We can do that.

AlexanderSher · 2022-03-18T16:54:48Z

sdk/core/Azure.Core/src/Shared/OperationInternalBase.cs

            RawResponse = rawResponse;
-            _fallbackStrategy = fallbackStrategy;
+
+            if (finalState != null)


To avoid overriding of one final state with another, final state is assigned in constructor instead of separate method.

AlexanderSher · 2022-03-18T16:57:58Z

sdk/core/Azure.Core/src/Shared/OperationInternalBase.cs

+
+        private async ValueTask<bool> TrySetPendingResponseAsync(bool async, Response response, CancellationToken cancellationToken)
+        {
+            using var asyncLock = await _operationStateLock.GetLockOrValueAsync(async, cancellationToken).ConfigureAwait(false);


This method allows update of the RawResponse up until operation is completed

Could you add that as a comment? Would help with future maintenance.

AlexanderSher · 2022-03-18T16:59:07Z

sdk/core/Azure.Core/src/Shared/OperationInternalBase.cs

        private async ValueTask<Response> UpdateStatusAsync(bool async, CancellationToken cancellationToken)
        {
+            using var asyncLock = await _operationStateLock.GetLockOrValueAsync(async, cancellationToken).ConfigureAwait(false);


Here is where synchronization of calls happens.

Same here. Would help with future maintenance.

AlexanderSher · 2022-03-18T17:01:43Z

sdk/core/Azure.Core/src/Shared/OperationInternalBase.cs

+                if (!state.HasSucceeded && state.OperationFailedException == null)
+                {
+                    var exception = async
+                        ? await _diagnostics.CreateRequestFailedExceptionAsync(state.RawResponse).ConfigureAwait(false)


@annelo-msft when custom error parsing introduced, we will have to update this place as well

AlexanderSher · 2022-03-18T17:02:19Z

sdk/core/Azure.Core/tests/AsyncLockWithValueTests.cs

+
+namespace Azure.Core.Tests
+{
+    public class AsyncLockWithValueTests


Tests also copied from Azure.Identity

AlexanderSher · 2022-03-18T17:05:06Z

sdk/confidentialledger/Azure.Security.ConfidentialLedger/src/PostLedgerEntryOperation.cs

@@ -61,15 +61,13 @@ async ValueTask<OperationState> IOperation.UpdateStateAsync(bool async, Cancella
                    .ConfigureAwait(false)
                : _client.GetTransactionStatus(Id, new RequestContext { CancellationToken = cancellationToken, ErrorOptions = ErrorOptions.NoThrow });

-            _operationInternal.RawResponse = statusResponse;


This is not needed, _operationInternal updates its RawResponse

AlexanderSher · 2022-03-18T17:09:16Z

I'll add more tests for this if design is approved.

heaths

General and KV changes LGTM, though I had a few suggestions and question.

heaths · 2022-03-18T17:40:11Z

sdk/core/Azure.Core/src/Shared/AsyncLockWithValue.cs

+                }
+            }
+
+            public void Dispose() => _owner?.Reset();


Might be worth a comment here clarifying what this is intended to do. I believe the goal here is just to release any queued lock - not necessarily this one being disposed - and cycle through until we get another lock that hasn't completed, effectively like a semaphore, right?

sdk/core/Azure.Core/src/Shared/OperationInternalBase.cs

heaths · 2022-03-18T17:44:19Z

sdk/core/Azure.Core/src/Shared/OperationInternalBase.cs

+
+        private async ValueTask<bool> TrySetPendingResponseAsync(bool async, Response response, CancellationToken cancellationToken)
+        {
+            using var asyncLock = await _operationStateLock.GetLockOrValueAsync(async, cancellationToken).ConfigureAwait(false);


Could you add that as a comment? Would help with future maintenance.

heaths · 2022-03-18T17:44:40Z

sdk/core/Azure.Core/src/Shared/OperationInternalBase.cs

        private async ValueTask<Response> UpdateStatusAsync(bool async, CancellationToken cancellationToken)
        {
+            using var asyncLock = await _operationStateLock.GetLockOrValueAsync(async, cancellationToken).ConfigureAwait(false);


Same here. Would help with future maintenance.

heaths · 2022-03-18T17:51:42Z

sdk/keyvault/Azure.Security.KeyVault.Certificates/src/DeleteCertificateOperation.cs

@@ -24,18 +24,15 @@ internal DeleteCertificateOperation(KeyVaultPipeline pipeline, Response<DeletedC
        {
            _pipeline = pipeline;
            _value = response.Value ?? throw new InvalidOperationException("The response does not contain a value.");
+            // The recoveryId is only returned if soft delete is enabled.


Nit: can you add a blank line between 26 and 27. IIRC, this may cause some code style errors.

m-nash · 2022-03-18T18:14:57Z

sdk/core/Azure.Core/src/Shared/OperationInternalOfT.cs

-            DelayStrategy? fallbackStrategy = null)
-            : base(clientDiagnostics, rawResponse, operationTypeName ?? operation.GetType().Name, scopeAttributes, fallbackStrategy)
+            DelayStrategy? fallbackStrategy = null,
+            OperationState<T>? finalState = null)


Given we are taking this in on the constructor now I think we should move this class into its own file along with the non T version

https://github.com/Azure/azure-sdk-for-net/pull/27639/files#diff-f4e6fe06a0be62f7078fe921b22173ed7ad094e129c90206626734d527eb31f8R286

m-nash · 2022-03-18T18:27:19Z

sdk/core/Azure.Core/src/Shared/OperationInternalBase.cs

        }

-        protected abstract ValueTask<Response> UpdateStateAsync(bool async, CancellationToken cancellationToken);
+        protected abstract ValueTask<OperationState> UpdateStateAsync(bool async, CancellationToken cancellationToken);


It feels weird that we have a State class representing the state of the current operation yet that state is never stored in the instance of the class it simply creates one, then assigns some member variables. Should we maybe store this as a member variable and have the HasCompleted / HasValue / Value all lambda into this object to get their answers?

It is stored: https://github.com/Azure/azure-sdk-for-net/pull/27639/files#diff-f9f7a9d046f21930f94a8ca1d25df78506aabb692ce00a3504bdce32498b4ef3R254

m-nash · 2022-03-18T18:30:12Z

sdk/core/Azure.Core/src/Shared/OperationInternal.cs

-            DelayStrategy? fallbackStrategy = null)
-            :base(clientDiagnostics, rawResponse, operationTypeName ?? operation.GetType().Name, scopeAttributes, fallbackStrategy)
+            DelayStrategy? fallbackStrategy = null,
+            OperationState? finalState = null)


Do we worry about the conflict between the response in this final state and the response as the parameter? Should we take one or the other?

@heaths , is it possible that these responses are different in keyvault?

For the operations you changed, they are all pseudo-LROs anyway. The service backed endpoints aren't actually LROs, but we poll the final resource URI until it's not 404. Given the response and the final state should be agreed in that case, I don't think it should be possible.

m-nash · 2022-03-18T18:36:17Z

sdk/core/Azure.Core/src/Shared/OperationInternalOfT.cs

-        }
-
-        protected override async ValueTask<Response> UpdateStateAsync(bool async, CancellationToken cancellationToken)
+        protected override async ValueTask<OperationState> UpdateStateAsync(bool async, CancellationToken cancellationToken)
        {
            OperationState<T> state = await _operation.UpdateStateAsync(async, cancellationToken).ConfigureAwait(false);


This method on IOperation.UpdateStateAsync feels like it should be named GetStateAsync since it shouldn't be updating anything only this instance method should?

m-nash · 2022-03-18T18:40:29Z

sdk/core/Azure.Core/src/Shared/OperationInternalBase.cs


-            HasCompleted = true;


I would like to understand more why this race condition has to be fixed this way. I don't like that we are forcing an extra call even when the operation is completed (unless there is some nuance in your statement there).

I read the OP comment of To mitigate this problem we are adding an extra call, but it was referring to the previous bullet and how it was previously fixed. My understanding is now that the extra call is being removed.

I still would like to understand if there is a simpler way to resolve the race condition and allow the occasional thread to need an extra call

Depends on what do you mean by "simpler". If we make an extra call, then we need to throw away its response if operation is in a final state. Otherwise, Response.FromValue(Value, rawResponse) may use Value from one request and rawResponse from another.
There are two options how to achieve it: using locks/semaphores or using state object that can be read/replaced in atomic operation. Since we have OperationInternal and OperationInternal<T>, there will be at least two objects (one derived from another). I can implement this version in a separate branch so we can compare.

AlexanderSher · 2022-04-25T21:39:06Z

Closing in favor of #28375

Synchronize OperationInternalBase calls

06fd7ba

ghost added Azure.Core Cognitive - Text Analytics KeyVault labels Mar 18, 2022

AlexanderSher commented Mar 18, 2022

View reviewed changes

Revert unused change

bf1165b

AlexanderSher commented Mar 18, 2022

View reviewed changes

heaths approved these changes Mar 18, 2022

View reviewed changes

m-nash requested changes Mar 18, 2022

View reviewed changes

AlexanderSher mentioned this pull request Apr 25, 2022

Synchronize OperationInternal calls #28375

Merged

AlexanderSher closed this Apr 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Synchronize OperationInternalBase calls #27639

Synchronize OperationInternalBase calls #27639

AlexanderSher commented Mar 18, 2022 •

edited

Loading

AlexanderSher Mar 18, 2022

christothes Mar 21, 2022 •

edited

Loading

AlexanderSher Mar 22, 2022

AlexanderSher Mar 18, 2022

AlexanderSher Mar 18, 2022

heaths Mar 18, 2022

AlexanderSher Mar 18, 2022

heaths Mar 18, 2022

AlexanderSher Mar 18, 2022

AlexanderSher Mar 18, 2022

AlexanderSher Mar 18, 2022

AlexanderSher commented Mar 18, 2022

heaths left a comment

heaths Mar 18, 2022

heaths Mar 18, 2022

heaths Mar 18, 2022

heaths Mar 18, 2022

m-nash Mar 18, 2022

m-nash Mar 18, 2022

AlexanderSher Mar 18, 2022

m-nash Mar 18, 2022

AlexanderSher Mar 18, 2022

heaths Mar 18, 2022

m-nash Mar 18, 2022

m-nash Mar 18, 2022

m-nash Mar 18, 2022

AlexanderSher Mar 18, 2022

AlexanderSher commented Apr 25, 2022

Synchronize OperationInternalBase calls #27639

Synchronize OperationInternalBase calls #27639

Conversation

AlexanderSher commented Mar 18, 2022 • edited Loading

Choose a reason for hiding this comment

christothes Mar 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexanderSher commented Mar 18, 2022

heaths left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexanderSher commented Apr 25, 2022

AlexanderSher commented Mar 18, 2022 •

edited

Loading

christothes Mar 21, 2022 •

edited

Loading