[FOR DISCUSSION] - Draft revised API

[ahmedneilhussain]

Following the discussions on #251 about the need to reform the UI to address concurrency problems, I have gone through the existing API, specifically LanguageServiceAccessor, LanguageServerWrapper and LSPDocumentInfo in some detail, to try and understand the use cases.

Please see LSPExecutor in the latest commit for an outline of what I think could be implemented.

I have based this commit off of #251 as it will build on some of the work in that branch (but replace some of it), although I possibly should have just kept the design discussion based off master (sorry!)

The core workflow for most uses of the language server goes as follows:

1. Select the Language Servers that apply to the task at hand. This boils down into using either an IDocument or an IProject as a selector, combined with a server capabilities predicate.
2. Dispatch operations onto the server. All LSP4j calls work asynchronously, returning CompletableFutures, so the dispatch will result in one or more completable futures.
3. Process the results when the CompletableFuture(s) complete.

Stage 3 is complicated by the fact the calling code must deal with potentially more than one language server in play. At the moment, this is handled ad-hoc by the calling code. This results in a lot of messy code at the various call-sites, which tends to obscure what is going on. I think it would reduce boilerplate to handle this as part of the API, and I think this can be done fairly easily as there are only a small number of actual patterns in play, probably reflecting the fact that there are not too many sensible ways of handling multiple results! I propose:
a. Return all the results aggregated as a CompletableFuture<List<T>>. This is by far the commonest case in the existing code, and the one I already implemented in #251 .
b. Return all the results separately as List<CompletableFuture<T>>. This is a minor use case at present, but crops up in e.g. LSPCodeActionsMenu where placeholder items are created and replaced as the server results return one by one.
c. We only want one result, e.g. edits from a format request. This isn't handled well at the moment - there are a couple of instances of just arbitrarily grabbing the result from the first server in the list, with a TODO about maybe selecting the result from the first server to respond. I think I know how to implement this with a bit of CompletableFuture trickery and so we can have a method returning CompletableFuture<Optional<T>>

Stages 1 and 2 are complicated by a lot of lifecycle logic, with some pretty subtle considerations. For this reason I propose to implement this part of LSPExecutor largely on top of this existing logic inside LanguageServiceAccessor and LanguageServerWrapper. This avoids the danger of ditching and reimplementing a lot of presumably battle-hardened code. It also provides a smooth migration path: the existing methods can be deprecated and then made internal-only (with any unused ones deleted).

I have proposed a builder-like pattern for the LSPExecutor design. I think this works quite well when you have to support a lot of different use cases, many of which are closely related, and you risk having an explosion of overloaded methods. It also offers a degree of future proofing as further customisation could be added on later without breaking existing code.

The basic usage would go something like

LSPExecutor.forDocment(document).withCapabilities(s -> s.supportsHover())
    .computeAll((w, ls) -> ls.getHover(position));

[ahmedneilhussain]

Addendum: There are quite a
few places in the code where we pass around raw LanguageServer objects - often we receive results from a server call and construct objects, which then receive the LS as a parameter for further calls.

I think that in all of these cases we should be passing around a LanguageServerWrapper rather than the LS directly. As shown in the earlier work, we can make the wrapper the gatekeeper that can execute calls correctly on the dispatcher thread.

This would also obsolete some of the rest of the API: for example there are static methods on LanguageServiceAccessor that works out what the server capabilities are of a server and whether it is running, or what the corresponding server definition might be, by looping through all of the wrappers to see if any matches the supplied server. This only needs to happen because we have as it were thrown away information by using the LS directly rather than the wrapper.

I think much of the same applies to LSPDocumentInfo - in principle it is nice in that it essentially ties together a document and a wrapper. In practice all of the uses I could find basically used it to just call the server via that underlying wrapper, and used ad-hoc logic to aggregate over document infos when it might as well be aggregating over language server [wrapper]s.

