Skip to content

[LSP] Disable text sync for C# in 16.9#51708

Merged
dibarbet merged 3 commits intodotnet:release/dev16.9-vs-depsfrom
dibarbet:169_disable_text_sync
Mar 10, 2021
Merged

[LSP] Disable text sync for C# in 16.9#51708
dibarbet merged 3 commits intodotnet:release/dev16.9-vs-depsfrom
dibarbet:169_disable_text_sync

Conversation

@dibarbet
Copy link
Member

@dibarbet dibarbet commented Mar 6, 2021

Disables text sync for C# in 16.9 due to https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1287570
The current known issues are on the LSP client but the fixes are in their refactoring.
We disable text sync for C# for the following reasons

  1. We don't need text sync for C# right now (it is only for cntrl+Q search which can use the workspace text).
  2. There are very likely other underlying text sync issues, so if we don't throw and restart we may end up with invalid documents or encounter other issues which would lead to wrong cntrl+Q search results (e.g. we get an open and then drop all change requests). It is best to just use the workspace text which we know to be correct.
  3. There's not much more we can learn from 16.9 by leaving it on. There is more LSP client logging in 16.10 (and on our side), we've improved our text sync handling, and the LSP client refactor will fix a ton of race conditions. Additionally, there are bugs in 16.9 with attaching loghub logs to feedback, so we don't even always get logs. We'd be doing more harm than good by leaving it on.
  4. Wholesale disabling text sync is a small change that is easy to verify.

We leave it enabled for razor for a couple reasons.

  1. LSP for razor is non-sensical without text sync. Diagnostics will be mismatched, completion will be wrong, etc.
  2. LSP for razor is experimental and under a feature flag and not rolled out completely
  3. Text buffers for razor are in-memory and not actual files and are protected by client name. We think it is less likely that changes to these buffers (e.g. during git checkout / project reload) will randomly become text files. We don't have feedback issues around txt files causing the razor c# server to restart (though there are other known text sync issues, e.g. https://github.com/dotnet/aspnetcore/issues/30002)

f846b17 should be reverted in 16.10 as we've somewhat mitigated the restarting issue and there are additional LSP client logs to help diagnose issues. Will followup with PR as soon as this merges to main.

6bc956f adds extra logging for text sync to 16.9 so for scenarios where it is enabled (razor) we have logs to show the issue.

FYI @jinujoseph

@dibarbet dibarbet added Area-IDE LSP issues related to the roslyn language server protocol implementation labels Mar 6, 2021
@dibarbet dibarbet requested a review from a team as a code owner March 6, 2021 00:52
@dibarbet dibarbet changed the title 169 disable text sync [LSP] Disable text sync for C# in 16.9 Mar 6, 2021
@dibarbet dibarbet merged commit 59eedc3 into dotnet:release/dev16.9-vs-deps Mar 10, 2021
@dibarbet dibarbet deleted the 169_disable_text_sync branch March 10, 2021 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Approved to merge Area-IDE LSP issues related to the roslyn language server protocol implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants