Avoid cancellation exceptions in async queue implementation #68412

sharwell · 2023-06-01T15:31:29Z

Addresses 59+% of the exceptions (220K out of 375K total) observed in CABs associated with AB#1827592.

CyrusNajmabadi · 2023-06-01T15:47:58Z

Can we instead wait for @jcouv 's work to be done. This code is pretty confusing to understand.

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/TestHooks/ValueTaskExtensions.cs

sharwell · 2023-06-01T15:52:32Z

Can we instead wait for @jcouv 's work to be done. This code is pretty confusing to understand.

That work will make 780403b unnecessary, but it will not address c933b80. We also don't have an ETA for this and it seems questionable as a short term target.

sharwell · 2023-06-01T15:56:28Z

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource_ProduceTags.cs

                // If any of the requests was high priority, then compute at that speed.
                var highPriority = changes.Contains(true);
-                await RecomputeTagsAsync(highPriority, cancellationToken).ConfigureAwait(false);


📝 This await resulted in 47916 exceptions in the trace I'm reviewing. Following this change, there are no exceptions thrown from this location.

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource_ProduceTags.cs

CyrusNajmabadi · 2023-06-01T17:33:33Z

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource_ProduceTags.cs

+                        // tagging operation. Otherwise, it is impossible to know which changes occurred prior to the
+                        // request to tag, and which ones occurred during the tagging itself.
+                        this.AccumulatedTextChanges = null;
+                    }


i'm not getting the logic here.

This change is a separate commit. It might help to review it that way.

The underlying problem is we claimed data from the type and reset that data at the point of acquisition. If cancellation occurs after acquisition but before processing completes, the accumulated text changes for that batch processing operation were simply discarded previously. The new code will re-process the accumulated text changes if cancellation interrupts processing.

➡️ I retained this change as it is a correctness fix

src/Workspaces/Core/Portable/Shared/Utilities/AsyncBatchingWorkQueue`1.cs

CyrusNajmabadi · 2023-06-01T17:36:02Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/TestHooks/ValueTaskExtensions.cs

+            }
+        }
+    }
+}


very worried about the difficulty validating that logic like this is correct. I would prefer we just wait on the compiler doing this work for us so we can get the battle testing of that.

I'd be happy to have @stephentoub review this. I would consider it a candidate for inclusion in vs-threading along with the NoThrowAwaitable method they already have there for Task<TResult>.

Personally, I'm not a fan of changing the await to return a tuple of the status, exception, and result and would hope we wouldn't promote that. We're adding support for no-throw awaiting in .NET 8 and, after many go-arounds on design and discussing doing this exact thing and various variations, are very explicitly only doing so for Task and not ValueTask. If you really need to do this with ValueTask, you have various options, like

vt = vt.Preserve(); await vt.NoThrow(); // your simple awaiter that just nops GetResult ... examine vt

and it doesn't require inventing a complicated thing or using an unfamiliar pattern.

⏱️ Works for me. I'll rewrite to this pattern.

➡️ This has now been rewritten.

➡️ This code will be rewritten to use microsoft/vs-threading#1193 after 17.7 Preview 3 ships.

was this done?

➡️ No, I checked CI this morning and the images are still on 17.7 Preview 2.

CyrusNajmabadi

currently blocking. but open to discussion on this. but it should be combined with the convo on compiler async/await emit.

stephentoub · 2023-06-02T01:52:16Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/TestHooks/ValueTaskExtensions.cs

+            /// </summary>
+            public (TaskStatus Status, ExceptionDispatchInfo? Exception, TResult? Result) GetResult()
+            {
+                var preserved = _task.Preserve();


If this VT is backed by an IValueTaskSource and it's faulted or canceled, this is going to result in throwing and catching its exception. My understanding is that's the cost this whole thing is trying to prevent.

This is also on very shaky ground. It may work most of the time, but again with an IVTS, this is calling Preserve after it's already hooked up a continuation to the IVTS. That's not something that's technically supported, and could possibly fail with some implementations. I think it should work with ours. But, shaky ground.

➡️ This has been rewritten to call Preserve() at the point the awaitable is initialized.

My understanding is that's the cost this whole thing is trying to prevent.

This isn't the specific situation impacting us, so it's acceptable that there is no clear resolution for it.

sharwell · 2023-06-02T13:21:43Z

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource_ProduceTags.cs

-                // no point preceding if we're already disposed.  We check this on the UI thread so that we will know
-                // about any prior disposal on the UI thread.
-                if (cancellationToken.IsCancellationRequested)
-                    return;


📝 I retained this change as it served no purpose (SwitchToMainThreadAsync checks the cancellation token immediately before this).

sharwell · 2023-06-02T13:22:54Z

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource_ProduceTags.cs

+                    if (cancellationToken.IsCancellationRequested)
+                        return;


📝 I retained these changes as they represent a trivial consistency update to this method, and do not overlap with the planned compiler changes.

sharwell · 2023-06-02T13:23:24Z

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource_ProduceTags.cs

+                        // tagging operation. Otherwise, it is impossible to know which changes occurred prior to the
+                        // request to tag, and which ones occurred during the tagging itself.
+                        this.AccumulatedTextChanges = null;
+                    }


➡️ I retained this change as it is a correctness fix

Changes which overlap the compiler work have been removed from this pull request

sharwell · 2023-06-02T13:25:47Z

src/Workspaces/Core/Portable/Shared/Utilities/AsyncBatchingWorkQueue`2.cs

