search

eclipse-shellwax / shellwax

Shell script editor plugin for Eclipse
https://marketplace.eclipse.org/content/shellwax
Eclipse Public License 2.0
17 stars 12 forks source link

Fix for the Bash LS installation and startup #55

Closed vrubezhny closed 3 years ago

vrubezhny commented 3 years ago

The change in BashLanguageServer (Shellwax) fixes the multiple bash LS installation job creation and invocation. A single static instance of CompletableFuture is used to create and run a runnable for the Bash Language Server module installation. When start() API method is invoked it waits for this CompletableFuture to be resolved (which means the installation is finished) then starts the server.

This PR works best with this change https://git.eclipse.org/r/c/lsp4e/lsp4e/+/173719 to be applied on LSP4E, but doesn't require it for the normal functionality.

Signed-off-by: Victor Rubezhny vrubezhny@redhat.com

mickaelistria commented 3 years ago

This looks like a complex hack/workaround. I'd like to use the issue as an opportunity to figure out whether LSP4E API can be improved. 1 first quesiton is "why isn't the install stuff part of the start method?"

vrubezhny commented 3 years ago

1 first quesiton is "why isn't the install stuff part of the start method?"

We probably should ask @akurtakov, but as far as I understand it's placed here in order to start the installation as much earlier as it's possible. I just kept this code where it was placed before. Moving it to start method will not make any profit here: as the installation is a quite long procedure we have to execute it in a job, so 'start' method will still be finished before the installation is completed and, as such, the server will be restarted several times before it really start working.

mickaelistria commented 3 years ago

So what about: the installation is done in a Job, and the LS start method invokes job.join()? That way, we have the installation started eagerly, and the LS properly waiting for completion before starting, so we should avoid many issues.

vrubezhny commented 3 years ago

So what about: the installation is done in a Job, and the LS start method invokes job.join()? That way, we have the installation started eagerly, and the LS properly waiting for completion before starting, so we should avoid many issues.

Then we go back to having a number of errors in attempt to start the LS, like:

internal/modules/cjs/loader.js:985
  throw err;
  ^

Error: Cannot find module '/home/jeremy/.local/share/shellwax/1.16.1/node_modules/.bin/bash-language-server'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:982:15)
    at Function.Module._load (internal/modules/cjs/loader.js:864:27)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:74:12)
    at internal/main/run_main_module.js:18:47 {
  code: 'MODULE_NOT_FOUND',
  requireStack: []
}
Dec 06, 2020 3:30:24 PM org.eclipse.lsp4j.jsonrpc.RemoteEndpoint notify
WARNING: Failed to send notification message.
org.eclipse.lsp4j.jsonrpc.JsonRpcException: java.io.IOException: Stream closed
    at org.eclipse.lsp4j.jsonrpc.json.StreamMessageConsumer.consume(StreamMessageConsumer.java:72)
    at org.eclipse.lsp4e.LanguageServerWrapper.lambda$1(LanguageServerWrapper.java:243)
    at org.eclipse.lsp4j.jsonrpc.RemoteEndpoint.notify(RemoteEndpoint.java:126)
    at org.eclipse.lsp4j.jsonrpc.services.EndpointProxy.invoke(EndpointProxy.java:88)
    at com.sun.proxy.$Proxy30.exit(Unknown Source)
    at org.eclipse.lsp4e.LanguageServerWrapper.lambda$5(LanguageServerWrapper.java:420)
    at java.base/java.util.concurrent.CompletableFuture$AsyncRun.run(CompletableFuture.java:1736)
    at java.base/java.util.concurrent.CompletableFuture$AsyncRun.exec(CompletableFuture.java:1728)
    at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:290)
    at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1020)
    at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1656)
    at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1594)
    at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:183)
Caused by: java.io.IOException: Stream closed
    at java.base/java.lang.ProcessBuilder$NullOutputStream.write(ProcessBuilder.java:442)
    at java.base/java.io.OutputStream.write(OutputStream.java:157)
    at java.base/java.io.BufferedOutputStream.flushBuffer(BufferedOutputStream.java:81)
    at java.base/java.io.BufferedOutputStream.flush(BufferedOutputStream.java:142)
    at org.eclipse.lsp4j.jsonrpc.json.StreamMessageConsumer.consume(StreamMessageConsumer.java:69)
    ... 12 more

... as well as some other exceptions:

