Semantic Highlighting breaks when rapidly typing due to improper cancellation

[MartinGC94]

Prerequisites

• I have written a descriptive issue title.
• I have searched all open and closed issues to ensure it has not already been reported.
• I have read the troubleshooting guide.
• I am sure this issue is with the extension itself and does not reproduce in a standalone PowerShell instance.
• I have verified that I am using the latest version of Visual Studio Code and the PowerShell extension.
• If this is a security issue, I have read the security issue reporting guidance.

Summary

Like the title says, semantic highlighting doesn't work for any new text you type, it only works for the text that was already there when you opened a file. Interestingly the VS code token inspect feature correctly lists the semantic color but the visuals aren't updated. Initially I assumed this was a bug in VS code itself but if I try it with the C# extension it works perfectly fine in VS code so they must be doing something different from this extension.
Here's a clip that demonstrates the issue:

Highlight.Bug.mp4

PowerShell Version

Name                           Value
----                           -----
PSVersion                      7.5.0-preview.5
PSEdition                      Core
GitCommitId                    7.5.0-preview.5
OS                             Microsoft Windows 10.0.19045
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0…}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0

Name             : Visual Studio Code Host
Version          : 2024.5.0
InstanceId       : 0de14edd-e7a3-4dcc-9ad1-f833dc86ffca
UI               : System.Management.Automation.Internal.Host.InternalHostUserInterface
CurrentCulture   : da-DK
CurrentUICulture : da-DK
PrivateData      : Microsoft.PowerShell.ConsoleHost+ConsoleColorProxy
DebuggerEnabled  : True
IsRunspacePushed : False
Runspace         : System.Management.Automation.Runspaces.LocalRunspace

Visual Studio Code Version

1.96.0-insider
19fabc20e35c89915c772116503a079554166a3f
x64

Extension Version

[email protected]

Steps to Reproduce

Enable semantic highlighting with colors that don't match the colors from the textmate highlighting and start typing.

Visuals

No response

Logs

No response

[JustinGrote]

Thanks for the submission Martin! Keep them coming, I plan to look more into it once I finish up the logging PR, it will help to give much better visibility into what's going on.

[JustinGrote]

I looked into this some more and it looks like things are working fine up to a point where the delta semanticTokens doesn't get a response, and I assume vscode is hanging the semantic provider waiting on this response for that particular document.

No idea why it isn't responding, but the SemanticTokensHandler has a custom omnisharp base class SemanticTokensHandlerBase that handles all the delta stuff automatically and asks us for a Tokenize method to provide it the tokens, which is a bit of an odd method: it gives us a builder, asks us to push tokens to the builder, then return a completed task.

I'll look into this more to see if I can find why it is hanging on not sending the response.

[JustinGrote]

So it seems to be that if the operation is canceled, somewhere that's not getting handled correctly. If you do one edit at a time and wait, it works fine, it's when you type at such a rate that the operation gets cancelled in favor of a new one that seems to bunch it up.

I think it has something to do with the textDocument/completion getting cancelled causing the semanticTokens response to get cancelled somehow, and therefore vscode doesn't ever respond, so we need to make sure the semanticTokens response always replies or at least also replies with a cancelled

[JustinGrote]

OK, I'm very sure it's this problem: OmniSharp/csharp-language-server-protocol#784

Implementing the workaround fixes it, however the average request processing goes up to 500ms with a lot of typing, which is unacceptable. Going to need to either fix this upstream or potentially create our own custom SemanticToken handler and not rely on the base one here.

[MartinGC94]

Nice find!
I don't know much about LSP but the name indicates that C# is using this as well. Since semantic tokens work fine there, maybe it's worth checking out how they solved it without this performance issue?

[JustinGrote]

@MartinGC94 unfortunately the "Omnisharp" LSP in the C# extension has been replaced by a closed source LSP, they no longer use this as their underlying engine, and there's real concern it's going to be abandoned as it is the foundation of our entire extension, certainly development and issue fixing has all but stopped based on the commit history.

