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

Switch go-to-impl to be entirely async/non-blocking. #57861

Merged
merged 46 commits into from
Nov 22, 2021
Merged
Show file tree
Hide file tree
Changes from 45 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
3ba2fd7
Release the UI thread when performing GoToImpl command
CyrusNajmabadi Nov 18, 2021
60ebb32
Formatting
CyrusNajmabadi Nov 18, 2021
332dee4
About to undo
CyrusNajmabadi Nov 18, 2021
edf0c28
Fixes
CyrusNajmabadi Nov 18, 2021
cd9fa01
Entirely async goto-impl
CyrusNajmabadi Nov 18, 2021
4c11943
Extract file
CyrusNajmabadi Nov 18, 2021
0d0ae01
Fix integration harness
CyrusNajmabadi Nov 18, 2021
dab14d5
UI improvements
CyrusNajmabadi Nov 18, 2021
cf39c6e
Simplify
CyrusNajmabadi Nov 18, 2021
f4db61c
Let the user know if we're waiting on the solution to load
CyrusNajmabadi Nov 18, 2021
b9727d4
Update src/VisualStudio/Core/Def/Implementation/FindReferences/Contex…
CyrusNajmabadi Nov 18, 2021
9c71338
Add comments
CyrusNajmabadi Nov 18, 2021
8caed62
Add comments
CyrusNajmabadi Nov 18, 2021
32c03f1
Remove stale comment
CyrusNajmabadi Nov 18, 2021
4edd1f7
Wrap
CyrusNajmabadi Nov 18, 2021
269304f
Give the user a message if the solution is not fully loaded.
CyrusNajmabadi Nov 18, 2021
c869f77
Rvert
CyrusNajmabadi Nov 18, 2021
62da7bb
REvert
CyrusNajmabadi Nov 18, 2021
d333ed4
Add loc string
CyrusNajmabadi Nov 18, 2021
7f443d7
Pass information along even when popping up a message
CyrusNajmabadi Nov 18, 2021
59a79cf
Simplify
CyrusNajmabadi Nov 18, 2021
a76591b
Change indentation
CyrusNajmabadi Nov 18, 2021
7bc5faa
Remove partial
CyrusNajmabadi Nov 18, 2021
943d671
Rename file
CyrusNajmabadi Nov 18, 2021
2fc5071
Fix comment
CyrusNajmabadi Nov 18, 2021
abf9082
Increase version number
CyrusNajmabadi Nov 18, 2021
b268465
Notify out of lock
CyrusNajmabadi Nov 18, 2021
938bd79
Simplify
CyrusNajmabadi Nov 18, 2021
81851ed
Simplify
CyrusNajmabadi Nov 18, 2021
23a90d5
Flip
CyrusNajmabadi Nov 18, 2021
7f02e92
Update docs
CyrusNajmabadi Nov 18, 2021
62a22fe
Simplify
CyrusNajmabadi Nov 18, 2021
267b6dc
Spelling
CyrusNajmabadi Nov 18, 2021
3cffaef
Renames
CyrusNajmabadi Nov 18, 2021
c07f4f0
Update src/VisualStudio/Core/Def/Implementation/FindReferences/Contex…
CyrusNajmabadi Nov 18, 2021
7cff1e7
Add doc
CyrusNajmabadi Nov 18, 2021
ad49244
Add ocmment
CyrusNajmabadi Nov 18, 2021
cc70f1e
Merge branch 'gotoImplPumping' of https://github.com/CyrusNajmabadi/r…
CyrusNajmabadi Nov 18, 2021
fbc556a
Simplify
CyrusNajmabadi Nov 18, 2021
be9c09b
Add contract
CyrusNajmabadi Nov 18, 2021
4cae7d2
Simplify
CyrusNajmabadi Nov 18, 2021
aaec370
revert
CyrusNajmabadi Nov 18, 2021
df2586f
remove copy
CyrusNajmabadi Nov 18, 2021
703bb7e
Use simple using statement
CyrusNajmabadi Nov 20, 2021
a22525d
Use nrt and an internal state object
CyrusNajmabadi Nov 20, 2021
88fdda9
Update src/VisualStudio/Core/Def/Implementation/FindReferences/Stream…
CyrusNajmabadi Nov 21, 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

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,259 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.Editor.Host;
using Microsoft.CodeAnalysis.Editor.Shared.Extensions;
using Microsoft.CodeAnalysis.Editor.Shared.Tagging;
using Microsoft.CodeAnalysis.Editor.Shared.Utilities;
using Microsoft.CodeAnalysis.Editor.Tagging;
using Microsoft.CodeAnalysis.ErrorReporting;
using Microsoft.CodeAnalysis.FindUsages;
using Microsoft.CodeAnalysis.Host;
using Microsoft.CodeAnalysis.Internal.Log;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Microsoft.CodeAnalysis.Shared.TestHooks;
using Microsoft.CodeAnalysis.Text;
using Microsoft.VisualStudio.Commanding;
using Microsoft.VisualStudio.Text.Editor.Commanding;
using Microsoft.VisualStudio.Threading;
using Microsoft.VisualStudio.Utilities;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.Editor.CommandHandlers;