!ENTRY org.eclipse.lsp4e 4 0 2020-12-06 15:30:24.719
!MESSAGE java.util.concurrent.CancellationException
!STACK 0
java.util.concurrent.ExecutionException: java.util.concurrent.CancellationException
    at java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:395)
    at java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2022)
    at org.eclipse.lsp4e.LanguageServerWrapper.getServerCapabilities(LanguageServerWrapper.java:699)
    at org.eclipse.lsp4e.LanguageServiceAccessor.lambda$15(LanguageServiceAccessor.java:590)
    at java.base/java.util.concurrent.CompletableFuture$UniCompose.tryFire(CompletableFuture.java:1072)
    at java.base/java.util.concurrent.CompletableFuture$Completion.exec(CompletableFuture.java:479)
    at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:290)
    at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1020)
    at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1656)
    at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1594)
    at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:183)
Caused by: java.util.concurrent.CancellationException
    at java.base/java.util.concurrent.CompletableFuture.cancel(CompletableFuture.java:2396)
    at org.eclipse.lsp4e.LanguageServerWrapper.stop(LanguageServerWrapper.java:394)
    at org.eclipse.lsp4e.LanguageServerWrapper.start(LanguageServerWrapper.java:203)
    at org.eclipse.lsp4e.LanguageServerWrapper.getInitializedServer(LanguageServerWrapper.java:667)
    at org.eclipse.lsp4e.LanguageServiceAccessor.lambda$14(LanguageServiceAccessor.java:589)
    at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195)
    at java.base/java.util.Iterator.forEachRemaining(Iterator.java:133)
    at java.base/java.util.Spliterators$IteratorSpliterator.forEachRemaining(Spliterators.java:1801)
    at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)
    at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
    at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:550)
    at java.base/java.util.stream.AbstractPipeline.evaluateToArrayNode(AbstractPipeline.java:260)
    at java.base/java.util.stream.ReferencePipeline.toArray(ReferencePipeline.java:517)
    at org.eclipse.lsp4e.LanguageServiceAccessor.getLanguageServers(LanguageServiceAccessor.java:602)
    at org.eclipse.lsp4e.operations.folding.LSPFoldingReconcilingStrategy.reconcile(LSPFoldingReconcilingStrategy.java:128)
    at org.eclipse.lsp4e.operations.folding.LSPFoldingReconcilingStrategy.initialReconcile(LSPFoldingReconcilingStrategy.java:326)
    at org.eclipse.jface.text.reconciler.MonoReconciler.initialProcess(MonoReconciler.java:98)
    at org.eclipse.jface.text.reconciler.AbstractReconciler$BackgroundThread.run(AbstractReconciler.java:177)

!ENTRY org.eclipse.jface.text 4 0 2020-12-06 15:30:24.724
!MESSAGE java.lang.NullPointerException
!STACK 0
java.util.concurrent.CompletionException: java.lang.NullPointerException
    at java.base/java.util.concurrent.CompletableFuture.encodeThrowable(CompletableFuture.java:314)
    at java.base/java.util.concurrent.CompletableFuture.completeThrowable(CompletableFuture.java:319)
    at java.base/java.util.concurrent.CompletableFuture$UniCompose.tryFire(CompletableFuture.java:1081)
    at java.base/java.util.concurrent.CompletableFuture$Completion.exec(CompletableFuture.java:479)
    at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:290)
    at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1020)
    at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1656)
    at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1594)
    at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:183)
Caused by: java.lang.NullPointerException
    at org.eclipse.lsp4e.operations.codelens.CodeLensProvider.lambda$0(CodeLensProvider.java:41)
    at org.eclipse.lsp4e.LanguageServiceAccessor.lambda$15(LanguageServiceAccessor.java:590)
    at java.base/java.util.concurrent.CompletableFuture$UniCompose.tryFire(CompletableFuture.java:1072)
    ... 6 more

And even worse, because the Eclipse becomes not responsive until the installation is finished. The problem is that LanguageServerWrapper (or something behind it) cancels the start() (by timeout probably) and then tries to restart the server, so we repeatedly go into that job.joing() inside start() method which makes everything not responsive and throwing quite the same amount of error messages.

A bad thing here is that we have no anything like 'skip server start' like API to be used in a situation of a long server start procedure (or at least me cannot find such an API)

mickaelistria commented 3 years ago 
internal/modules/cjs/loader.js:985
  throw err;
  ^

