Make the IDocumentNavigationSerivice entirely async. by CyrusNajmabadi · Pull Request #59784 · dotnet/roslyn

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

            {
                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

        /// </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

            {
                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.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

            }

            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

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

ghost 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

		@@ -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 =>

Conversation

CyrusNajmabadi commented Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

CyrusNajmabadi commented Feb 25, 2022 •

edited

Loading