internal abstract class AbstractGoToCommandHandler<TLanguageService, TCommandArgs> : ICommandHandler<TCommandArgs>
where TLanguageService : class, ILanguageService
where TCommandArgs : EditorCommandArgs
{
private readonly IThreadingContext _threadingContext;
private readonly IStreamingFindUsagesPresenter _streamingPresenter;
private readonly IUIThreadOperationExecutor _uiThreadOperationExecutor;
private readonly IAsynchronousOperationListener _listener;

/// <summary>
/// The current go-to command that is in progress. Tracked so that if we issue multiple find-impl commands that
/// they properly run after each other. This is necessary so none of them accidentally stomp on one that is still
/// in progress and is interacting with the UI. Only valid to read or write to this on the UI thread.
/// </summary>
private Task _inProgressCommand = Task.CompletedTask;

/// <summary>
/// CancellationToken governing the current <see cref="_inProgressCommand"/>. Only valid to read or write to this
/// on the UI thread.
/// </summary>
/// <remarks>
/// Cancellation is complicated with this feature. There are two things that can cause us to cancel. The first is
/// if the user kicks off another actual go-to-impl command. In that case, we just attempt to cancel the prior
/// command (if it is still running), then wait for it to complete, then run our command. The second is if we have
/// switched over to the streaming presenter and then the user starts some other command (like FAR) that takes over
/// the presenter. In that case, the presenter will notify us that it has be re-purposed and we will also cancel
Copy link
Member

@dibarbet dibarbet Nov 19, 2021

Choose a reason for hiding this comment

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

once we've computed the items and put them in the presenter, why is there anything that needs to be cancelled here if another presenter takes over? That seems a bit strange - it seems like the other presenter should just take over, and the existing one is 'disposed' of

Copy link
Member Author

Choose a reason for hiding this comment

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

So, there's only one FAR window (unless you select 'keep results' in the window). So once another feature wants to push results into that window we want to stop what we're doing as there's no point anymore.

/// this source.
/// </remarks>
private CancellationTokenSource _cancellationTokenSource = new();

public AbstractGoToCommandHandler(
IThreadingContext threadingContext,
IStreamingFindUsagesPresenter streamingPresenter,
IUIThreadOperationExecutor uiThreadOperationExecutor,
IAsynchronousOperationListener listener)
{
_threadingContext = threadingContext;
_streamingPresenter = streamingPresenter;
_uiThreadOperationExecutor = uiThreadOperationExecutor;
_listener = listener;
}

public abstract string DisplayName { get; }
protected abstract string ScopeDescription { get; }
protected abstract FunctionId FunctionId { get; }

protected abstract Task FindActionAsync(Document document, int caretPosition, IFindUsagesContext context, CancellationToken cancellationToken);

public CommandState GetCommandState(TCommandArgs args)
{
var document = args.SubjectBuffer.AsTextContainer().GetOpenDocumentInCurrentContext();
return document?.GetLanguageService<TLanguageService>() is null
? CommandState.Unspecified
: CommandState.Available;
}

public bool ExecuteCommand(TCommandArgs args, CommandExecutionContext context)
{
// Should only be called on the UI thread.
Contract.ThrowIfFalse(_threadingContext.HasMainThread);

var subjectBuffer = args.SubjectBuffer;
var caret = args.TextView.GetCaretPoint(subjectBuffer);
if (!caret.HasValue)
return false;

var document = subjectBuffer.AsTextContainer().GetOpenDocumentInCurrentContext();
if (document == null)
return false;

var service = document.GetLanguageService<TLanguageService>();
if (service == null)
return false;

// cancel any prior find-refs that might be in progress.
_cancellationTokenSource.Cancel();
_cancellationTokenSource = new();

// we're going to return immediately from ExecuteCommand and kick off our own async work to invoke the
// operation. Once this returns, the editor will close the threaded wait dialog it created.
_inProgressCommand = ExecuteCommandAsync(document, caret.Value.Position, _cancellationTokenSource);
return true;
}

private async Task ExecuteCommandAsync(
Document document,
int position,
CancellationTokenSource cancellationTokenSource)
{
// This is a fire-and-forget method (nothing guarantees observing it). As such, we have to handle cancellation
// and failure ourselves.
try
{
// Should only be called on the UI thread.
Contract.ThrowIfFalse(_threadingContext.HasMainThread);

// Make an tracking token so that integration tests can wait until we're complete.
using var token = _listener.BeginAsyncOperation($"{this.GetType().Name}.{nameof(ExecuteCommandAsync)}");

// Only start running once the previous command has finished. That way we don't have results from both
// potentially interleaving with each other. Note: this should ideally always be fast as long as the prior
// task respects cancellation.
//
// Note: we just need to make sure we run after that prior command finishes. We do not want to propagate
// any failures from it. Technically this should not be possible as it should be inside this same
// try/catch. however this code wants to be very resilient to any prior mistakes infecting later operations.
await _inProgressCommand.NoThrowAwaitable(captureContext: false);
await this.ExecuteCommandWorkerAsync(document, position, cancellationTokenSource).ConfigureAwait(false);
}
catch (OperationCanceledException)
{
}
catch (Exception ex) when (FatalError.ReportAndCatch(ex))
{
}
}

private async Task ExecuteCommandWorkerAsync(
Document document,
int position,
CancellationTokenSource cancellationTokenSource)
{
// Switch to the BG immediately so we can keep as much work off the UI thread.
await TaskScheduler.Default;

// We kick off the work to find the impl/base in the background. If we get the results for it within 1.5
// seconds, we then either navigate directly to it (in the case of one result), or we show all the results in
// the presenter (in the case of multiple).
//
// However, if the results don't come back in 1.5 seconds, we just pop open the presenter and continue the
// search there. That way the user is not blocked and can go do other work if they want.

// We create our own context object, simply to capture all the definitions reported by the individual
// TLanguageService. Once we get the results back we'll then decide what to do with them. If we get only a
// single result back, then we'll just go directly to it. Otherwise, we'll present the results in the
// IStreamingFindUsagesPresenter.
var findContext = new BufferedFindUsagesContext();

var cancellationToken = cancellationTokenSource.Token;
var delayTask = Task.Delay(TaggerDelay.OnIdle.ComputeTimeDelay(), cancellationToken);
Copy link
Member

Choose a reason for hiding this comment

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

hmm I think I'd prefer to not use the tagger delay since this doesnt really have anything to do with tagging. I don't think we'd want this changed if we changed the idle tagger delay for tagging reasons

Copy link
Member Author

Choose a reason for hiding this comment

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

so, 'TaggerDelay' is poorly named. It should really be called something like 'PerceptionConstants'. These are values provided form research from teh platform/UX teams about how people perceive delays. i.e. what a timespan needs to be to be considered with the 'instantaneous' threshold for people, etc.

This timespan (which we use in many places outside of tagging) is the: enough time has passed for me to start realizing it's going to take a while to get results, so transition the UI to let hte user know.

var findTask = Task.Run(() => FindResultsAsync(document, position, findContext, cancellationToken), cancellationToken);
Copy link
Contributor

Choose a reason for hiding this comment

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

For my curiosity: Is the Task.Run needed here, if we're already on a background thread due to the TaskScheduler.Default?

Copy link
Member Author

Choose a reason for hiding this comment

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

It likely isn't. i can remove in a followup :)


var firstFinishedTask = await Task.WhenAny(delayTask, findTask).ConfigureAwait(false);
if (cancellationToken.IsCancellationRequested)
{
// we bailed out because another command was issued. Immediately stop everything we're doing and return
// back so the next operation can run.
return;
}

if (firstFinishedTask == findTask)
{
// We completed the search within 1.5 seconds. If we had at least one result then Navigate to it directly
// (if there is just one) or present them all if there are many.
var definitions = await findContext.GetDefinitionsAsync(cancellationToken).ConfigureAwait(false);
if (definitions.Length > 0)
{
var title = await findContext.GetSearchTitleAsync(cancellationToken).ConfigureAwait(false);
await _streamingPresenter.TryNavigateToOrPresentItemsAsync(
_threadingContext,
document.Project.Solution.Workspace,
title ?? this.DisplayName,
definitions,
cancellationToken).ConfigureAwait(false);
return;
}
}

// We either got no results, or 1.5 has passed and we didn't figure out the symbols to navigate to or
// present. So pop up the presenter to show the user that we're involved in a longer search, without
// blocking them.
await PresentResultsInStreamingPresenterAsync(findContext, findTask, cancellationTokenSource).ConfigureAwait(false);
}

private async Task PresentResultsInStreamingPresenterAsync(
BufferedFindUsagesContext findContext,
Task findTask,
CancellationTokenSource cancellationTokenSource)
{
var cancellationToken = cancellationTokenSource.Token;
await _threadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken);
var (presenterContext, presenterCancellationToken) = _streamingPresenter.StartSearch(this.DisplayName, supportsReferences: false);
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this could be made cleaner by allowing the IStreamingFindReferencesPresenter to take in an optional find usages context that it uses. Then it seems like you wouldn't need all the complexity in BufferedFindUsagesContext to switch between the presenters

Copy link
Member Author

Choose a reason for hiding this comment

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

It wouldn't work. The FindContext is what features push into. The window itself (which is the context actually), needs to pull the already computed values.


try
{
await TaskScheduler.Default;

// Now, tell our find-context (which has been collecting intermediary results) to swap over to using the
// actual presenter context. It will push all results it's been collecting into that, and from that
// point onwards will just forward any new results directly to the presenter.
await findContext.AttachToStreamingPresenterAsync(presenterContext, cancellationToken).ConfigureAwait(false);

// Hook up the presenter's cancellation token to our overall governing cancellation token. In other
// words, if something else decides to present in the presenter (like a find-refs call) we'll hear about
// that and can cancel all our work.
presenterCancellationToken.Register(() => cancellationTokenSource.Cancel());

// now actually wait for the find work to be done.
await findTask.ConfigureAwait(false);
}
finally
{
// Ensure that once we pop up the presenter, we always make sure to force it to the completed stage in
// case some other find operation happens (either through this handler or another handler using the
// presenter) and we don't actually finish the search.
await presenterContext.OnCompletedAsync(cancellationToken).ConfigureAwait(false);
}
}

private async Task FindResultsAsync(
Document document, int position, IFindUsagesContext findContext, CancellationToken cancellationToken)
{
using (Logger.LogBlock(FunctionId, KeyValueLogMessage.Create(LogType.UserAction), cancellationToken))
{
await findContext.SetSearchTitleAsync(this.DisplayName, cancellationToken).ConfigureAwait(false);

// Let the user know in the FAR window if results may be inaccurate because this is running prior to the
// solution being fully loaded.
var service = document.Project.Solution.Workspace.Services.GetRequiredService<IWorkspaceStatusService>();
var isFullyLoaded = await service.IsFullyLoadedAsync(cancellationToken).ConfigureAwait(false);
if (!isFullyLoaded)
{
await findContext.ReportInformationalMessageAsync(
EditorFeaturesResources.The_results_may_be_incomplete_due_to_the_solution_still_loading_projects, cancellationToken).ConfigureAwait(false);
}

// We were able to find the doc prior to loading the workspace (or else we would not have the service).
// So we better be able to find it afterwards.
await FindActionAsync(document, position, findContext, cancellationToken).ConfigureAwait(false);
}
}
}
Loading