Rework shutdown/exit requests #55
Conversation
As suggested by the `lsp4j` team: eclipse-lsp4j/lsp4j#770
I couldn't manage to make a working mock of `RalphLangClient`, as we need the `logMessage` to be implemented, so I created an `EmptyRalphLangClient` instead
| thisServer.synchronized { | ||
| logger.info("shutdown") | ||
| if(thisServer.state.shutdownReceived){ | ||
| notifyAndThrow(ResponseError.ShutdownRequested) |
There was a problem hiding this comment.
Just a question for my knowledge - Since this throws an Exception, the client that invoked shutdown() is still waiting a response right? Because CompletableFuture remains un-responded. Should a failed CompletableFuture be returned instead?
CompletableFuture.failedFuture(ResponseError.ShutdownRequested.toResponseErrorException)Another question: Since the protocol here say
If a server receives requests after a shutdown request those requests should error with
InvalidRequest.
Should all other requests (didOpen, didChange, didClose, didSave, didChangeWatchedFiles etc) also respond with InvalidRequest when a shutdown in progress?
There was a problem hiding this comment.
oh nice catch, we shouldn't throw indeed. I don't event think we need to complete with a failure, we maybe need to simply complete the error:
CompletableFuture.completedFuture(ResponseError.ShutdownRequested)
I'll check.
Regarding the second question I think so yes, it seems every request should get rejected in case of shutdown. I'll create an issue
There was a problem hiding this comment.
so yes throwing isn't a good idea 😅
There was a problem hiding this comment.
actually all the did* functions are notifications, not requests, so we don't need to answer with InvalidRequest, We could just do nothing from our side.
We need to reply on things like: Completion Request, currently we only need to add this to the Initialize request
There was a problem hiding this comment.
and actually you were right, they recommend to fail the future, but with completeExceptionnally: https://github.com/eclipse-lsp4j/lsp4j/blob/3c98376797b96922689af963b8403431135f0f35/documentation/jsonrpc.md?plain=1#L36-L56
Then the json rpc server handle it and return an error to the client
lsp-server/src/main/scala/org/alephium/ralph/lsp/server/RalphLangServer.scala
Outdated
Show resolved
Hide resolved
lsp-server/src/main/scala/org/alephium/ralph/lsp/server/state/ServerState.scala
Outdated
Show resolved
Hide resolved
lsp-server/src/main/scala/org/alephium/ralph/lsp/server/ResponseError.scala
Outdated
Show resolved
Hide resolved
…angServer.scala Co-authored-by: Simer <simer.j@gmail.com>
Co-authored-by: Simer <simer.j@gmail.com>
We shouldn't throw the error, as the app will then stop and the client will wait forever an answered. As defined in the protocol, we should respond with the error to the client
Resolves: #37
@simerplaha I think we can have a review.
Base on eclipse-lsp4j/lsp4j#770 it's better to cancel the listener on
exitAs explained in the issue, I tried to follow the lsp specs
Note about tests: every time I try to use
scalamockI fail, I tried all possibility I could find to mock the client and mock thelogMessagefunction, but it always failed, so I went with anEmpytRalphLangClient, don't hesitate to give it a try