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

Unwrap InvocationTargetException which are ResponseErrorException #809

Closed jonahgraham closed 4 months ago

jonahgraham commented 4 months ago

By wrapping ResponseErrorException with another layer in RuntimeException it means the unwrapping of the exception in RemoteEndpoint.DEFAULT_EXCEPTION_HANDLER doesn't work as expected and instead of getting the original ResponseError the other end gets the fallback ResponseError of type internal error.

This change includes updates to documentation to show that it is ok to throw ResponseErrorException (reverts https://github.com/eclipse-lsp4j/lsp4j/pull/578). While it is also OK to return an exceptionally completed future, it is not needed to do that way and simply throwing is more straightforward.

There are new tests to cover the changed handling. Also tested is the previously documented way of handling exceptions (see tries == 1 case in testVersatility and testVersatilityResponseError

Fixes https://github.com/eclipse-lsp4j/lsp4j/issues/802

jonahgraham commented 4 months ago

I updated docs and updated tests. New commit was used to update description of the PR

jonahgraham commented 4 months ago

There is one todo thing left, hence turned to draft, the code change when it is delegate segments has no test coverage. Probably also worth adding to changelog.

jonahgraham commented 4 months ago

There is one todo thing left, hence turned to draft, the code change when it is delegate segments has no test coverage. Probably also worth adding to changelog.

Newest commit adds to the changelog. It also adds the code coverage as the find delegate's change I had done previously was incorrect.

pisv commented 4 months ago

The 'sneaky throws' change is ready for review: https://github.com/eclipse-lsp4j/lsp4j/pull/809/commits/4de67343c5a30bf9110ace2969c832708e06e1df :-)

pisv commented 4 months ago

I have another approach in the works, which does not use sneaky throws. It is based on the fact that an Endpoint is not expected to throw a checked exception. Therefore, throwing a checked exception from an annotated jsonrpc method can (and should) be treated as a programming error, and can be signalled via an IllegalStateException in the GenericEndpoint. An inaccessible jsonrpc method (IllegalAccessException) can also be handled in a similar way. There is no need to use sneaky throws or a CompletionException in this case. Runtime exceptions or errors would just be re-thrown.

pisv commented 4 months ago

The latest change is ready for review: https://github.com/eclipse-lsp4j/lsp4j/pull/809/commits/3982fd3ba22c2ae71cebafdc3647dbfd347ff770.

jonahgraham commented 4 months ago

@ivy-cst this change should end up on snapshot builds soon: https://github.com/eclipse-lsp4j/lsp4j?tab=readme-ov-file#snapshots

Please comment in #807 if you have input on if/when a full release will help your adoption so that I can consider how and when to make a release.

ivy-cst commented 4 months ago

@ivy-cst this change should end up on snapshot builds soon: https://github.com/eclipse-lsp4j/lsp4j?tab=readme-ov-file#snapshots

Please comment in #807 if you have input on if/when a full release will help your adoption so that I can consider how and when to make a release.

Thank you for the fix. We do not need a release. We are currently living with our own exception handler. We are tied to the eclipse rcp releases at the moment anyway.

jonahgraham commented 4 months ago

OK thanks for responding - if that changes please comment on #807

PS I really appreciate that you followed up on this issue, especially knowing that it did not affect your code in the short term. It will now make everyone's code in the future better and is part of what makes me very happy to be working on open source projects with a vibrant community!