Partly to make this kind of thing easier, you will note the proposed api takes a BiFunction that receives both a wrapper as well as the underlying server. In many cases the wrapper argument will be unused, but this is fine, the supplied function will just be something like (wrapper, ls) -> ls.doSomething.
However it can if necessary support

  if (wrapper.getCapabilities().canDoRegionFormatting()) {
   return ls.doRegionFormatting(regionParams);
  else {
  return ls.doWholeDocumentFormatting(wholeDocParams);
}
}

and also

[mickaelistria]

Thanks for this proposal.
So if I get it right, the general idea is to 1. make LSPExecutor the main entry point, and then 2. make LangaugeServerWrapper the main object to manipulate? I think I like this proposal in general.
One thing I find missing in this proposal is a note about what can be deprecated/removed if we start using this new API. Do you think most of LangaugeServerAccessor can be removed or made internal? Do you think some parts of LanguageServerWrapper can also be removed (eg methods returning direct access to the LanguageServer instance), or maybe we can extract the execute/notify methods into an interface and let only those be referenced/consumed in most cases?
I've also added some code comments inline.

[mickaelistria]

Those change would be welcome in a separate patch as they're code improvements independently of the API

[mickaelistria]

I suggest we call is applyEditsToDocumentVersion and document it with a good javadoc. Particularly, it seems important that the behavior in case of incorrect version gets specified; and for this, it could be better to send a check exception instead of a RuntimeException.
This new method (and the other one in this class) + example of consumers would be good candidates for a separate PR IMO, as it's not really re-designing the API but providing a new utility that would be useful independently of other API changes.

[ahmedneilhussain]

You're right - I'm regretting basing this PR off of #251! See my recent comments below - shall I just create a new PR off master with just the core API changes? All the examples and nice-to-have related changes can be submitted in separate PRs.

[mickaelistria]

Can we try to have it an interface instead?
How to get an instance of this?
What about calling it just LanguageServers?

[ahmedneilhussain]

Can we try to have it an interface instead?

I guess, although in this case it makes sense to use subclassing as (a) the bulk of the implementation is common to the Document and Project implementations, and (b) these implementations have some methods that are specific to themselves.

How to get an instance of this?

There are two static factory methods forDocument() and forProject()

What about calling it just LanguageServers?

Sure - not wedded to any particular name!

I think LanguageServers would be good for the static factory methods. For the actual implementation classes LSPDocumentExecutor and LSPProjectExecutor, what would they be called instead?

[mickaelistria]

Do we want something else than "onLatestVersion"? If not, we may just omit the suffix and call it requestLanguageServer and notifiyLanguageServer.
The javadoc shouldn't really mention things about how stuff is dispatch, it should be a bit higher level. Technical comments can be added as @implNote to the Javadoc IIRC.

[ahmedneilhussain]

I agree, the names are confusing, especially if we are making the concurrency guarantees intrinsic to the core API rather than just adding in extra methods with optional extra guarantees. I think I favour execute(fn) and sendNotification(fn) - short and simple; notify is a bit problematic to use on its own as a method name because of Object.notify()!

Maybe the technical notes about the threading design could be a markdown document at the top level?

[ahmedneilhussain]

Thanks for this proposal.
No problem, great that you think an approach along these lines could work!

So if I get it right, the general idea is to 1. make LSPExecutor the main entry point, and then 2. make LangaugeServerWrapper the main object to manipulate?

Exactly - we want to control access to the LS so that people don't subvert the lifecycle/dispatch/ordering guarantees, and also try and make it as easy as possible to do standard things.

One thing I find missing in this proposal is a note about what can be deprecated/removed if we start using this new API. Do you think most of LangaugeServerAccessor can be removed or made internal?

I think pretty much everything in LSA can be removed/made internal. One way of doing this might be to rename all the existing methods Foo as FooImpl, making them package-private. Then restore the public Foo methods but make them @Deprecated and make them just forward on their arguments to the implementation methods. Once the deprecated public methods are removed then the implementation layer can be cleaned up and stripped down to the essentials.

Do you think some parts of LanguageServerWrapper can also be removed (eg methods returning direct access to the LanguageServer instance), or maybe we can extract the execute/notify methods into an interface and let only those be referenced/consumed in most cases?

