Move C# and Html document generation (ie, compilation) to the Cohost side of the world #9766

davidwengier · 2023-12-27T04:56:19Z

In theory this means the end of document sync issues forever!

In practice there is one fire-and-forget async method call here, from a sync context, so edge cases will still creep in. But all of this code should be deleted eventually anyway :D

This still leaves in place the text sync endpoints, and updates to the LSP servers project system, so that everything else functions as expected

This also nicely lets us Find All Refs on the Cohost feature flag and see all of the code that can be deleted :)

src/Razor/src/Microsoft.VisualStudio.LanguageServerClient.Razor/Cohost/OpenDocumentGenerator.cs

alexgav · 2023-12-27T18:36:25Z

src/Razor/src/Microsoft.VisualStudio.LanguageServerClient.Razor/Cohost/OpenDocumentGenerator.cs

+                    // The server clearly wants to tell us about a document in a project, but we don't know which project it's in.
+                    // Sadly there isn't anything we can do here to, we're just in a state where the server and client are out of
+                    // sync with their understanding of the document contents, and since changes come in as a list of changes,
+                    // the user experience is broken. All we can do is hope the user closes and re-opens the document.


All we can do is hope the user closes and re-opens the document.

Should we show a shell message box to the user asking them to do that?

src/Razor/src/Microsoft.VisualStudio.LanguageServerClient.Razor/Cohost/OpenDocumentGenerator.cs

alexgav · 2023-12-27T19:12:05Z

src/Razor/src/Microsoft.VisualStudio.LanguageServerClient.Razor/Cohost/DidChangeHandler.cs

-        return _projectSnapshotManagerDispatcher.RunOnDispatcherThreadAsync(
-            () => _razorProjectService.UpdateDocument(uri.GetAbsoluteOrUNCPath(), sourceText, version),
-            cancellationToken);
+        await await _projectSnapshotManagerDispatcher.RunOnDispatcherThreadAsync(async () =>


_projectSnapshotManagerDispatcher.RunOnDispatcherThreadAsync

just for my education - curious why projectSnapshotManager needs it's own dispatcher and when should we run on that dispatcher's thread. We can chat in sync-up if it's a long answer :)

This is a long, and hopefully eventually removed, answer :)

The questions in this PR are all excellent and unfortunately the answer to most of them is "yes, this is not ideal, but hopefully we can remove it eventually" 😁

src/Razor/src/Microsoft.VisualStudio.LanguageServerClient.Razor/Cohost/DidChangeHandler.cs

alexgav

src/Razor/src/Microsoft.VisualStudio.LanguageServerClient.Razor/Cohost/OpenDocumentGenerator.cs

ryzngard · 2023-12-27T20:49:34Z

src/Razor/src/Microsoft.VisualStudio.LanguageServerClient.Razor/Cohost/DidChangeHandler.cs

-        return _projectSnapshotManagerDispatcher.RunOnDispatcherThreadAsync(
-            () => _razorProjectService.UpdateDocument(uri.GetAbsoluteOrUNCPath(), sourceText, version),
-            cancellationToken);
+        await await _projectSnapshotManagerDispatcher.RunOnDispatcherThreadAsync(async () =>


This is a long, and hopefully eventually removed, answer :)

src/Razor/src/Microsoft.VisualStudio.LanguageServerClient.Razor/Cohost/OpenDocumentGenerator.cs

ryzngard · 2023-12-27T21:05:12Z

src/Razor/src/Microsoft.VisualStudio.LanguageServerClient.Razor/Cohost/OpenDocumentGenerator.cs

+                    // sync with their understanding of the document contents, and since changes come in as a list of changes,
+                    // the user experience is broken. All we can do is hope the user closes and re-opens the document.
+                    Debug.Fail($"Server wants to update {documentSnapshot.Uri} in {projectKey} but we don't know about the document being in any projects");
+                    _logger.LogError("[Cohost] Server wants to update {hostDocumentUri} in {projectKeyId} by we only know about that document in misc files. Server and client are now out of sync.", documentSnapshot.Uri, projectKey);


Might as well include a "closing and opening the document may fix this issue" if a user happens to look at the logs.

ryzngard · 2023-12-27T21:05:39Z

src/Razor/src/Microsoft.VisualStudio.LanguageServerClient.Razor/Cohost/OpenDocumentGenerator.cs

+                    // sync with their understanding of the document contents, and since changes come in as a list of changes,
+                    // the user experience is broken. All we can do is hope the user closes and re-opens the document.
+                    Debug.Fail($"Server wants to update {documentSnapshot.Uri} in {projectKey} but we don't know about the document being in any projects");
+                    _logger.LogError("[Cohost] Server wants to update {hostDocumentUri} in {projectKeyId} by we only know about that document in misc files. Server and client are now out of sync.", documentSnapshot.Uri, projectKey);


