🐞 Passing cancellationToken here (and below) is the bug. If the cancellationToken passed in by the caller is cancelled during this call, that leaves _initializedTask in a cancelled state. So, attempts to await the task just throw a TaskCanceledException and the remote service call never actually happens. Subsequent remote service calls note that initialization hasn't succeeded yet, so they try to initialize again and fail in the same way.

And on and on and on...

When these tasks are in the faulted state, they will permanently stay that way and this will effectively rethrow. Is that the intent here? Assume so but wanted to check.

Yeah, I was thinking about that and decided I'd rather they just throw, at least for now. If we fail to initialize, that means that we couldn't get a remote client or the remote initialization call itself failed somehow (e.g. the process gets torn down or something like that).

I think we can make this more resilient and fail gracefully, but we need to change the TryInvokeAsync method above to do it. Currently, it just returns TResult?, so there's no way for a caller to tell the difference between an expected null result and a null-because-we're-in-bad-state result. So, instead of returning TResult?, I would propose returning Roslyn's Optional<TResult>, which is what RemoteClientHost.TryInvokeAsync is returning to us anyway. However, that's a much larger change to update all call sites. I'll file a bug for that and clean it up in a follow up.

#11431

Happy to see remoteClient acquisition moving into the method vs. being passed in. Previously it was passed in but only conditionally used based which felt a bit odd. Now it's still conditionally used but the context is much clearer about when that occurs.

I spent a lot of time looking at the relationship between CanGetClientCapabilities and GetClientCapabilities. Essentially, seeing if behavior in any of the implementations could break this code path. Particularly given that their appear to be threading concerns in this code (hence the lock) and that the property is explicitly mutable. Seemed like I could get things to break if I could find a place where we evere went from CanGetClientCapabilities being true to being false.

After digging through the implementations I can't see a case where this could occur. At the same though I was wondering if you considered just changing the interface to more simply:

internal interface IClientCapabilitiesService
{
    VSInternalClientCapabilities? ClientCapabilities { get; }
}

Basically let null be the "cannot provide" state? That is what all the implementations seem to do.

You're correct. CanGetClientCapabilities can't really ever transition from true to false. (Unless the language server is torn down and started up again, but that's a whole other problem!)

Unfortunately, changing the interface to let null be the signal is a much more substantial change. There is a lot of code that touches that property that would need to be visited and updated (see below). Also, the vast majority of the code paths that touch the ClientCapabilities will only ever be called when the property is guaranteed not to be null by the LSP contract. E.g., the LSP client won't call them until it has called the "initialize" handler, which is what sets this value. So, while it's a bit more awkward and looks a little funny, we should probably leave CanGetClientCapabilities in place, at least for now.