Dont Use LanguageServiceAccessor! #558
Replies: 17 comments 12 replies
-
|
For me, the use case is that we add some custom protocol to our language server and so we also need a means to get a reference to something that lets us 'talk to' the language server using that custom protocol. I figured that I need to get a hold of a 'LanguageServerWrapper' which seems to be the thing that represents a language server instance and has all the machinery to talk to the remote server. However, perhaps this PR: #543 might provide a different way to do what I want, as you (@angelozerr) suggested in our other discussion. I am not sure yet whether the PR does what I want/need because I don't understand how it works yet. What I will do is try to actually use it and see whether I can make it work for me. That will hopefully also allow me to give some constructive feedback on the PR. |
Beta Was this translation helpful? Give feedback.
-
|
Thanks @kdvolder for your feedback. Have you some link to your code project to understand more what you need? |
Beta Was this translation helpful? Give feedback.
-
|
@kdvolder perhaps I have not understood your usecase, but it seems you want to communicate to your language server from your LancguageClientImpl, right? If it that you have a getLanguageServer() which is used for that. See how we use this getlanguageServer in our LSP Qute client https://github.com/redhat-developer/intellij-quarkus/blob/eedb7bd64c10ac7838e4269d7af02dc8835cd2ab/src/main/java/com/redhat/devtools/intellij/qute/lsp/QuteLanguageClient.java#L82 This code send a custom notification to the Qute language server. is it your usecase? |
Beta Was this translation helpful? Give feedback.
-
|
I think the use case is similar, but I am not sure its the same. I am also not sure I understand it from your example. But I think it boils down to the fact that there are two places you can add custom api.
We are adding a request respone pattern so that we can ask the server to send us information. This means adding a thing like 'getListOfApp' that the server responds to. If you actually want to be able to make use of this new protocol you added you will need to obtain a reference to the 'LanguageServer' instance on the client side. I cannot send you a link to our code because sadly this project is not open-source. I hope it may become so as some point but at broadcom the policy is not to make new projects open-source by default. So let me try and explain it as best as possible what our 'use pattern' is. Say you have a server which has a model that contains information you want to display in a view (like a tree/structure viewer) . This information is very 'custom' so does not fit into any language-server protocol. For a simple example, (close to our use-case). Say this language server is watching the state of some applications deployed to a cluster. The view displays a list of applications. When the view opens up, it needs to get access to an object that it can use to send messages to the language server (and receive responses back). i.e. we want to have some code like (not actually correct code, but to convey the gist of it): There are also more complex 'subscribe/even-stream' patterns which extend the protocol on the client (i.e. to receive notifcation/events for something you subsribed to). I think in your example you are extending the client-side api (messages a server can send to a client), and you can have code that reacts to those messages and does things on the client side. In our example there is that kind of thing as well, but there is also extra protocol on the other side (custom messages a client can send to a server). |
Beta Was this translation helpful? Give feedback.
-
|
Ok I think I understand more what you need. I have the impression that you need the same thing that we have to support workspace symbol (since you don't have file in parameters) https://github.com/redhat-developer/lsp4ij/blob/main/src/main/java/com/redhat/devtools/lsp4ij/features/workspaceSymbol/LSPWorkspaceSymbolSupport.java by replacing:
and after that you consume the support like this: Our support are very advanced because they support cancel support which is very important to have the best performance. right? If it that, I think we should provide a base class which usesinternally LanguageServiceAccessor and implement a method like I mean you will just need to implement filter to collect your ls and cast the LanguageServer instance with your Custom Language Server API to consume your custom server service and collect data. My main goal is to avoid using LanguageServerWrapper and LanguageServiceAccessor and just using LSP4J LanguageServer and the support will give you the cancel support feature. Makes sense? |
Beta Was this translation helpful? Give feedback.
-
|
Yes, the WorkspaceSymbolSupport looks like it implements the kind of 'request / response' protocol that we want. And yes, I can agree that LanguageServerWrapper / Accessor aren't meant to be public api and it would be nice to design something more principled. What I don't understand yet in that sample code is how it triggers and synchronizes server startup/initialization. The code I see in WorkspaceSymbolSupport Uses 'getStartedServers' so I guess it assumes server must be started somehow beforehand, I haven't been able to find where that actually happens. This is important. When we would ask for 'ApplicationViewSupport' then we need to be able to get some guarantees that the server is started/initialized before trying talk to it. So we use a pattern similar to yours but instead use another one of the accessor methods: Actually we are using one of the methods that requires a 'file' as a parameter though we really don't want to. The reason we use this method because it starts server 'as needed' and does the 'intialize/sync' logic. The method we actually would want doesn't exist (yet). We want one that is like 'getStartedServers' in that it doesn't require a 'file' as context (just a project), but has the 'start/init/sync' logic as well. Note that this WorkspaceSymbolSupport is only an example of req/response pattern. But here is also a use pattern where the client (our plugin) would send a 'subscribe' message to the server and then expects to start receiving notifications/callbacks (i.e. a more sophisticated version to info the server will send 'change events' as notifications back to a subscription on the client and then there needs to be a way to handle these notification messages to update the view. So imagine something like 'TanzuAppSupport.subscribe(...some-params..., listener). To be able to extend the LSP protocol in these scenarios you need to be able implement handlers on the client. We do that sort of thing by overriding 'createLanguageClient' in LanguageServerFactory. This works but it is a bit tricky to decide which 'subscription' (listener callback) a particular notification should be forwarded to when it is received. This is an interesting discussion. But it is getting a bit difficult to type it all out. And probably it may be equally cumbersome for you to read/parse my ramblings on your end as well. So I wonder if perhaps it might be a good idea to try and setup a video chat or something like that with 'higher bandwidth communication channel'. If you want to do that I'd be happy to post a summary here afterwards (for the public record). |
Beta Was this translation helpful? Give feedback.
-
|
BTW: There is a certain logic to the fact that LanguageServerAccessor method that only takes a project and not a file doesn't provide server startup, and instead only grabs already started servers. The logic here is consistent with the idea that server life-cycle is tied strictly to open editors. But for features like ours and WorkspaceSymbols, then there may not be a file open and I guess you will run into a similar problem as us as well. Namely that workspace symbols will not work unless you have at least one file open to start the language server. Perhaps that is acceptable but it probably isn't 'ideal' :-). So indeed I think you picked a good example to compare/equate to our use-case. |
Beta Was this translation helpful? Give feedback.
-
It is exactly why LanguageServerAccessor should be never used (for the moment) because this class comes from LSP4E and I spend my time to clean up to have the best API. The workspace symbol use-case was the first LSP feature which doesn't requires PsiFile. When you open Symbol (Ctrl+N), I didn't want to start all language servers to check if each language server supports workspace symbol. It was the reason why I did that, and too as you have noticed LanguageServerAccessor works only with file. BUT in my PR #543 I have fixed that and now you can comsume LanguageServerAccessor without file parameter and with the power of return getLanguageServers(project,
f -> f.getWorkspaceSymbolFeature().isEnabled(), // here check by default that ls is starting or started
f -> f.getWorkspaceSymbolFeature().isSupported()) // here check by default that the ls supports workspace symbol
.thenComposeAsync(languageServers -> {As you can see you have no dependency to the LanguageServiceAccessor because getLanguageServers is defined in the bastract method. Once this PR will be merged you will able to to extends AbstractLSPWorkspaceFeatureSupport and write this kind of code: return getLanguageServers(project,
f -> f.isServerDefinition("Tanzu"), // here check that ls is Tanzu language server (done before starting the ls)
null) // no need to check capability (done after starting the ls)
.thenComposeAsync(languageServers -> {By extending AbstractLSPWorkspaceFeatureSupport you will benefit with cancel support and you will have no dependency to the LanguageServiceAccessor.
Yes I know that, but with my current PR you should have this feature.
Perhaps I have not understood your need, but it seems it is exactly that we do in our Qute / Quarkus language client.
Is not your usecase? The thing which is very important is to remove listener on the dispose method, because if you stop and restart the language server,the client instance is re-created. |
Beta Was this translation helpful? Give feedback.
-
I will be happy to discuss with you, but let's merge the PR and see if it is enough for you otherwise let's do a meeting if my doc and PR doesn't cover your need :) |
Beta Was this translation helpful? Give feedback.
-
|
@kdvolder after some discussion with @fbricon we will provide a simple API to write
|
Beta Was this translation helpful? Give feedback.
-
|
@kdvolder I did the change, please read the DeveloperGuide.md from the PR, you should have the capability to write: CompletableFuture<List<Application>> applications =
LanguageServerManager.getInstance(project)
.getLanguageServer("Tanzu")
.thenApply(languageServerItem ->
languageServerItem != null ? languageServerItem.getServer() // here getServer is used because we are sure that server is initialized
: null)
.thenCompose(ls -> {
if (ls == null) {
return CompletableFuture.completedFuture(Collections.emptyList());
}
TanzuServerAPI myServer = (TanzuServerAPI) ls;
return myServer.getApplications();}
);or Project project = ...
CompletableFuture<@Nullable LanguageServerItem> languageServerFuture =
LanguageServerManager.getInstance(project)
.getLanguageServer("Tanzu");In an another piece of code: CompletableFuture<List<Application>> applications =
languageServerFuture
.thenCompose(languageServerItem -> {
if (languageServerItem != null) {
return CompletableFuture.completedFuture(null);
}
return languageServerItem.getInitializedServer(); // here getInitializedServer is used because language server could be stopped and must be restarted
})
.thenCompose(ls -> {
if (ls == null) {
return CompletableFuture.completedFuture(Collections.emptyList());
}
TanzuServerAPI myServer = (TanzuServerAPI) ls;
return myServer.getApplications();}
); |
Beta Was this translation helpful? Give feedback.
-
|
@kdvolder you can play with this API now with https://github.com/redhat-developer/lsp4ij/?tab=readme-ov-file#testing-nightly-builds, see #561 for more information Any feedback is welcome to have a clean API before the release of 0.7.0 (for the moment no plan to do this release, we will see that according feedbacks) |
Beta Was this translation helpful? Give feedback.
-
|
This is great. I tried the api. I used I was a little confused about the two different usage patterns in your example and why one can use 'getServer' whereas the other needs to use 'getInitializedServer'. Yes, I see the comment that tries to explain this. But I just couldn't quite understand why in one case you can think you can be sure the server is initialized whereas in the other case you do not. After some thought, I guess the reason is that in the second scenario because the creation of the If so, this is really not something we would want to happen. It is okay when the protocol / server is conceptually 'stateless'. However, for a more complex 'subscribe/eventstream' this is not the case. Adding a 'subscription' to the server inherently is stateful. And shutting down the server means the subscription/state is lost. Symptom on the user side will be the views depending on the subscription events will stop updating when the server shuts down. And even if the server gets 'restarted' the new server will not have a 'memory' of the active subscriptions anymore. I suppose there are ways to deal with that somehow but at the very least you would need to somehow be able to know when the server got shutdown and/or is being restarted so you can reestablish any subscriptions that were implicitly terminated when server shutdown. This seems complicated (and I wouldn't know right now how to implement that within the available apis, its a bit of a puzzle how to do that). It is a much simpler model to implement if you can instead 'claim a lease' on the server that can keep it active unless you no longer have any active subscriptions. Anyhow... I am using the 'hack' you suggested before, of setting a high value for PS: I also took a look at the It seems to implement a kind of 'cache the last request / response' kind of pattern. Where the 'checkValid' is intended to validate whether the cached value can be reused or not. But its odd to me that checkValid method doesn't have access to the Params to make that decision (wouldn't you need to really check whether the 'new' parameters haven't changed in a 'meaningful way' from the 'old' parameters to decide whether values need to be recomputed or can be safely re-used? Also the caching behavior isn't really that useful to me right now. In fact we'd probably only make a request to explicitly 'refresh' the view, or to initiate/terminate a subscription for change events, in both cases we explicitly wouldn't want to get a cached value but want to make sure the request is really executed on the server. |
Beta Was this translation helpful? Give feedback.
-
Great!
Yes it is that. When LanguageServerItem is created, the ls is initialized and the server field (used in getServer) is set with it. If you stop the language server, the server field has a reference of a language server which is shutdown. We use LanguageServerItem internally and we use getServer. But as LanguageServerItem could be cached, I have add getInitializeServer which starts the ls if needed and returns the proper language server. Hope you will understand my explanation. The doc is not perfect,any PR are welcome!
Ok
Don't use this hack, you can configure that now, see https://github.com/redhat-developer/lsp4ij/blob/main/docs/LSPApi.md#lsp-client-features by overriding
If I understand, you should have a queue with events which belong to a project and this queue must not loose some events when server is stopped. If it that, I will create a class like
If your need is to just refresh a View, I think its not interesting to use AbstractLSPFeatureSupport
Int he case of main LSP features, the cache must be evicted when PsiFile is updated. It is a generic behaviour. For some LSP features like completion, the future is canceled when offset changes in the load. Anyway if your need is just to collect some data like list of Application for a View you don't need this support class. If you need more API please tell me. |
Beta Was this translation helpful? Give feedback.
-
Not exactly, well yes that could also be a concern as well. But its more than that. The server's job is to watch changes and send updates. When there is no server because it is shutdown, then there is no way for the server to keep doing its job. It is not just an issue about some events getting lost 'on the boundary'. Moreover this works by a kind of subscription protocol. The client calls the server to declare interest in a topic (i.e. subscribe to some specific type of changes in some specific scope). This creates an 'active subscription' on the server (this is what starts the server to watch for those particular things to happen and create events to send back to the client as notification). On a subscription request the server also responds with a unique id that identifies that specific subscription. And this id is also included in notifications sent back (so the events can be routed to a specific subscriber/listener on the client side). The point here is the server maintains a list of 'active subscriptions' and when the server shutdowns and is restarted arbitrarily, it will 'forget' this list and so even when you restart the server the subscriptions will no longer be active. I suppose you could work around this problem if the client-side is able to detect that the server got shutdown and reestablishes the subscriptions when it restarts it. But as it is now its kind of hard to tell the difference when you use 'getInitializedServer' whether you got a restarted server that lost its memory or one that still 'remembers everything'. Anyhow... I'll look at using 'keepAlive' instead of the 'long timeout hack'. That will solve the issue and seems better than the hack. Thanks for the tip. Perhaps we can even implement it to only return 'true' if their are active subscriptions and false otherwise and it will allow for server to shutdown when no longer needed. I suspect though that in scenario where:
The server will not get shutdown simply because there is not going to be anything triggering a 'recheck' of that 'keepAlive' condition that has changed by removing the last active subscription. The way to remedy this would be to add something to the api that allows client to do something like ask 'please keep the server alive for me until I say I no longer need it'. I.e. a kind of 'cancelable lease' that is attached to a languageserver reference. I could imagine an api similar to what you gave me in LanguageSeverManager but where the 'LanguageServerItem' returned is or has a Or I could imagine separate api wich allows adding 'Disposable' leases to an already existing and active language server. Either api would work. But to me it seems logical that when you request a language server reference from the LanguageServerManager api:
This is why I think the api should return something that is 'Disposable/Cancelable'. |
Beta Was this translation helpful? Give feedback.
-
|
Let me time to re read your comment, but my first feeling is that you need to track server status like started, stopping and do something when server status changed. We have a listener manager which takes care of that and we use it to update for instance the icon to show the state of the server in the lsp console. But please dont use it because those listener works with LanguageServerWrapper which is not an api. If you need to track the server status, my idea is to provide in the LSPClientFeatures a new method like handleServerstatuschanged which will have the exidting ServerStatus enum. If you have that, is it covers your need? |
Beta Was this translation helpful? Give feedback.
-
I don't. That is assuming I have a way to keep the server alive while I am using it. I would only need to be able to track server status as a workaround to cope with the 'unwanted shutdown' and do some hacky stuff around that to
With a proper protocol that provides assurances that server doesn't get terminated while it is in use we don't/wouldn't need to implement these workarounds. We now have the 'keepAlive' which serves the need just fine. However, as I said, I think it has this minor bug that there won't be a 'trigger' to shutdown the server when both of these are true at the same time:
In this scenario language server will stay alive even thoug keepAlive would return false (if it were called again). This is a bit of en edge case and maybe not very important. I am not sure how often this would happen and how much it would matter (and even with that bug... this is still better than setting a really long timeout). |
Beta Was this translation helpful? Give feedback.
-
Formally, I guess it would. But its really not what I want. I really don't need to track the server status what I really need instead is a way to tell the languageservermanager "please don't terminate this server while I am using it". Tracking server status and reacting to it would only be a 'coping mechanism' to handle something we don't actually want to happen in the first place. |
Beta Was this translation helpful? Give feedback.
-
|
The language server wrapper start to be stopped here If I understand your need, you would like to specify a CompletableFuture like Is that? |
Beta Was this translation helpful? Give feedback.
-
Its is a specific usecase, right? You can stop/start the language server with Api https://github.com/redhat-developer/lsp4ij/blob/main/docs/DeveloperGuide.md#start--stop |
Beta Was this translation helpful? Give feedback.
-
|
As I am trying to implement our 'keepAlive' with some real logic rather than just have it always returning true... I am realizing that I need more context information to do so. Particularly, I have no way to know which server a 'createClientFeatures' call relates to. So since subscriptions belong to specific servers we need some way to know that. Perhaps passing in the Project would be enough, or a reference to the |
Beta Was this translation helpful? Give feedback.
-
I suppose so, but then I need to be able to avoid stopping it when there are open editors. Really I do not want to actually stop it. I just want a way to be able to say "Do not stop it while I am using it". That is not quite the same. |
Beta Was this translation helpful? Give feedback.
-
I don't understand what you suggest there but even so I think its not what I want. I don't want to shutdown the language server. I want the lsp4ij to manage ls lifecycle pretty much the way it already does. The only thing I want to do is put an additional 'keep alive' claim on the server process for some durarion. I.e. just like an open editor right now represents a 'keep alive' claim. So would I want our view to be a 'keep alive claim' as well. I don't really want to say 'please shut down the server now'. I want to leave that up to lsp4ij. All I want to be able to do is something like this: At the point where serverAccessClaim is disposed, the number of open claims (tracked by lsp4ij) will be decreased by 1 and if it hits 0 and at the same time there are no open editors (which also represent a kind of 'accessClaim') then shutdown sequence is triggered. I don't actually want to call server.shutdown instead of 'dispose'. The reason being that I think only lsp4ij itself is in the right position to make the decision whether 'there is anything else still using the server'. The call to 'dispose' there says 'I no longer care if you keep the server alive'. It doesn't say 'shut down the server now'. Lps4ij now knows that I don't need the server anymore and can re-evaluate whether it can/should shutdown the server or not using information from 'all possible other things that might need the server still alive'. |
Beta Was this translation helpful? Give feedback.
-
I suppose, its a matter of poinf of view. But I think its more of a generalization of what you are already doing with respect to open editors. So I think these kinds of 'leases' on a languageserver would be a pretty generic thing to add to the api. I also think its very natural when you are 'using' a language server you'd want to have assurances that it doesn't get shutdown while you are using it. So I think it represents a pretty general kind of need for anyone who'd extend the language server with some custom protocol and then wants keep hold of a reference to the server to talk to it. |
Beta Was this translation helpful? Give feedback.
-
|
Today by default language server can be stopped
If I understand your need you want to avoid stopping with 1) and 2) For 1) we have now keepServerAlive. For 2) we should provide a new method like canStopByUser in LSPClientFeatures When canStopByUser return false, the stop menu buttons will be hide. And to stop the ls you use the api Makes sense? |
Beta Was this translation helpful? Give feedback.
-
|
It makes sense for the most part. Also interesting, I hadn't actually thought about the fact that user can stop ls by menu. I guess that's kind of 'annoying' but its not as fundamentally problematic. If the user really does that, then I suppose we'd need our view to be able to just detect that this happened and rather than override this user action and reconnect, we'd need to display some kind of a message in the view indicating it has stopped working because basically the user terminated the connection themselves. Or we add that api you suggested to 'disable/disallow' that user action. That's probably actually better/nicer.
This is the part that still doesn't quite make a lot of sense to me. To be clear. I don't really have a problem that there is an api to allow someone to do that really. I just don't want to use that api. It is not the api for me. I don't want to explicitly call 'stop the server' api because its too difficult for me to implement that correctly and without duplicating the logic you already have implemented to detect 'no more open editors'. I suppose I could, assuming you provide api for me to check whether there are open editors. So I can then effectively duplicate the termination logic and then 'add my own twist' on top op it (i.e. I will need to add a check right before I call 'stop the server' to make sure there are no more open editors. But I really don't think that is a good solution. What happens if this logic changes in lsp4ij? Also there's still the caveat that I can't really correctly implement the 'keepServerAlive' with the api as it designed now. As I tried to explain somewhere in the wall of text above (too much text, its easy to miss). I do not actually know at the point where `keepServerAlive' is called which server needs to be kept alive. This is not that big deal, but I think the method needs some extra argument(s) to provide enough context to the implementor to know which server we are being asking to 'keepAlive'. |
Beta Was this translation helpful? Give feedback.
-
|
As it seems kind of hard for me to explain it any better and we don't seem to be getting on the same page because I am probably not explaining myself clearly enough. I thought the best thing to do would be to quickly hash out an implementation of my idea: Note: this PR has not been tested and could have logic bugs in it. Think of it more as a way to actually show what I'm thinking than a working implementation. |
Beta Was this translation helpful? Give feedback.
-
|
Hi I am not using |
Beta Was this translation helpful? Give feedback.
-
|
I have started to do a refactoring to avoid using lanaguage server wrapper and it impacts badly those listeners. I think having an handleStatuschanged in LSPClientFeatures will simplify code with listener. |
Beta Was this translation helpful? Give feedback.
-
I have tried to add a comment in
LanguageServiceAccessorbut I create an announce about that.Please don't use LanguageServiceAccessor! This class comes from LSP4E and the API is not stable. For instance LanguageServiceAccessor uses Predicate to check if the language server has some capability, but ServerCapability is for static server capability, not for dynamic server capability.
By using this Predicate we cannot support documentSelector for instance you should never done this kind of code https://github.com/themartdev/intellij-gleam/blob/9eea2c41723d6c2b99bf15e70b04545952b4f194/src/main/kotlin/com/github/themartdev/intellijgleam/ide/formatter/lsp/GleamAbstractLspFormattingServiceProxy.kt#L38
//cc @FalsePattern @themartdev
I understand you are doing that because current LSP4IJ version doesn't allow LSP formatting when it already exists an formatter, but LSP API PR which will be available in 0.7.0 #543 will fix this problem (see LSPApi.md doc from this PR).
Today I have never seen some usecase to use LanguageServiceAccessor in an external plugin. If you have a usecase, please share with us and in this case we should provide a clean API (with an another class I think).
cc @CppCXY @InSyncWithFoo @seachicken @KUGDev @AlexWeinstein92 @aparnamichael @MituuZ @turkeylurkey @mrglavas @koxudaxi @FalsePattern @onriv @kristofvb @themartdev
Beta Was this translation helpful? Give feedback.
All reactions