Make the IDocumentNavigationSerivice entirely async. #59784

CyrusNajmabadi · 2022-02-25T22:36:00Z

Extracting out from #59759.

This is step one in making navigation saner. Next step is to make the service non-imperative itself. e.g. it can be queried to determine navigatino locations independent of actually navigating to those locations.

In many cases, this asynchrony just pushed up till it reached something already async (yaay).

In other cases this async pushed up until we got to a synchronous location due to an API we implement, but that had an ITHreadingContext. In those cases we can jsut JTF run the async code then.

In the final cases this pushed up to a synchronous location without an IThreadingContext. In those cases, an IThreadingContext was imported and passed in to support this.

CyrusNajmabadi · 2022-02-25T22:41:47Z

src/EditorFeatures/Core.Wpf/Peek/PeekableItemSource.cs

@@ -58,63 +64,65 @@ public void AugmentPeekSession(IPeekSession session, IList<IPeekableItem> peekab

            _uiThreadOperationExecutor.Execute(EditorFeaturesResources.Peek, EditorFeaturesResources.Loading_Peek_information, allowCancellation: true, showProgress: false, action: context =>


review with whitespace off.

CyrusNajmabadi · 2022-02-25T22:43:46Z

src/EditorFeatures/Core/Shared/Utilities/WorkspaceThreadingServiceProvider.cs

+            Service = service;
+        }
+
+        public IWorkspaceThreadingService Service { get; }


@jasonmalinowski i needed this for teh DocumentNavigationOperation change below. That's a public API we can't pass an IThreadingContext into. INstead, it just gets a WOrkspace, so we need a way to get this from that.

Yeah, this seems reasonable in the sense that I don't have a good alternative here.

CyrusNajmabadi · 2022-02-25T22:44:12Z

src/Features/Core/Portable/Common/NavigationOperation.cs

@@ -34,7 +35,10 @@ public override void Apply(Workspace workspace, CancellationToken cancellationTo
            if (workspace.CanOpenDocuments)
            {
                var navigationService = workspace.Services.GetService<IDocumentNavigationService>();
-                navigationService.TryNavigateToPosition(workspace, _documentId, _position, cancellationToken);
+                var threadingService = workspace.Services.GetService<IWorkspaceThreadingServiceProvider>();


this is a new concept @jasonmalinowski . not sure if there's any better way here.

Yeah, I don't have many better options other than leave the sync method on the IDocumentNavigationService and then inline this, which of course kinda undoes what you're trying to do. So unless @sharwell has a better idea, I'm OK with this in the "I will pinch my nose and sign off because I don't have a better idea".

CyrusNajmabadi · 2022-02-25T22:45:16Z

src/Workspaces/Core/Portable/Shared/Utilities/IWorkspaceThreadingService.cs

+    internal interface IWorkspaceThreadingServiceProvider : IWorkspaceService
+    {
+        IWorkspaceThreadingService Service { get; }
+    }


moral equivalent of IWorkspaceThreadingService @jasonmalinowski . However the above one is only available through MEF, where i need it when i only have a workspace available.

src/Tools/ExternalAccess/FSharp/Navigation/FSharpDocumentNavigationService.cs

…ationService.cs

jasonmalinowski

Some comments but generally seems fine. I don't like the hack you had to do that you flagged but I don't have a better idea.

src/Features/Core/Portable/Navigation/IDocumentNavigationService.cs

jasonmalinowski · 2022-02-25T22:45:10Z

src/Features/Core/Portable/Navigation/IDocumentNavigationService.cs

@@ -27,18 +21,13 @@ internal interface IDocumentNavigationService : IWorkspaceService
        /// Determines whether it is possible to navigate to the given line/offset in the specified document.
        /// </summary>
        /// <remarks>Only legal to call on the UI thread.</remarks>
-        bool CanNavigateToLineAndOffset(Workspace workspace, DocumentId documentId, int lineNumber, int offset, CancellationToken cancellationToken);
+        Task<bool> CanNavigateToLineAndOffsetAsync(Workspace workspace, DocumentId documentId, int lineNumber, int offset, CancellationToken cancellationToken);


It's a bit fishy to me we would have added the async methods but left the sync ones too -- was there some reason that still applies? Is this being used via IVT or something?

src/Tools/ExternalAccess/FSharp/Navigation/IFSharpDocumentNavigationService.cs

jasonmalinowski · 2022-02-25T23:05:00Z

src/Features/Core/Portable/Common/NavigationOperation.cs

@@ -34,7 +35,10 @@ public override void Apply(Workspace workspace, CancellationToken cancellationTo
            if (workspace.CanOpenDocuments)
            {
                var navigationService = workspace.Services.GetService<IDocumentNavigationService>();
-                navigationService.TryNavigateToPosition(workspace, _documentId, _position, cancellationToken);
+                var threadingService = workspace.Services.GetService<IWorkspaceThreadingServiceProvider>();


Yeah, I don't have many better options other than leave the sync method on the IDocumentNavigationService and then inline this, which of course kinda undoes what you're trying to do. So unless @sharwell has a better idea, I'm OK with this in the "I will pinch my nose and sign off because I don't have a better idea".

jasonmalinowski · 2022-02-25T23:33:29Z

src/EditorFeatures/TestUtilities2/Utilities/MockDocumentNavigationServiceProvider.vb

                Me.ProvidedDocumentId = documentId
                Me.ProvidedLineNumber = lineNumber

-                Return CanNavigateToLineAndOffsetReturnValue
+                Return If(CanNavigateToLineAndOffsetReturnValue, SpecializedTasks.True, SpecializedTasks.False)


We don't have a helper for this?

couldn't find one :)

...ore/Portable/ExternalAccess/VSTypeScript/Api/VSTypeScriptDocumentNavigationServiceWrapper.cs

src/EditorFeatures/Core/Implementation/InlineRename/InlineRenameService.cs

jasonmalinowski · 2022-02-25T23:42:08Z

src/EditorFeatures/Core/Implementation/CodeActions/CodeActionEditHandlerService.cs

@@ -184,7 +184,8 @@ internal class CodeActionEditHandlerService : ForegroundThreadAffinitizedObject,
            }

            var updatedSolution = operations.OfType<ApplyChangesOperation>().FirstOrDefault()?.ChangedSolution ?? oldSolution;
-            TryNavigateToLocationOrStartRenameSession(workspace, oldSolution, updatedSolution, cancellationToken);


Wait, so if applied is false we do the navigation anyways?

jasonmalinowski · 2022-02-25T23:57:45Z

src/VisualStudio/Core/Def/Implementation/Workspace/VisualStudioDocumentNavigationService.cs

            // Mappings for opened razor files are retrieved via the LSP client making a request to the razor server.
            // If we wait for the result on the UI thread, we will hit a bug in the LSP client that brings us to a code path
            // using ConfigureAwait(true).  This deadlocks as it then attempts to return to the UI thread which is already blocked by us.
            // Instead, we invoke this in JTF run which will mitigate deadlocks when the ConfigureAwait(true)
            // tries to switch back to the main thread in the LSP client.
            // Link to LSP client bug for ConfigureAwait(true) - https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1216657


Is this comment applicable anymore?

oh that's terrifying.

this should no longer apply as we're either all async, or our callers need to be in JTF run calls themselves (this PR).

…oslyn into asyncNavigation

Make the IDocumentNavigationSerivice entirely async.

c065bed

CyrusNajmabadi requested a review from jasonmalinowski February 25, 2022 22:36

CyrusNajmabadi requested review from a team as code owners February 25, 2022 22:36

dotnet-issue-labeler bot added the Area-IDE label Feb 25, 2022

Update tests

a2cdcb1

CyrusNajmabadi commented Feb 25, 2022

View reviewed changes

CyrusNajmabadi added 2 commits February 25, 2022 15:43

Simplify threading

00e538b

Remove unnecessary code

1782bae

CyrusNajmabadi commented Feb 25, 2022

View reviewed changes

src/Tools/ExternalAccess/FSharp/Navigation/FSharpDocumentNavigationService.cs Outdated Show resolved Hide resolved

Update src/Tools/ExternalAccess/FSharp/Navigation/FSharpDocumentNavig…

5819767

…ationService.cs

jasonmalinowski approved these changes Feb 25, 2022

View reviewed changes

CyrusNajmabadi added 5 commits February 25, 2022 16:06

Remove stale remarks

5f6f8e8

Merge branch 'asyncNavigation' of https://github.com/CyrusNajmabadi/r…

589abae

…oslyn into asyncNavigation

Simplify

cc7a4b6

Push async up

f3fcff4

REmove comment

487d63a

CyrusNajmabadi enabled auto-merge February 26, 2022 00:20

CyrusNajmabadi added 3 commits February 26, 2022 12:40

Merge remote-tracking branch 'upstream/main' into asyncNavigation

ecfc0a5

lint

355fa65

Threading

bedc049

CyrusNajmabadi merged commit 7b723b7 into dotnet:main Feb 27, 2022

ghost added this to the Next milestone Feb 27, 2022

RikkiGibson modified the milestones: Next, 17.2.P2 Mar 1, 2022

CyrusNajmabadi deleted the asyncNavigation branch March 2, 2022 22:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the IDocumentNavigationSerivice entirely async. #59784

Make the IDocumentNavigationSerivice entirely async. #59784

CyrusNajmabadi commented Feb 25, 2022 •

edited

Loading

CyrusNajmabadi Feb 25, 2022

CyrusNajmabadi Feb 25, 2022

jasonmalinowski Feb 25, 2022

CyrusNajmabadi Feb 25, 2022

jasonmalinowski Feb 25, 2022

CyrusNajmabadi Feb 25, 2022

jasonmalinowski left a comment

jasonmalinowski Feb 25, 2022

jasonmalinowski Feb 25, 2022

jasonmalinowski Feb 25, 2022

CyrusNajmabadi Feb 26, 2022

jasonmalinowski Feb 25, 2022

jasonmalinowski Feb 25, 2022

CyrusNajmabadi Feb 26, 2022

CyrusNajmabadi Feb 26, 2022

		@@ -58,63 +64,65 @@ public void AugmentPeekSession(IPeekSession session, IList<IPeekableItem> peekab

		_uiThreadOperationExecutor.Execute(EditorFeaturesResources.Peek, EditorFeaturesResources.Loading_Peek_information, allowCancellation: true, showProgress: false, action: context =>

Make the IDocumentNavigationSerivice entirely async. #59784

Make the IDocumentNavigationSerivice entirely async. #59784

Conversation

CyrusNajmabadi commented Feb 25, 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

jasonmalinowski 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

CyrusNajmabadi commented Feb 25, 2022 •

edited

Loading