Certainly an interface might be nice - certainly submitting a call to the server, returning the wrapper status and server capabilities are all high-level API methods that ordinary code should be fine to call, whereas the various low-level methods, particularly the lifecycle stuff, should really be used only by other low-level framework code. Similarly for the handful of miscellaneous but important plugin-level stuff like connect/disconnect content types on LanguageServiceAccessor - needs to be public but shouldn't really be for general use.

One thing I have been pondering is whether to have a SingleExecutor that just wraps a single LanguageServerWrapper - this could then keep LanguageServerWrapper itself as purely low-level framework code to be used only by other low-level code. One to ponder...

[ahmedneilhussain]

I've also added some code comments inline.

Thanks @mickaelistria . I haven't responded in detail just yet because I wanted to try and implement a bit more of the proposed API just to make sure I hadn't written a cheque I couldn't cash, i.e. proposed something that was actually impossible to implement!!

I think naming conventions and interfaces and so on are things we can certainly tidy up if we're happy the approach is going to work.

Please keep the comments coming - what I am going to do next is try porting a few bits of client code over to use it, to see how that looks. The collectAll() method is more or less what I implemented earlier in #251 so I'm happy that can support a good set of existing code; what I'm keen to do is try a variety of usages that don't follow that pattern to see whether the new API accurately captures their needs.

I think so long as we can capture the major use-cases then that's probably adequate: we can cope with a small amount of specialised code that has corner-case needs and so has to combine low-level functions itself in very unusual ways. CommandExecutor might be one such example, but I'll see what emerges!

[rubenporras]

Thanks a lot for the PR. I hope a find more time tomorrow to do an in depth review of all the conversation and the changed code. So far only a partial review.

[rubenporras]

This looks like you need to rebase the PR, I already commit version of it which does the same (and to my taste is nicer because it relies on an incremental document version and not timestamps).

[ahmedneilhussain]

It does! I'm rather regretting basing this PR off of #251 rather than master... It draws upon the earlier work but actually reimplements most of it, so I could probably have started it from scratch off of master.

I'm mostly trying to get comments and feedback on the approach rather than proposing to merge ASAP, but if we think this is a goer then I'll either do a major clean-up or just open a new PR off master with the relevant commits cherry-picked and cleaned up.

[rubenporras]

The LanguageServerWrapper already has a reference to the languageServer, so why pass it on its own? Could we not add a method to the LanguageServerWrapper if we do not want to call languageServerWrapper.getInitializedServer() (is it not just fine to do so, or is the problem that it can side effects?)

[ahmedneilhussain]

Yeah it's a bit ugly. The key thing I'm trying to achieve with the new dispatch logic is to make it possible to have an (internal) dispatch function of the form getServer().thenComposeAsync({code}, dispatcher), with getServer() doing returning a completable future that guarantees that all the initialisation prerequisites will have completed.

The key thing is that we want in the majority of cases getServer() to return a completable future that has already completed, in which case the block {code} will be scheduled straight onto the dispatcher thread, i.e. now from the point of view of the caller. This is critical in making sure most of the client code in LSP4e, which is usually initiated from the UI thread, gets its work scheduled at consistent point in time with respect to any document updates.

I think in this case you're right and it's OK to call getInitializedServer(). The reason I've done this is if you look at the place where this is constructed in LanguageServerWrapper, it's off the back of an initialisation future, so at that point, we are guaranteed to have the LanguageServer ready.

I was just a bit over-paranoid about calling some of the methods that return async objects, just in case I accidentally introduced another possible race condition. It's important that the didOpen message gets sent to the language server before the object returned by LanguageServerWrapper.connect() gets used for any more calls.

lsp4e/org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerWrapper.java

Line 673 in ad55abb

DocumentContentSynchronizer listener = new DocumentContentSynchronizer(this, theDocument, syncKind);

and

lsp4e/org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerWrapper.java

Line 658 in ad55abb

