Unwrap InvocationTargetException which are ResponseErrorException

[jonahgraham]

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 #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 #802

[pisv]

@jonahgraham This looks like a nice improvement indeed, thank you! 👍

It seems that the documentation update in #578 needs to be reverted or amended now, either as part of this PR or a follow-up.

[jonahgraham]

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

[jonahgraham]

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]

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.

[ivy-cst]

Looks good, thanks for the fix 👍

[pisv]

The 'sneaky throws' change is ready for review: 4de6734 :-)

[pisv]

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]

The latest change is ready for review: 3982fd3.

[jonahgraham]

(Silly GitHub - I can't approve my own PR even though I am reviewing @pisv's latest change)

+1 on this change.

[pisv]

Given that @jonahgraham has approved my latest change, this PR is complete and ready to be merged, as far as I can tell.

Thank you @jonahgraham and @ivy-cst!

[jonahgraham]

@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]

@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]

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!