workspace/didChangeConfiguration: InvalidOperationException: An attempt was made to transition a task to a final state when it had already completed.

[dkattan]

PowerShellEditorServices/src/PowerShellEditorServices/Extensions/EditorObject.cs

Line 140 in a12edc9

s_editorObjectReady.SetResult(true);

Consider using TrySetResult to prevent this exception when the editor is re-initialized or sends a workspace/didChangeConfiguration request

@rjmholt

[dkattan]

Also consider checking that incomingSettings.Powershell isn't null before using it in ConfigurationHandler.SendFeatureChangesTelemetry:

PowerShellEditorServices/src/PowerShellEditorServices/Services/Workspace/Handlers/ConfigurationHandler.cs

Line 161 in a12edc9

if (incomingSettings.Powershell.ScriptAnalysis.Enable == false &&

The MonacoLanguageClient sends an empty settings object along with workspace/didChangeConfiguration and it throws an exception.

{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{}}}

@TylerLeonhardt

[andyleejordan]

I see two different bugs here in one issue, but will get them addressed.

[dkattan]

I included both in the same issue since the repro steps are the same for both.

[andyleejordan]

@rjmholt Should we consider changing all uses of SetResult to TrySetResult? When do we actually ever want the server to crash?

[rjmholt]

When do we actually ever want the server to crash?

Just while I think of it, there's an answer to this: when we're debugging it. SetResult() crashing is useful to verify that a result is set exactly once under things like concurrent conditions (since otherwise subtler bugs may be occurring). When we actually stabilise a feature using SetResult(), it should probably use TrySetResult().

[dkattan]

When do we actually ever want the server to crash?

Just while I think of it, there's an answer to this: when we're debugging it. SetResult() crashing is useful to verify that a result is set exactly once under things like concurrent conditions (since otherwise subtler bugs may be occurring). When we actually stabilise a feature using SetResult(), it should probably use TrySetResult().

To be fair, that's how I found this bug. But rather than letting it crash, why not write a warning so that it shows up in the console? For my use case, it is normal for it to not be able to set these values until Monaco gets around to sending the didChangeConfiguration message.

[andyleejordan]

I wonder if this is somehow related to how the extension would fail when the OmniSharp race condition was present. The race prevented its didChangeConfiguration from reaching the server, causing it to never start. Manually triggering a second didChangeConfiguration would "fix" it. 🤔

[dkattan]

I wonder if this is somehow related to how the extension would fail when the OmniSharp race condition was present. The race prevented its didChangeConfiguration from reaching the server, causing it to never start. Manually triggering a second didChangeConfiguration would "fix" it. 🤔

That sounds very similar to what I was experiencing initially. Except the language client wasn't ever sending didChangeConfiguration.

I dug through the code trying to figure out what sets these values and determined that they were only getting set when the client sends didChangeConfiguration.

I stumbled across TypeFox/monaco-languageclient#201 and subsequently a conversation with the guys at Theia here where they say that the Language Server Protocol doesn't specify that the client needs to send didChangeConfiguration, but VS Code does it anyway. Which makes sense as to why this isn't a problem in you guys' testing.

The "new" way to do this is evidently to advertise support for didChangeConfiguration and have PSES ask for the configuration.

But truthfully it doesn't matter, as long as I am able to mimic VS Code's behavior of sending it, and not having PSES crash when the settings is null.

[andyleejordan]

Woah, super informative. Thanks @dkattan! Also raxod052 is someone I used to work with a bit when I was using Emacs for everything (which was my entire life before I took over the VSCode extension for PowerShell). I think part of the backlog for PSES to actually update the logic to the new, modern spec with dynamic registrations and such, we just haven't gotten to it yet.