Should we fault on states like this? Is there really nothing we can do, or is it we don't know what causes this yet?

src/Razor/src/Microsoft.VisualStudio.LanguageServerClient.Razor/Cohost/OpenDocumentGenerator.cs

davidwengier · 2023-12-28T04:44:30Z

Thanks for the feedback.. Have address most of it, and I've adjusted some of the edge cases to be a bit more aggressive about failing, but I left the user experience unchanged in a release build, because I hope this code is not around any more once we start rolling the feature flag out to users.

Part of #9519 Now that the initial cohost PR has been merged, we don't need to use the feature branch any more. That means this PR, which is big, but all of the code in it has been reviewed, in #9761, #9766 and #9767.

davidwengier added 4 commits December 27, 2023 15:00

Do code generation as soon as a document opens or changes in cohosting

18bc37c

Don't publish document changes from the LSP server when cohosting is on

4398c6a

This still leaves in place the text sync endpoints, and updates to the LSP servers project system, so that everything else functions as expected

Prevent accidental regressions

34f01cd

This also nicely lets us Find All Refs on the Cohost feature flag and see all of the code that can be deleted :)

Regenerate open documents when the project changes

78ad08f

davidwengier requested a review from a team as a code owner December 27, 2023 04:56

alexgav reviewed Dec 27, 2023

View reviewed changes

src/Razor/src/Microsoft.VisualStudio.LanguageServerClient.Razor/Cohost/OpenDocumentGenerator.cs Outdated Show resolved Hide resolved

alexgav reviewed Dec 27, 2023

View reviewed changes

src/Razor/src/Microsoft.VisualStudio.LanguageServerClient.Razor/Cohost/OpenDocumentGenerator.cs Show resolved Hide resolved

alexgav reviewed Dec 27, 2023

View reviewed changes

src/Razor/src/Microsoft.VisualStudio.LanguageServerClient.Razor/Cohost/OpenDocumentGenerator.cs Outdated Show resolved Hide resolved

alexgav reviewed Dec 27, 2023

View reviewed changes

src/Razor/src/Microsoft.VisualStudio.LanguageServerClient.Razor/Cohost/OpenDocumentGenerator.cs Outdated Show resolved Hide resolved

alexgav reviewed Dec 27, 2023

View reviewed changes

src/Razor/src/Microsoft.VisualStudio.LanguageServerClient.Razor/Cohost/DidChangeHandler.cs Outdated Show resolved Hide resolved

alexgav approved these changes Dec 27, 2023

View reviewed changes

alexgav reviewed Dec 27, 2023

View reviewed changes

src/Razor/src/Microsoft.VisualStudio.LanguageServerClient.Razor/Cohost/OpenDocumentGenerator.cs Outdated Show resolved Hide resolved

ryzngard reviewed Dec 27, 2023

View reviewed changes

PR Feedback

8f51c5e

davidwengier merged commit 2ded3ad into dotnet:features/cohost Dec 28, 2023
12 checks passed

davidwengier deleted the dev/dawengie/GenerateOnCohostOnly branch December 28, 2023 05:11

davidwengier mentioned this pull request Jan 4, 2024

Merge Cohost feature branch into main #9786

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move C# and Html document generation (ie, compilation) to the Cohost side of the world #9766

Move C# and Html document generation (ie, compilation) to the Cohost side of the world #9766

davidwengier commented Dec 27, 2023

alexgav Dec 27, 2023

alexgav Dec 27, 2023

ryzngard Dec 27, 2023

davidwengier Dec 27, 2023 •

edited

Loading

alexgav left a comment

ryzngard Dec 27, 2023

ryzngard Dec 27, 2023

ryzngard Dec 27, 2023

davidwengier commented Dec 28, 2023 •

edited

Loading

Move C# and Html document generation (ie, compilation) to the Cohost side of the world #9766

Move C# and Html document generation (ie, compilation) to the Cohost side of the world #9766

Conversation

davidwengier commented Dec 27, 2023

alexgav Dec 27, 2023

Choose a reason for hiding this comment

alexgav Dec 27, 2023

Choose a reason for hiding this comment

ryzngard Dec 27, 2023

Choose a reason for hiding this comment

davidwengier Dec 27, 2023 • edited Loading

Choose a reason for hiding this comment

alexgav left a comment

Choose a reason for hiding this comment

ryzngard Dec 27, 2023

Choose a reason for hiding this comment

ryzngard Dec 27, 2023

Choose a reason for hiding this comment

ryzngard Dec 27, 2023

Choose a reason for hiding this comment

davidwengier commented Dec 28, 2023 • edited Loading

davidwengier Dec 27, 2023 •

edited

Loading

davidwengier commented Dec 28, 2023 •

edited

Loading