Maintenance: Tidy up server lifecycle

[ahmedneilhussain]

Further tweaks to server lifecycle code to make it a little simpler to follow.

Avoid mutating initializeFuture from within blocks chained onto initializeFuture itself. There is no need to e.g. call initializeFuture.completeExceptionally from within an exceptionallyBlock chained onto initializeFuture! If that block is hit the future is already in a 'completed, exceptionally' state.

[ahmedneilhussain]

The latest commit splits a lot of the lifecycle code out of LanguageServerWrapper. It's not as drastic a change as it looks, as it mostly involves just moving the low-level code in chained futures out of LanguageServerWrapper.start() into a separate class, and the moved code is more or less intact. I wanted to have a few smaller commits and keep the (small) logic changes separate from the pure refactoring, but that wasn't possible :-(

Rationale:

1. Currently, LanguageServerWrapper has a lot of state fields, each managing different aspects of the server connection.
2. The fields are initialised at different low-level stages in the startup, and cleared individually (and sometimes haphazardly) when stopping. This adds a lot of possible partly-started states where some calls will succeed and others won't.
3. Bundling the state together makes it easier to have the server atomically available or not, so clearing up partial-state is handled outside LanguageServerWrapper.
4. Generally I think we should avoid or at least minimise using completable futures for side effects rather than to compute a result.

Caveats:

1. I have a connection field as well as the intializeFuture. This is really redundant but it was the least intrusive change to the existing code: it means code can simply test for null rather than dereference the future and have a load of exception-handling boilerplate.
2. There is some state left on LanguageServerWrapper. Possibly it should be moved across to ServerConnection but there is at least a reasonably clean split: all the LSP4j stuff is on ServerConnection whereas the platform objects (listeners and so on) live on LanguageServerWrapper.
3. I think the code is probably more robust (or at least easy to reason about) wrt possible interleavings of start() and stop(), but I'm still not sure it's bulletproof. In essence start() and stop() are synchronised methods so can't be called simultaneously, but they each trigger asynchronous operations (essentially 'starting' and 'stopping' states). I think the calls to ConnectionFactory.checkCancelled() in combination with the cancellation/cleanup code mean that calling stop while starting or start while stopping should be OK.

[ahmedneilhussain]

@rubenporras @mickaelistria any general thoughts? Need to be very careful making changes to this logic, so I definitely don't think we should be in a hurry to merge any of this. On the other hand, the existing code has evolved quite a lot to the point where it is very hard to reason about the lifecycle, so I think sooner or later it would be good to bite the bullet and try and rationalise things.

Would appreciate thoughts on this as a general approach.

[ahmedneilhussain]

Just fixed an issue with workspace folders. Of interest because it is related to the discussion about lifecycle and LanguageServerWrapper reuse (or not) in #512

Two observations come to mind: we seem to be interpreting the 'workspace folders' capability slightly differently to the spec. AIUI, workspace folders are supported at basic level either as a yes or a no, and that doesn't change dynamically. The feature that can be added dynamically is subscription to the workspaceFolders/didChange event. That's not the same thing as pure workspaceFolders support - the latter is just the server's ability to send a request to the client asking for a list of workspace folders.

Secondly - what are we gaining from keeping wrappers around when the server is shut down?

[mickaelistria]

we seem to be interpreting the 'workspace folders' capability slightly differently to the spec.

Can you please open a dedicated issue for that?

Secondly - what are we gaining from keeping wrappers around when the server is shut down?

In case a LS instance or connection dies, then the wrapper is still supposed to be partly usable and to revive the LS. Usually, the LS keeps the same capabilities and so on, so we can keep using the wrapper to select the language servers that we will query for an operation or another even if this LS has died in the meantime.

[mickaelistria]

The initial target branch master was deleted and replaced by main, so this PR got closed automatically. If this is still relevant, please re-create this PR targetting the main branch.