So, assuming I can find a way to gracefully cancel this (hopefully the cancellation process mentioned in the issue triggers the cancellation token and doesn't just hard kill the tasks) within the extension, it should be fixable. If it has to be fixed in the LSP, we will have to beg them to release a new version otherwise we may have to fork/innersource it for future.

[JustinGrote]

I checked the Bicep implementation which uses the same omnisharp LSP, and I didn't see any kind of special handling there, so maybe it also affects them, I'll have to test to be sure.
https://github.com/Azure/bicep/tree/main/src/Bicep.LangServer

[JustinGrote]

Here's a potential clue from bicep:

I'm guessing they made all their requests parallel based instead of serial based, so the serial/parallel switchover cancellation doesn't ever happen.

[JustinGrote]

It appears the cancellation of the completionHandler causes an unhandled cancellation exception in our GetCommandInfoAsync here, most likely because we have passed our handler cancellation token to ExecutePSCommandAsync and it is throwing it after doing whatever graceful shutdown it needs to do:
IReadOnlyList results = await executionService.

DidChanged and Completion are the only two LSP commands that I see in the log that are considered "serial" commands and trigger the parallel to serial engine change. I'm not sure why Completion would be that way, maybe we try overriding it to parallel? We can always cancel it if we get a DidChanged and the client can request a restart.

Whatever is happening there apparently isn't "cleaning up" in the way Omnisharp wants in order to proceed, so the semantic response never gets sent, and it stops working beyond that point as the client is still waiting for the first semantic response (there doesn't seem to be any sort of configurable timeout)

It seems to mostly trigger on when you hit a completion when typing with an active completion already in flight to the LSP (as completions can take 300-800ms sometimes).

CC @andyleejordan for help. I'm not quite sure what the coordination is expected to be in the event of a cancellationToken here.

Exception has occurred: CLR/System.Threading.Tasks.TaskCanceledException
An exception of type 'System.Threading.Tasks.TaskCanceledException' occurred in System.Private.CoreLib.dll but was not handled in user code: 'A task was canceled.'
   at Microsoft.PowerShell.EditorServices.Services.PowerShell.Utility.CommandHelpers.<GetCommandInfoAsync>d__8.MoveNext() in D:\PowerShellEditorServices\src\PowerShellEditorServices\Services\PowerShell\Utility\CommandHelpers.cs:line 147
   at Microsoft.PowerShell.EditorServices.Handlers.PsesCompletionHandler.<Handle>d__14.MoveNext() in D:\PowerShellEditorServices\src\PowerShellEditorServices\Services\TextDocument\Handlers\CompletionHandler.cs:line 165
   at OmniSharp.Extensions.LanguageServer.Server.Pipelines.SemanticTokensDeltaPipeline`2.<Handle>d__0.MoveNext()
   at OmniSharp.Extensions.LanguageServer.Server.Pipelines.ResolveCommandPipeline`2.<Handle>d__3.MoveNext()
   at MediatR.Pipeline.RequestPreProcessorBehavior`2.<Handle>d__2.MoveNext()
   at MediatR.Pipeline.RequestPostProcessorBehavior`2.<Handle>d__2.MoveNext()
   at MediatR.Pipeline.RequestExceptionProcessorBehavior`2.<Handle>d__2.MoveNext()

[JustinGrote]

Can't believe I didn't spot this earlier. In registration our completion handler has been forced to serial mode, and I'm guessing Omnisharp didn't account/test for this.

@SeeminglyScience since not all handlers go thru the pipeline like the semantic handler, maybe there's a better way to approach this?

[JustinGrote]

Removing the serial does fix it, the cancellation still happens but it doesn't cause the parallel to serial switch (since all commands past didChange are parallel) and doesn't cause the semantic to be cancelled.