+                var batchResultTask = _processBatchAsync(nextBatch, batchCancellationToken).Preserve();
+                await batchResultTask.NoThrowAwaitableInternal(false);


📝 This change is retained because it does not overlap with compiler work. The no-throw logic in async state machines does not apply to await statements within a try block that contains one or more catch clauses, so avoiding an exception here requires updating to a no-throw form of awaiter.

CyrusNajmabadi · 2023-06-12T17:32:00Z

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource_ProduceTags.cs

@@ -170,32 +170,31 @@ private void OnEventSourceChanged(object? _1, TaggerEventArgs _2)
            private void EnqueueWork(bool highPriority)
                => _eventChangeQueue.AddWork(highPriority, _dataSource.CancelOnNewWork);

-            private async ValueTask ProcessEventChangeAsync(ImmutableSegmentedList<bool> changes, CancellationToken cancellationToken)
+            private ValueTask<VoidResult> ProcessEventChangeAsync(ImmutableSegmentedList<bool> changes, CancellationToken cancellationToken)


what's the urpse of hte 'Voidresult' in all of this?

➡️ Required as part of the move to AsyncBatchingWorkQueue<bool, VoidResult>. The helper type AsyncBatchingWorkQueue<bool> is not suitable for this scenario due to performance impact in an edge case. This restriction is documented and localized to private implementation code.

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource.cs

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource_ProduceTags.cs

CyrusNajmabadi · 2023-06-12T17:36:06Z

src/Workspaces/Core/Portable/Shared/Utilities/AsyncBatchingWorkQueue`2.cs

+                else
+                {
+                    // Access the result to force the exception to be thrown.
+                    _ = batchResultTask.Result;


does this throw cancellation in the event of cancellatino? or does it throw an aggregate exception? should this be GetAwaiter().GetResult() instead?

➡️ This is a ValueTask, so there is no need to use GetAwaiter().GetResult() (exceptions are not wrapped in AggregateException)

can you comment this. needing to know the subtle differences between the task systems is non trivial mental overhead for validating this stuff.

➡️ Now uses helper method VerifyCompleted

CyrusNajmabadi · 2023-06-12T17:36:59Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/TestHooks/ValueTaskExtensions.cs

+            public NoThrowValueTaskAwaitable(ValueTask task, bool captureContext)
+            {
+                Contract.ThrowIfNull(task, nameof(task));
+                _task = task.Preserve();


➡️ Rewrote this to use a direct copy of the code from vs-threading (only changes were renames to avoid code style warnings):
https://github.com/microsoft/vs-threading/blob/main/src/Microsoft.VisualStudio.Threading/TplExtensions.cs

CyrusNajmabadi · 2023-06-12T17:37:04Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/TestHooks/ValueTaskExtensions.cs

+            public NoThrowValueTaskAwaitable(ValueTask<TResult> task, bool captureContext)
+            {
+                Contract.ThrowIfNull(task, nameof(task));
+                _task = task.Preserve();


➡️ Rewrote this to use a direct copy of the code from vs-threading (only changes were renames to avoid code style warnings):
https://github.com/microsoft/vs-threading/blob/main/src/Microsoft.VisualStudio.Threading/TplExtensions.cs

CyrusNajmabadi · 2023-07-19T15:27:52Z

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource_ProduceTags.cs

+                        // tagging operation. Otherwise, it is impossible to know which changes occurred prior to the
+                        // request to tag, and which ones occurred during the tagging itself.
+                        this.AccumulatedTextChanges = null;
+                    }


ok. this change seems very safe to me. That said, i would prefer the comment on it (or some documentation somewhere) be expanded a bit. namely, stating that it's ok to not clear this out, as the only impact is slightly worse performance for some taggers by indicating potentially more of the document was changed than actually changed.

Basically, i'd like docs somewhere that this is more a conservative estimate of what changed, and it is ok to be more (but not less) than what actually did change. Thanks!

➡️ Documentation updated

CyrusNajmabadi · 2023-07-19T15:30:04Z

src/Workspaces/Core/Portable/Shared/Utilities/AsyncBatchingWorkQueue`2.cs

-                // Don't bubble up cancellation to the queue for the nested batch cancellation.  Just because we decided
-                // to cancel this batch isn't something that should stop processing further batches.
-                return default;
+                var batchResultTask = _processBatchAsync(nextBatch, batchCancellationToken).Preserve();


ok. just to check. we need to all .Preserve because not only are we awaiting this value-task, but we're also doing things like directly checking .Result (and other members) off of it. And ValueTasks otherwise do not support this after an await because they may be put back in a pool?

➡️ Correct

CyrusNajmabadi · 2023-07-19T15:33:03Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/ValueTaskExtensions.cs

