Expose a way for Razor to register capabilities into the AlwaysActivateInProcLanguageClient#72609
Conversation
|
Converting to draft, as the semantic tokens queue needs more work for us to actually consume. |
|
Okay, fixed things. This is ready for review so we can consume in Razor and move to dynamic registration in future (and then remove the cohost language client and some related classes/interfaces) |
ae30c7a to
8ed8018
Compare
Previously we didn't need this because CLaSP wasn't a source package
|
@dibarbet PTAL. With your recent changes for serialization, this actually is very easy and pretty much entirely limited to the Razor EA, so hopefully not a big deal. This doesn't support text sync, but I don't know how to do that yet. Not sure whether we want to allow Razor to plug in to the text sync handlers, or have it use |
src/Tools/ExternalAccess/Razor/Cohost/RazorDynamicRegistrationServiceFactory.cs
Outdated
Show resolved
Hide resolved
| public void Initialize(string clientCapabilitiesString) | ||
| { | ||
| var clientCapabilities = JsonConvert.DeserializeObject<VSInternalClientCapabilities>(clientCapabilitiesString) ?? new(); | ||
| semanticTokensRefreshQueue.Initialize(clientCapabilities); |
There was a problem hiding this comment.
will this result in initialize being called twice? (one for the main on initialized, once for razor oninitialize?
There was a problem hiding this comment.
Yesn't. This Razor class only initializes in the AlwaysActivatedInProc server, and its main oninitialized will call the queue to initialize, but since it doesn't handle semantic tokens, that call does nothing. That's why I had to extract the initialize method out in the queue itself, so I could call it here separately.
There was a problem hiding this comment.
If it isn't too hard can we make the Initialize resilient to being called twice (in a potential race). I worry we won't realize its being called twice later (e.g. when we enable cohosting in vscode which does call semantic tokens initialize).
There was a problem hiding this comment.
Oh, the initialize call in the queue itself is resilient. It doesn't lock though. Do you think that's worth it? Are we likely to fire IOnInitialized handlers in parallel ever?
There was a problem hiding this comment.
It seems like it could race if two calls to initialize run in parallel (might recreate the actual batching work queue and subscribe twice to the events).
But now that you mention it, if both calls are coming from an IOnInitialized service we do technically run them serially
So it probably wouldn't get called twice as long as that is the case. Would be good to add comment on this mentioning that is why its safe.
There was a problem hiding this comment.
Yep, will do. The indirection makes it opaque, but yes the call to this method comes from code in the Razor repo which is called from that said IOnInitialized loop
What do you need to do? the text sync handlers are intentionally very very light (so there is little chance of exceptions and as little work done as possible) |
At the moment in cohosting I use text sync to generate the Html document, and update its buffer. In regular Razor LSP it happens after a short delay, and we have a synchronization system to try to make requests wait. The idea of using the RequestExecutionQueue as that synchronization system is very tempting, but at the same time, it probably doesn't fall under "as little work done as possible". That's why I punted this for now, since cohosting doesn't actually call the Html server for anything yet. Once I've done that, hopefully we will be in a better place to understand the tradeoffs. I'm sort of hoping the sync system we have can just be simpler, because it's only worrying about Html, not C#, and isn't even necessary for some things (eg, semantic tokens, which never calls Html anyway) |
I think the way I'd prefer to see it happen is that the synchronization doesn't happen until there is a request that requires it. This is what we do for applying LSP solution changes for example - in text sync we just save the text and wait to fork the solution until we're building the request context for a request that requires it. Note that this still happens serially during request context creation (before the request is dispatched to the handler) |
src/Tools/ExternalAccess/Razor/Cohost/RazorDynamicRegistrationServiceFactory.cs
Outdated
Show resolved
Hide resolved
I agree because
This is exactly the bit that makes it hard for Razor to do it any other way than waiting for the first request that needs it, because to generate the Html we need a document for the source generator to see. |
This is the Razor side of dotnet/roslyn#72609 It can't build yet, because that PR hasn't merged, and it doesn't result in colours in the editor being applied yet due to platform bugs in semantic tokens (specifically https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2003715 mainly), but thought I'd put it up in case anyone wants to look at it, and so I don't forget it exists.
This will allow Razor to use dynamic registration, and run in the existing "normal" Roslyn LSP server, rather than having us use a separate cohosting server. Once this is consumed on the Razor side, most of the rest of the cohost folder in the EA can be deleted.
There is one hack here that will be unnecessary once we can use the Roslyn protocol types.
Part of dotnet/razor#9519