Merge pull request #41475 from jasonmalinowski/change-graphmodel-error-handling

Change our error handling for GraphQueryManager

src/VisualStudio/Core/Def/Implementation/Progression/GraphQueries/SearchGraphQuery.cs

@@ -13,6 +13,7 @@
 using Microsoft.CodeAnalysis.Shared.Extensions;
 using Microsoft.VisualStudio.GraphModel;
 using Roslyn.Utilities;
+using System.Runtime.ExceptionServices;
 
 namespace Microsoft.VisualStudio.LanguageServices.Implementation.Progression
 {
@@ -109,8 +110,28 @@ private async Task<GraphNode> AddLinkedNodeForMemberAsync(Project project, ISymb
         internal async Task<ImmutableArray<ISymbol>> FindNavigableSourceSymbolsAsync(
             Project project, CancellationToken cancellationToken)
         {
-            var declarations = await DeclarationFinder.FindSourceDeclarationsWithPatternAsync(
-                project, _searchPattern, SymbolFilter.TypeAndMember, cancellationToken).ConfigureAwait(false);
+            ImmutableArray<SymbolAndProjectId> declarations;
+
+            // FindSourceDeclarationsWithPatternAsync calls into OOP to do the search; if something goes badly it
+            // throws a SoftCrashException which inherits from OperationCanceledException. This is unfortunate, because
+            // it means that other bits of code see this as a cancellation and then may crash because they expect that if this
+            // method is raising cancellation, it's because cancellationToken requested the cancellation. The intent behind
+            // SoftCrashException was since it inherited from OperationCancelled it would make things safer, but in this case
+            // it's violating other invariants in the process which creates other problems.
+            //
+            // https://github.com/dotnet/roslyn/issues/40476 tracks removing SoftCrashException. When it is removed, the
+            // catch here can be removed and simply let the exception propagate; our Progression code is hardened to
+            // handle exceptions and report them gracefully.
+            try
+            {
+                declarations = await DeclarationFinder.FindSourceDeclarationsWithPatternAsync(
+                    project, _searchPattern, SymbolFilter.TypeAndMember, cancellationToken).ConfigureAwait(false);
+            }
+            catch (SoftCrashException ex) when (ex.InnerException != null)
+            {
+                ExceptionDispatchInfo.Capture(ex.InnerException).Throw();
+                throw ExceptionUtilities.Unreachable;
+            }
 
             var symbols = declarations.SelectAsArray(d => d.Symbol);
 

src/VisualStudio/Core/Def/Implementation/Progression/GraphQueryManager.cs

@@ -2,6 +2,8 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
+#nullable enable
+
 using System;
 using System.Collections.Generic;
 using System.Linq;
@@ -9,6 +11,7 @@
 using System.Threading.Tasks;
 using Microsoft.CodeAnalysis;
 using Microsoft.CodeAnalysis.Editor.Shared.Utilities;
+using Microsoft.CodeAnalysis.ErrorReporting;
 using Microsoft.CodeAnalysis.Shared.TestHooks;
 using Microsoft.VisualStudio.GraphModel;
 using Roslyn.Utilities;
@@ -23,13 +26,13 @@ internal class GraphQueryManager
         private readonly IAsynchronousOperationListener _asyncListener;
 
         /// <summary>
-        /// This gate locks manipulation of trackedQueries.
+        /// This gate locks manipulation of <see cref="_trackedQueries"/>.
         /// </summary>
         private readonly object _gate = new object();
         private readonly List<ValueTuple<WeakReference<IGraphContext>, List<IGraphQuery>>> _trackedQueries = new List<ValueTuple<WeakReference<IGraphContext>, List<IGraphQuery>>>();
 
         // We update all of our tracked queries when this delay elapses.
-        private ResettableDelay _delay;
+        private ResettableDelay? _delay;
 
         internal GraphQueryManager(Workspace workspace, IAsynchronousOperationListener asyncListener)
         {
@@ -116,7 +119,7 @@ private Task UpdateAsync()
             List<ValueTuple<IGraphContext, List<IGraphQuery>>> liveQueries;
             lock (_gate)
             {
-                liveQueries = _trackedQueries.Select(t => ValueTuple.Create(t.Item1.GetTarget(), t.Item2)).Where(t => t.Item1 != null).ToList();
+                liveQueries = _trackedQueries.Select(t => ValueTuple.Create(t.Item1.GetTarget(), t.Item2)).Where(t => t.Item1 != null).ToList()!;
             }
 
             var solution = _workspace.CurrentSolution;
@@ -152,33 +155,35 @@ private static bool IsTrackingContext(WeakReference<IGraphContext> weakContext)
         /// <summary>
         /// Populate the graph of the context with the values for the given Solution.
         /// </summary>
-        private static Task PopulateContextGraphAsync(Solution solution, List<IGraphQuery> graphQueries, IGraphContext context)
-        {
-            var cancellationToken = context.CancelToken;
-            var graphTasks = graphQueries.Select(q => q.GetGraphAsync(solution, context, cancellationToken)).ToArray();
-            var whenAllTask = Task.WhenAll(graphTasks);
-            return whenAllTask.SafeContinueWith(
-                t => ProcessGraphTasks(t.Result, context),
-                cancellationToken, TaskContinuationOptions.None, TaskScheduler.Default);
-        }
-
-        private static void ProcessGraphTasks(GraphBuilder[] graphBuilders, IGraphContext context)
+        private static async Task PopulateContextGraphAsync(Solution solution, List<IGraphQuery> graphQueries, IGraphContext context)
         {
-            // Perform the actual graph transaction 
-            using var transaction = new GraphTransactionScope();
-            // Remove any links that may have been added by a previous population. We don't
-            // remove nodes to maintain node identity, matching the behavior of the old
-            // providers.
-            context.Graph.Links.Clear();
-
-            foreach (var graphBuilder in graphBuilders)
+            try
             {
-                graphBuilder.ApplyToGraph(context.Graph);
+                var cancellationToken = context.CancelToken;
+                var graphBuilderTasks = graphQueries.Select(q => q.GetGraphAsync(solution, context, cancellationToken)).ToArray();
+                var graphBuilders = await Task.WhenAll(graphBuilderTasks).ConfigureAwait(false);
 
-                context.OutputNodes.AddAll(graphBuilder.CreatedNodes);
-            }
+                // Perform the actual graph transaction 
+                using var transaction = new GraphTransactionScope();
+
+                // Remove any links that may have been added by a previous population. We don't
+                // remove nodes to maintain node identity, matching the behavior of the old
+                // providers.
+                context.Graph.Links.Clear();
 
-            transaction.Complete();
+                foreach (var graphBuilder in graphBuilders)
+                {
+                    graphBuilder.ApplyToGraph(context.Graph);
+
+                    context.OutputNodes.AddAll(graphBuilder.CreatedNodes);
+                }
+
+                transaction.Complete();
+            }
+            catch (Exception ex) when (FatalError.ReportWithoutCrashUnlessCanceledAndPropagate(ex))
+            {
+                throw ExceptionUtilities.Unreachable;
+            }
         }
     }
 }