-            return task.GetAwaiter().GetResult();
+            // Propagate any exceptions that may have been thrown. Relies on ValueTask.Result behaving like
+            // Task.GetAwaiter().GetResult() in the presence of exceptions.
+            return task.Result;


i don't get it. why was the old code wrong?

➡️ The old code had the same behavior for the case where the ValueTask<T> is already completed, but @stephentoub has suggested on some other cases where I used .GetAwaiter().GetResult() that it was unnecessary and we can just use .Result for ValueTask<T>.

➡️ I removed this change from the pull request.

Avoids an intermediate async state machine that throws a frequent cancellation exception.

CyrusNajmabadi · 2023-07-19T16:17:42Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/TestHooks/ValueTaskExtensions.cs

+internal static class ValueTaskExtensions
+{
+    // Following code is copied from Microsoft.VisualStudio.Threading.TplExtensions (renamed to avoid ambiguity)
+    // https://github.com/microsoft/vs-threading/blob/main/src/Microsoft.VisualStudio.Threading/TplExtensions.cs


is this file up to date with the code in vs-threading? (i just want to make sure if we have a copy, we're reasonably close to it).

➡️ Yes, the only changes are renamed private fields to have _ prefix, and removal of this. qualification.

* Avoid throwing cancellation exceptions (follow-up to dotnet#68412) * Use provided NoThrowAwaitable extension method

sharwell requested a review from a team as a code owner June 1, 2023 15:31

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Jun 1, 2023

ToddGrun reviewed Jun 1, 2023

View reviewed changes

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/TestHooks/ValueTaskExtensions.cs Outdated Show resolved Hide resolved

sharwell commented Jun 1, 2023

View reviewed changes

ToddGrun reviewed Jun 1, 2023

View reviewed changes

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource_ProduceTags.cs Show resolved Hide resolved

CyrusNajmabadi reviewed Jun 1, 2023

View reviewed changes

src/Workspaces/Core/Portable/Shared/Utilities/AsyncBatchingWorkQueue`1.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jun 1, 2023

View reviewed changes

CyrusNajmabadi previously requested changes Jun 1, 2023

View reviewed changes

stephentoub reviewed Jun 2, 2023

View reviewed changes

sharwell force-pushed the no-throw branch from 54b6476 to caff4d7 Compare June 2, 2023 13:13

sharwell commented Jun 2, 2023

View reviewed changes

sharwell force-pushed the no-throw branch from 3bae8a1 to d32f31a Compare June 2, 2023 15:59

CyrusNajmabadi reviewed Jun 12, 2023

View reviewed changes

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource.cs Show resolved Hide resolved

CyrusNajmabadi reviewed Jun 12, 2023

View reviewed changes

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource_ProduceTags.cs Show resolved Hide resolved

CyrusNajmabadi reviewed Jun 12, 2023

View reviewed changes

sharwell force-pushed the no-throw branch from d32f31a to 635a876 Compare June 12, 2023 22:02

CyrusNajmabadi reviewed Jul 19, 2023

View reviewed changes

sharwell added 2 commits July 19, 2023 11:15

Rewrite async state machine to avoid throwing exceptions

bfbb4b5

Avoid some cancellation exceptions in tagging

449e9c0

sharwell added 2 commits July 19, 2023 11:15

Avoid marking text as processed when processing was cancelled

97c0dd5

Explicitly declare VoidResult for event queue

84d13b1

Avoids an intermediate async state machine that throws a frequent cancellation exception.

sharwell force-pushed the no-throw branch from 635a876 to 84d13b1 Compare July 19, 2023 16:15

CyrusNajmabadi reviewed Jul 19, 2023

View reviewed changes

sharwell enabled auto-merge July 19, 2023 16:26

CyrusNajmabadi approved these changes Jul 19, 2023

View reviewed changes

sharwell merged commit d1f6077 into dotnet:main Jul 19, 2023
24 checks passed

allisonchou mentioned this pull request Jul 21, 2023

[Automated] PRs inserted in VS build main-33921.318 #69157

Closed

sharwell deleted the no-throw branch August 1, 2023 16:01

sharwell added this to the 17.8 P1 milestone Aug 2, 2023

sharwell added a commit to sharwell/roslyn that referenced this pull request Sep 13, 2023

Use vs-threading extensions where available

3cf1090

* Avoid throwing cancellation exceptions (follow-up to dotnet#68412) * Use provided NoThrowAwaitable extension method

sharwell mentioned this pull request Sep 13, 2023

Use vs-threading extensions where available #69932

Merged

		var batchResultTask = _processBatchAsync(nextBatch, batchCancellationToken).Preserve();
		await batchResultTask.NoThrowAwaitableInternal(false);

Avoid cancellation exceptions in async queue implementation #68412

Avoid cancellation exceptions in async queue implementation #68412

Conversation

sharwell commented Jun 1, 2023 • edited Loading

CyrusNajmabadi commented Jun 1, 2023

sharwell commented Jun 1, 2023 • 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

sharwell Jun 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi 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

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

sharwell commented Jun 1, 2023 •

edited

Loading

sharwell commented Jun 1, 2023 •

edited

Loading

sharwell Jun 9, 2023 •

edited

Loading

sharwell Jul 19, 2023 •

edited

Loading