return initializeFuture.thenComposeAsync(theVoid -> {

are critical.

[rubenporras]

I understand that and I think it is good, since you want to be sure ;). I think than better than just passing the LS, would be to have a method that gets you the languageServer as is without doing anything else. Given that getServer already exists but calls getInitializedServer internally, I would propose we create a new method unwrap in LSW that just return languageServer;. What do you think of that?

[ahmedneilhussain]

Yeah, thought about that - I think having package-protected methods like unwrap on LanguageServiceAccessor, LanguageServerWrapper, LSPExecutor and DocumentContentSynchronizer as those are all closely-connected things concerned with the low-level details and can be carefully reviewed to make sure they're sound. So long as the low-level operations are not exposed to the general world, that's very sensible.

[ahmedneilhussain]

@mickaelistria @rubenporras please see my latest couple of checkins - I've ported a couple more bits of code across to see how things look in a wider variety of use-cases. Specifically a project-based executor and also using the computeAll() method.

I've got this afternoon off but next I'll have a go at format, as that's one of the intended uses of computeFIrst()

Cheers, A.

[rubenporras]

We only want one result, e.g. edits from a format request

To be complete, for formatting one could propose the pattern of looping though the servers and do a whole roundtrip when looping. Example if we have two servers s1 and s2.

Request formatting to s1 with the buffer contents => wait for answer to the request => apply format
Request formatting to s2 with the result of the buffer after last apply format => wait for answer to the request => apply format

This is not what we currently do but it would work if the server have orthogonal responsibilities, as if for example s2 would only replaces spaces with tabs, remove trailing whitespaces, setting end lines to UNIX/Windows format or something similar, would work fine.

It might not be relevant to the PR but just to be complete.

[rubenporras]

This would also obsolete some of the rest of the API: for example there are static methods on LanguageServiceAccessor that works out what the server capabilities are of a server and whether it is running, or what the corresponding server definition might be, by looping through all of the wrappers to see if any matches the supplied server. This only needs to happen because we have as it were thrown away information by using the LS directly rather than the wrapper.

I think looping through the servers is still a valid use case it one wants to manage its lifecycle programmatically. That is something we want to do because we want to start / restart different versions of the same LS depending on what our user has selected as his runtime environment. So we need code such as

  private void stopContextLanguageServer(final IProject project) {
    if (LspEnablement.isLspEnabled()) {
      LanguageServerDefinition languageServerDefinition = LanguageServersRegistry.getInstance().getDefinition(CONTEXT_LANGUAGE_SERVER);
      if (languageServerDefinition != null) {
        try {
          boolean haveServerToStop = !LanguageServiceAccessor.getLanguageServers(project, x -> true).isEmpty();
          LanguageServerWrapper languageServerWrapper = LanguageServiceAccessor.getLSWrapper(project, languageServerDefinition);
          if (haveServerToStop && languageServerWrapper.isActive()) {
            /*
             * This should also disconnect any documents connected to the server.
             * They should get automatically reconnected to another one as required.
             */
            languageServerWrapper.stop();
          }
        } catch (IOException e) {
          throw new WrappedException(e);
        }
      }
    }
  }

[rubenporras]

I think much of the same applies to LSPDocumentInfo - in principle it is nice in that it essentially ties together a document and a wrapper. In practice all of the uses I could find basically used it to just call the server via that underlying wrapper, and used ad-hoc logic to aggregate over document infos when it might as well be aggregating over language server [wrapper]s.

I have at least a case where I want to get the LSW and not the server, as in

  private int getDocumentVersion() {
    Iterator<@NonNull LSPDocumentInfo> iterator = LanguageServiceAccessor.getLSPDocumentInfosFor(document, this::hasSemanticTokensFull).iterator();
    if (iterator.hasNext()) {
      return iterator.next().getVersion();
    }
    return -1;
  }

getVersion is going to call LSW#getVersion

[rubenporras]

I would move streamSafely somewhere else, it does not look right on the LanguageServiceAccessor

[rubenporras]

I like a lot that such very long lamdas as factored out as methods, but doing it as part of this PR makes hard to see the actual difference, would you mind pushing a PR with this refactor (and other similar) independently? I think we can merge those quickly and make the diff here more clear

[rubenporras]

I know this is taste, but to me instead of having BiFunctions in the API, it looks more natural having just a Funciton and doing
.collectAll(w -> w.unwrap().getTextDocumentService().hover(params));
the API is simpler to understand and it does not like that the code is more complex or lengthier.

[rubenporras]

this applies to many calls in this PR, but I comment it only here and the one I already did before. I think than having two parameters, even when only one is used usually it is fine, but if you can derive one from the other easily, that looks weird.

[ahmedneilhussain]

Well, the problem is that unwrap would have to be a public method, which is the sort of thing I'm trying to discourage. It would be perfectly ok to call it inside the lambda function, but you wouldn't be able to prevent people just calling LanguageServerWrapper.unwrap().

What about adding overloads to the 3 main methods that take only a single-argument function LanguageServer, for the majority case when that's all you need? I was trying to avoid an explosion of overloads but that would be manageable, especially as they would just do trivial delegation.

[rubenporras]

I thought about that, but it is also not ideal.

What about actually offering the methods of LanguageServer in the LanguageServerWrapper and just implement them by delegating to the LanguageServer? That is delegating only seven methods, so it is straightforward and would mean that having the wrapper is as good as having the sever handle.

[ahmedneilhussain]

I've ported the formatting code to the new API 4963536

just as a proof-of-concept so that I could test that the computeFirst() method actually worked and see how it looked in practice.

Sorry that it's a bit messy - I had to leave the old code intact (but bypassed) as I didn't want to have to fix up the tests just for a PoC.

I have at least tested it with the Corrosion/Rust plugin and it works, at least for the happy case!

[ahmedneilhussain]

It's not strictly needed here but I also quickly sketched out 1957c25 to show that the related 'optimistic locking' problem can have direct API support - we make the executor track the document timestamp when it dispatches a request, and then add a supporting method to turn the response into a VersionedEdit.

@rubenporras this might particularly interest you, since you implemented the PR for formatting version-check.

[ahmedneilhussain]

I have at least a case where I want to get the LSW and not the server, as in

  private int getDocumentVersion() {
    Iterator<@NonNull LSPDocumentInfo> iterator = LanguageServiceAccessor.getLSPDocumentInfosFor(document, this::hasSemanticTokensFull).iterator();
    if (iterator.hasNext()) {
      return iterator.next().getVersion();
    }
    return -1;
  }

getVersion is going to call LSW#getVersion

@rubenporras see 1957c25 - I think we can actually offer a higher-level support for optimistic locking elegantly, in which case LSW#getVersion is not needed?

[ahmedneilhussain]

I think looping through the servers is still a valid use case it one wants to manage its lifecycle programmatically. That is something we want to do because we want to start / restart different versions of the same LS depending on what our user has selected as his runtime environment.

Absolutely, the low-level lifecycle methods are still needed, but I think only rare/specialized code will need access.

I think for that reason I should maybe not make LanguageServerWrapper the main access point but instead put it inside an interface IWrapper or ISingleServerExecutor that only exposes execute(fn) and getServerCapabilities() and maybe a couple of other things. What do you reckon? @mickaelistria ?

[ahmedneilhussain]

Hi guys, I've now done all of my PoC work in porting example code to use the API. Sorry that it has meant this PR is not as tightly-focused as it maybe should have been. However you can only really see whether an API works when you try and make use of it!

I think I'm happy that this API will cover the core usage of the plugin. I think maybe the next step is to open another PR with just the core API implementation and none of the ported examples nor dead baggage from #251 . Then we can establish the names and method signatures and the way we package things up into interfaces.

@mickaelistria @rubenporras what do you think? Since we're proposing to change the core way in which the majority of the code accesses the LS I think the API needs wide buy-in. Is there anyone in particular I should consult @mickaelistria ? Perhaps some of the core committers?

[rubenporras]

@rubenporras see 1957c25 - I think we can actually offer a higher-level support for optimistic locking elegantly, in which case LSW#getVersion is not needed?

Sure, if we can do better, then I am happy. I am a bit afraid we want to do too much at once and we might loose ourselves in discussions, but we well see ;)

[mickaelistria]

@ahmedneilhussain Is it still relevant since we merged #344 ?

[ahmedneilhussain]

Now superseded by the real thing!