Fix issue where Solution-Explorer symbol nodes collapse after an edit. #66354
Conversation
| @@ -836,11 +837,11 @@ public Graph Graph | |||
| } | |||
| } | |||
|
|
|||
| public IEnumerable<GraphNode> GetCreatedNodes(CancellationToken cancellationToken) | |||
| public ImmutableArray<GraphNode> GetCreatedNodes(CancellationToken cancellationToken) | |||
There was a problem hiding this comment.
changed a bunch of code to be IMmutableArray to make it clear that lazy computation is not happening (especially scary with all the locks here).
| var asyncToken = _asyncListener.BeginAsyncOperation(nameof(BeginGetGraphData)); | ||
| _ = _graphQueryManager | ||
| .AddQueriesAsync(context, graphQueries) | ||
| .CompletesAsyncOperation(asyncToken); |
There was a problem hiding this comment.
context.OnCompleted(); is always called within AddQueriesAsync, so there was no need to special case things on this side of the call.
|
|
||
| /// <summary> | ||
| /// This gate locks manipulation of <see cref="_trackedQueries"/>. | ||
| /// </summary> | ||
| private readonly object _gate = new(); | ||
| private readonly List<ValueTuple<WeakReference<IGraphContext>, List<IGraphQuery>>> _trackedQueries = new(); | ||
| private ImmutableArray<(WeakReference<IGraphContext> context, ImmutableArray<IGraphQuery> queries)> _trackedQueries = ImmutableArray<(WeakReference<IGraphContext>, ImmutableArray<IGraphQuery>)>.Empty; |
There was a problem hiding this comment.
moved away from mutable collections to immutable ones.
|
|
||
| // We update all of our tracked queries when this delay elapses. | ||
| private ResettableDelay? _delay; | ||
| private readonly AsyncBatchingWorkQueue _updateQueue; |
There was a problem hiding this comment.
moved to a simple update-queue model that updates all live-queries after any workspace chnages.
There was a problem hiding this comment.
I don't think we had this type at the time this was written, so this is a nice improvement.
| } | ||
|
|
||
| private void EnqueueUpdateIfSolutionIsStale(Solution solution) | ||
| public async Task AddQueriesAsync(IGraphContext context, ImmutableArray<IGraphQuery> graphQueries) | ||
| { |
There was a problem hiding this comment.
made this function async to make it cleaner to work with tasks with async/await.
|
|
||
| // We want to ensure that no matter what happens, this initial context is completed | ||
| var task = populateTask.SafeContinueWith( | ||
| _ => context.OnCompleted(), context.CancelToken, TaskContinuationOptions.ExecuteSynchronously, TaskScheduler.Default); |
There was a problem hiding this comment.
moved away from SafeContinueWith now that this is asyncified.
| lock (_gate) | ||
| { | ||
| _trackedQueries = _trackedQueries.Add((new WeakReference<IGraphContext>(context), graphQueries)); | ||
| } |
There was a problem hiding this comment.
inlined helpers to the single places they are used.
| } | ||
| var target = t.context.GetTarget(); | ||
| return target is null || target.CancelToken.IsCancellationRequested; | ||
| }); | ||
| } |
There was a problem hiding this comment.
performing both the grab of things to update, and the remoal of stale queries under a single atomic transaction.
|
|
||
| graphBuilder.ApplyToGraph(context.Graph, cancellationToken); | ||
| context.OutputNodes.AddAll(graphBuilder.GetCreatedNodes(cancellationToken)); | ||
|
|
||
| transaction2.Complete(); | ||
| transaction.Complete(); | ||
| } |
There was a problem hiding this comment.
the meat of the cahnge is in this method. Instad of clearing things out in a transaciton, then adding the items in a transaction, we do the first clear/add as a single transaction, and any subsequent adds in new transactions. That first clear/add being done atomically is what allows solution-explorer to properly track old results to new results as they update.
The multi-add case is only for solution-explorer-search (where we do a normal search, then the SG-file search). And, in that case, there's nothing that solution-explorer needs to track, so it's fine for the adds to happen in multiple goes.
There was a problem hiding this comment.
This generally looks great, but I think it needs a try/catch in PopulateContextGraphAsync, or otherwise the graph queries being cancelled will result in us throwing cancelled exceptions where callers didn't pass those tokens in, and surprises might happen. I think the cancelled tasks will go unobserved, but it's hard to say.
Technically, the queue is resilient to this (because we don't want mistakes causing the queue to go belly up). But it is still poor hygiene. So i've fixed it to not be a bad citizen and to doc the semantics we want here. |
ghost
added this to the 
Fixes AB#1708110
Fixes AB#1548520
THe core issue here is that solution-explorer cannot track nodes (and open/closed state) unless the entire content change happens in a single transaction. This is how we used to update things until a perf change was made a while back for solution-explorer-search. To speed up that search, we perform the search in two passes. The first pass searches normal documents. THe second pass generates SG documents and then searches those. Because we needed the first pass to show up immediately (and not at the end), we broke things into two transactions, which broke solution-explorer-tracking.
The fix here is to go back to only having a single transaction in the case where we're just doing a normal solution-explorer symbol population. For solution-explorer-search though, we continue having multiple transactions.
There was also a lot of complexity and old patterns in this code. It's been extensively cleaned up and moved to more modern patterns (like AsyncBatchingWorkQueue) to help the code out.