Let F# create a workspace project context without blocking the main thread - #85219
Conversation
`FSharpWorkspaceProjectContextFactory.CreateProjectContext` wraps the async `IWorkspaceProjectContextFactory.CreateProjectContextAsync` in `JoinableTaskFactory.Run`, which waits through the main thread. A caller that runs while the main thread is already blocked therefore deadlocks, and F# navigation has such a caller: Peek reaches the F# language service through `INavigableItemsService` from inside `PeekableItemSource`, which is holding the main thread in its own `JoinableTaskFactory.Run` for the duration. Peeking at a symbol whose definition lives in metadata — which is every symbol once you are already looking at a metadata-as-source window — hangs Visual Studio. Add an async overload, which is the shape the underlying factory already has, and let the synchronous one be the wrapper over it rather than over the factory directly. Nothing about the existing signature changes. Roslyn's own metadata Peek has no such problem: `DefinitionPeekableItem` does its work in `IPeekResultSource.FindResults`, which the broker calls on a background thread, and it hands Peek a file path rather than opening a document. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Azure Pipelines: Successfully started running 2 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
dotnet-policy-service
Bot
added
the
Community
There was a problem hiding this comment.
🟡 Changes recommended
The new ExternalAccess API members need corresponding updates to the InternalAPI.Unshipped tracking files to keep the repo’s API surface accounting consistent (and to avoid API-checking failures).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds an asynchronous API for creating an F# workspace project context in Roslyn’s Visual Studio ExternalAccess layer, allowing callers (notably Peek-related flows) to avoid blocking the main thread while the underlying project context is created.
Changes:
- Add
IFSharpWorkspaceProjectContextFactory.CreateProjectContextAsync(...)to support non-blocking project context creation with cancellation. - Refactor the existing synchronous
CreateProjectContext(...)path to wrap the new async implementation (preserving the existing sync signature/behavior). - Thread a
CancellationTokenthrough toIWorkspaceProjectContextFactory.CreateProjectContextAsync(...).
File summaries
| File | Description |
|---|---|
| src/VisualStudio/ExternalAccess/Core/FSharp/VS/IFSharpWorkspaceProjectContextFactory.cs | Adds an async overload to the F# project context factory and routes sync creation through the async implementation. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
| /// <summary> | ||
| /// Creates the project context without blocking. <see cref="CreateProjectContext(string, string)"/> waits for | ||
| /// this through the main thread, so a caller that runs while the main thread is blocked — inside the Peek | ||
| /// broker's <c>JoinableTaskFactory.Run</c>, say — deadlocks on it. | ||
| /// </summary> | ||
| Task<IFSharpWorkspaceProjectContext> CreateProjectContextAsync(string filePath, string uniqueName, CancellationToken cancellationToken); |
`CreateProjectContext` is tracked in both external access baselines, so the async overloads belong there too. The type forwards already cover the factory and its interface: this adds members, not types. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Addressed in the latest commit: the two Same caveat as on #85213: nothing here consumes the |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes a VS threading/deadlock-sensitive path (JTF + project context creation), which warrants a human reviewer validating the intended joinable-task behavior under Peek.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/VisualStudio/ExternalAccess/Core/FSharp/VS/IFSharpWorkspaceProjectContextFactory.cs:27
- The XML doc comment here says
CreateProjectContext(string, string)"waits for this through the main thread". In the current implementation, the sync overload blocks viaJoinableTaskFactory.Run, but it doesn’t literally wait for this specific async method; and the deadlock description is really about the sync/blocking pattern. Rewording the comment would make the contract clearer and avoid misleading future callers/maintainers.
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
Peek reaches the F# language service through `INavigableItemsService` while its broker holds the main thread in `JoinableTaskFactory.Run`, and that wait does not pump messages. Producing a definition that only exists as generated metadata needs the main thread — to create the workspace project context and to open the document — so asking for it from there deadlocks Visual Studio, not just the editor. It presents as whatever the user happened to be doing, and is reliably reproduced by opening Peek from inside the metadata window a first Peek produced, where every symbol is external. Split the search rather than dropping the metadata case: Go To Definition keeps it, since it owns the wait it makes, and Peek gets the variant that stops at definitions which already have a document. Peek into metadata therefore shows nothing for now. Roslyn's own Peek avoids both waits by generating the file in `IPeekResultSource.FindResults`, which the broker calls on a background thread, and handing it a path instead of opening a document. Matching that needs the project context to be creatable off the main thread, which is dotnet/roslyn#85219. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Peeking at an F# symbol whose definition lives in metadata hangs Visual Studio. It is reliably
reproducible by opening Peek Definition twice: the second one, invoked from inside the metadata
window the first one produced, deadlocks the main thread.
PeekableItemSource.AugmentPeekSessionruns on the main thread and holds it inJoinableTaskFactory.Runfor the whole augmentation. F# reaches it throughFSharpNavigableItemsService, theINavigableItemsServicefor F#, so everything the F# languageservice does for Peek runs while the main thread is blocked. Producing a metadata-as-source
definition needs a workspace project context, and
FSharpWorkspaceProjectContextFactory.CreateProjectContextwraps the asyncIWorkspaceProjectContextFactory.CreateProjectContextAsyncin a secondJoinableTaskFactory.Run,which waits through the main thread that is already blocked.
This adds an async overload to
IFSharpWorkspaceProjectContextFactory, the shape the underlyingfactory already has, and makes the synchronous method a wrapper over it rather than over the factory
directly. The existing signature and its behaviour are unchanged, and the interface is implemented in
this repository, so nothing outside it has to move first.
That is one of the two waits on that path; the other is in dotnet/fsharp, which switches to the main
thread to open the generated document. Roslyn's own metadata Peek does neither:
DefinitionPeekableItemgenerates the file in
IPeekResultSource.FindResults, which the broker calls on a background thread("we must block the thread since the API doesn't support proper asynchrony"), and hands Peek a file
path through
PeekHelpers.CreateDocumentPeekResultrather than opening a document. The F# side isbeing moved to the same shape, and needs this overload to do it.
🤖 Generated with Claude Code
Microsoft Reviewers: Open in CodeFlow