search

eclipse-lsp4j / lsp4j

A Java implementation of the language server protocol intended to be consumed by tools and language servers implemented in Java.
https://eclipse.org/lsp4j
Other
581 stars 141 forks source link

ResponseErrorException is not propagated with the default exception handler #802

Closed ivy-cst closed 4 months ago

ivy-cst commented 4 months ago

The same as: https://github.com/eclipse-lsp4j/lsp4j/issues/576 But by then only the doc had changed: https://github.com/eclipse-lsp4j/lsp4j/pull/578

In my opinion this is not very user friendly. If I bother to throw a ResponseErrorException, it should be propagated even if it is thrown when the method is called and not only when the CompletableFuture is executed.

As an example, we do check some context stuff that is only available as a ThreadLocal and not while the CompletableFuture is executed.

There is also an open source example on eclpse-gslp that does this incorrectly. See DefaultGLSPServer

jonahgraham commented 4 months ago

Can I confirm that the use case you are seeing is that you are getting the "wrong" ResponseError back? Instead of getting the ResponseError with the code, message and data that you expect you are getting an Internal error message/code and the data is the stack trace?

ivy-cst commented 4 months ago

Can I confirm that the use case you are seeing is that you are getting the "wrong" ResponseError back? Instead of getting the ResponseError with the code, message and data that you expect you are getting an Internal error message/code and the data is the stack trace?

Yes a new ResponseError is generated and even worse the exception is logged as SEVERE here: https://github.com/eclipse-lsp4j/lsp4j/blob/88868d3506d317b12cf0bfd1e82a254489f8c47b/org.eclipse.lsp4j.jsonrpc/src/main/java/org/eclipse/lsp4j/jsonrpc/RemoteEndpoint.java#L62

jonahgraham commented 4 months ago

Yes a new ResponseError is generated [...]

Thanks for confirming. I think that the PR I provided will resolve this case. Thanks for pointing out the secondary problem of the SEVERE log message too.