Error: Cannot find module '/home/jeremy/.local/share/shellwax/1.16.1/node_modules/.bin/bash-language-server'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:982:15)
    at Function.Module._load (internal/modules/cjs/loader.js:864:27)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:74:12)
    at internal/main/run_main_module.js:18:47 {
  code: 'MODULE_NOT_FOUND',
  requireStack: []
}

I don't get why this are happening at this point. The node_modules server is supposed to be available at that time, isn't it? If it's not available, did the installation job complete properly?

I believe the issues you see for LSP4E are a bug in LSP4E about the start() method checking for isActive(), but not properly checking for some isActivating() state (ie StreamConnectionProvider.start() is being called). It would be something to improve in LSP4E directly.

vrubezhny commented 3 years ago

I don't get why this are happening at this point. The node_modules server is supposed to be available at that time, isn't it? If it's not available, did the installation job complete properly?

No. No server is installed at this point and installation job isn't finished yet. But calls to LanguageServerWrapper.getInitializedServer() keep going (and sequentially lead to new calls of start() of LS). LSP4E is not going to wait for the job finishing (with job.join()) - so in few seconds it cancels the LS startup and repeats starting the LS on a next request. We cannot just call job.joing()' and wait infinitely for the install job finish - the server will be killed (cancelled) and recreated at nextgetInitializedServer()` call - and all the things get repeated, so joining the job and waiting on it actually doesn't make any sense.

mickaelistria commented 3 years ago

so in few seconds it cancels the LS startup and repeats starting the LS on a next request.

That seems like something we need to fix in LSP4E. LSP4E should decently handle long server startups.

vrubezhny commented 3 years ago

I've updated the fix. Now start() blocks until the installation is finished (if any), so it requires this 'long' initialization to be supported by LSP4E. This change aims to add such support to LSP4E: https://git.eclipse.org/r/c/lsp4e/lsp4e/+/173719

vrubezhny commented 3 years ago

BTW, If I change the timeout values in 'LSPTextHoverand inLSIncompleteCompletionProposal` the hovering and content assisting do not generate any TimeOut exceptions at all. Maybe there is a reason to make it possible to configure these TimeOut values somehow for a particular LS?

mickaelistria commented 3 years ago

TimeOut exceptions

Please report issues to LSP4E about those exceptions. We need LSP4E to be smarter in that case.

possible to configure these TimeOut values

we won't work on that, as it's more a workaround. For cases where we have some timeout in LSP4E, we need to investigate ways to allow not waiting for the results, making consumers more "reactive". It usually means we need to adapt some extension points in Platform.

vrubezhny commented 3 years ago

TimeOut exceptions

Please report issues to LSP4E about those exceptions. We need LSP4E to be smarter in that case.

possible to configure these TimeOut values

we won't work on that, as it's more a workaround. For cases where we have some timeout in LSP4E, we need to investigate ways to allow not waiting for the results, making consumers more "reactive". It usually means we need to adapt some extension points in Platform.

These TimeOut exceptions are already reported in a bunch of LSP4E issues like: https://bugs.eclipse.org/bugs/show_bug.cgi?id=561372 - DocumentLinkDetector timeout too long https://bugs.eclipse.org/bugs/show_bug.cgi?id=569714 - UI freeze rooting to LanguageServerWrapper.supportsWorkspaceFolderCapability https://bugs.eclipse.org/bugs/show_bug.cgi?id=569372 - TimoutException on HyperlinkDetector, especially when just opening the file

as well as in some other projects, like Platform Text content assist related issues.

I don't know if we need to report yet another one to this list - a;though it's not hyperlinking related, however it is of the same nature and is to be solved in the same manner as the previous ones:

!ENTRY org.eclipse.lsp4e 4 0 2020-12-23 15:46:24.982
!MESSAGE 
!STACK 0
java.util.concurrent.TimeoutException
    at java.base/java.util.concurrent.CompletableFuture.timedGet(CompletableFuture.java:1886)
    at java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2021)
    at org.eclipse.lsp4e.operations.completion.LSIncompleteCompletionProposal.getAdditionalProposalInfo(LSIncompleteCompletionProposal.java:308)
    at org.eclipse.lsp4e.operations.completion.LSIncompleteCompletionProposal.getAdditionalProposalInfo(LSIncompleteCompletionProposal.java:1)
    at org.eclipse.jface.text.contentassist.AdditionalInfoController$Timer$2$1.run(AdditionalInfoController.java:116)
    at org.eclipse.core.internal.jobs.Worker.run(Worker.java:63)