Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ResponseErrorException is not propagated with the default exception handler #802

Closed
ivy-cst opened this issue Feb 13, 2024 · 3 comments · Fixed by #809
Closed

ResponseErrorException is not propagated with the default exception handler #802

ivy-cst opened this issue Feb 13, 2024 · 3 comments · Fixed by #809

Comments

@ivy-cst
Copy link

ivy-cst commented Feb 13, 2024 •
edited
Loading

The same as: #576
But by then only the doc had changed: #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
Copy link
Contributor

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?

jonahgraham added a commit that referenced this issue Feb 14, 2024
@jonahgraham
Unwrap InvocationTargetException which are ResponseErrorException
d2925a4 
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.

Fixes #802
@jonahgraham jonahgraham mentioned this issue Feb 14, 2024
Merged
jonahgraham added a commit that referenced this issue Feb 14, 2024
@jonahgraham
Unwrap InvocationTargetException which are ResponseErrorException
4c49a57 
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
@ivy-cst
Copy link
Author

ivy-cst commented Feb 14, 2024

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:

lsp4j/org.eclipse.lsp4j.jsonrpc/src/main/java/org/eclipse/lsp4j/jsonrpc/RemoteEndpoint.java

Line 62 in 88868d3

LOG.log(Level.SEVERE, header + ": " + throwable.getMessage(), throwable);

@jonahgraham
Copy link
Contributor

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.

jonahgraham added a commit that referenced this issue Feb 20, 2024
@jonahgraham
Unwrap InvocationTargetException which are ResponseErrorException
aa092c0 
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

Also-by: Vladimir Piskarev <pisv@1c.ru>
jonahgraham added a commit that referenced this issue Feb 20, 2024
@jonahgraham
Use more specific exception when @JsonDelegates throw an exception
aa0bb9e 
This exception is thrown when the endpoint is created, not as a result of
a receiving a message. This is generally a coding error, so make this
exception the same as the other exceptions thrown by recursiveFindRpcMethods.

Included is adding additional tests to ensure coverage of some
previously missing uses cases.

Code cleanup done as part of #802
jonahgraham added a commit that referenced this issue Feb 29, 2024
@jonahgraham
Unwrap InvocationTargetException which are ResponseErrorException
c3b1b60 
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

Also-by: Vladimir Piskarev <pisv@1c.ru>
jonahgraham added a commit that referenced this issue Feb 29, 2024
@jonahgraham
Use more specific exception when @JsonDelegates throw an exception
b76bb7e 
This exception is thrown when the endpoint is created, not as a result of
a receiving a message. This is generally a coding error, so make this
exception the same as the other exceptions thrown by recursiveFindRpcMethods.

Included is adding additional tests to ensure coverage of some
previously missing uses cases.

Code cleanup done as part of #802
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Unwrap InvocationTargetException which are ResponseErrorException eclipse-lsp4j/lsp4j
2 participants
@jonahgraham @ivy-cst