From 4c49a5780204fe202319413d80771f7c304c905d Mon Sep 17 00:00:00 2001 From: Jonah Graham Date: Wed, 14 Feb 2024 10:29:58 -0500 Subject: [PATCH] Unwrap InvocationTargetException which are ResponseErrorException 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 --- documentation/jsonrpc.md | 8 +- .../jsonrpc/services/GenericEndpoint.java | 16 ++- .../lsp4j/jsonrpc/test/IntegrationTest.java | 106 +++++++++++++++++- .../jsonrpc/test/RemoteEndpointTest.java | 69 ++++++++++++ 4 files changed, 191 insertions(+), 8 deletions(-) diff --git a/documentation/jsonrpc.md b/documentation/jsonrpc.md index 374be45e..49dd67d0 100644 --- a/documentation/jsonrpc.md +++ b/documentation/jsonrpc.md @@ -37,7 +37,7 @@ For the other direction, if the implementation calls request on the RemoteEndpoi The receiver of a request always needs to return a response message to conform to the JSON-RPC specification. In case the result value cannot be provided in a response because of an error, the `error` property of the `ResponseMessage` must be set to a `ResponseError` describing the failure. -This can be done by returning a `CompletableFuture` completed exceptionally with a `ResponseErrorException` from the request message handler in a local endpoint. The exception carries a `ResponseError` to attach to the response. The `RemoteEndpoint` will handle the exceptionally completed future and send a response message with the attached error object. +This can be done by throwing a `ResponseErrorException` from the request message handler in a local endpoint. The exception carries a `ResponseError` to attach to the response. The `RemoteEndpoint` will handle the exception and send a response message with the attached error object. For example: @@ -45,10 +45,8 @@ For example: @Override public CompletableFuture shutdown() { if (!isInitialized()) { - CompletableFuture exceptionalResult = new CompletableFuture<>(); - ResponseError error = new ResponseError(ResponseErrorCode.ServerNotInitialized, "Server was not initialized", null); - exceptionalResult.completeExceptionally(new ResponseErrorException(error)); - return exceptionalResult; + ResponseError error = new ResponseError(ResponseErrorCode.serverNotInitialized, "Server was not initialized", null); + throw new ResponseErrorException(error); } return doShutdown(); } diff --git a/org.eclipse.lsp4j.jsonrpc/src/main/java/org/eclipse/lsp4j/jsonrpc/services/GenericEndpoint.java b/org.eclipse.lsp4j.jsonrpc/src/main/java/org/eclipse/lsp4j/jsonrpc/services/GenericEndpoint.java index 0422480f..0c06a2eb 100644 --- a/org.eclipse.lsp4j.jsonrpc/src/main/java/org/eclipse/lsp4j/jsonrpc/services/GenericEndpoint.java +++ b/org.eclipse.lsp4j.jsonrpc/src/main/java/org/eclipse/lsp4j/jsonrpc/services/GenericEndpoint.java @@ -63,7 +63,13 @@ protected void recursiveFindRpcMethods(Object current, Set> visited, Se Method method = methodInfo.method; Object[] arguments = this.getArguments(method, arg); return (CompletableFuture) method.invoke(current, arguments); - } catch (InvocationTargetException | IllegalAccessException e) { + } catch (InvocationTargetException e) { + if (e.getTargetException() instanceof ResponseErrorException) { + ResponseErrorException ree = (ResponseErrorException) e.getTargetException(); + throw ree; + } + throw new RuntimeException(e); + } catch ( IllegalAccessException e) { throw new RuntimeException(e); } }; @@ -79,7 +85,13 @@ protected void recursiveFindRpcMethods(Object current, Set> visited, Se } else { LOG.fine("A delegate object is null, jsonrpc methods of '" + method + "' are ignored"); } - } catch (InvocationTargetException | IllegalAccessException e) { + } catch (InvocationTargetException e) { + if (e.getTargetException() instanceof ResponseErrorException) { + ResponseErrorException ree = (ResponseErrorException) e.getTargetException(); + throw ree; + } + throw new RuntimeException(e); + } catch ( IllegalAccessException e) { throw new RuntimeException(e); } }); diff --git a/org.eclipse.lsp4j.jsonrpc/src/test/java/org/eclipse/lsp4j/jsonrpc/test/IntegrationTest.java b/org.eclipse.lsp4j.jsonrpc/src/test/java/org/eclipse/lsp4j/jsonrpc/test/IntegrationTest.java index 82f28f7a..f39ad349 100644 --- a/org.eclipse.lsp4j.jsonrpc/src/test/java/org/eclipse/lsp4j/jsonrpc/test/IntegrationTest.java +++ b/org.eclipse.lsp4j.jsonrpc/src/test/java/org/eclipse/lsp4j/jsonrpc/test/IntegrationTest.java @@ -34,6 +34,8 @@ import org.eclipse.lsp4j.jsonrpc.ResponseErrorException; import org.eclipse.lsp4j.jsonrpc.json.StreamMessageProducer; import org.eclipse.lsp4j.jsonrpc.messages.Either; +import org.eclipse.lsp4j.jsonrpc.messages.ResponseError; +import org.eclipse.lsp4j.jsonrpc.messages.ResponseErrorCode; import org.eclipse.lsp4j.jsonrpc.services.GenericEndpoint; import org.eclipse.lsp4j.jsonrpc.services.JsonNotification; import org.eclipse.lsp4j.jsonrpc.services.JsonRequest; @@ -420,8 +422,14 @@ public CompletableFuture askClient(MyParam param) { tries++; throw new UnsupportedOperationException(); } + if (tries == 1) { + tries++; + CompletableFuture future = new CompletableFuture<>(); + future.completeExceptionally(new UnsupportedOperationException()); + return future; + } return CompletableFutures.computeAsync(executor, cancelToken -> { - if (tries++ == 1) + if (tries++ == 2) throw new UnsupportedOperationException(); return param; }); @@ -436,6 +444,7 @@ public CompletableFuture askClient(MyParam param) { clientSideLauncher.startListening(); serverSideLauncher.startListening(); + // tries == 0 CompletableFuture errorFuture1 = serverSideLauncher.getRemoteProxy().askClient(new MyParam("FOO")); try { System.out.println(errorFuture1.get()); @@ -443,6 +452,8 @@ public CompletableFuture askClient(MyParam param) { } catch (ExecutionException e) { Assert.assertNotNull(((ResponseErrorException)e.getCause()).getResponseError().getMessage()); } + + // tries == 1 CompletableFuture errorFuture2 = serverSideLauncher.getRemoteProxy().askClient(new MyParam("FOO")); try { errorFuture2.get(); @@ -450,6 +461,99 @@ public CompletableFuture askClient(MyParam param) { } catch (ExecutionException e) { Assert.assertNotNull(((ResponseErrorException)e.getCause()).getResponseError().getMessage()); } + + // tries == 2 + CompletableFuture errorFuture3 = serverSideLauncher.getRemoteProxy().askClient(new MyParam("FOO")); + try { + errorFuture3.get(); + Assert.fail(); + } catch (ExecutionException e) { + Assert.assertNotNull(((ResponseErrorException)e.getCause()).getResponseError().getMessage()); + } + + // tries == 3 + CompletableFuture goodFuture = serverSideLauncher.getRemoteProxy().askClient(new MyParam("FOO")); + Assert.assertEquals("FOO", goodFuture.get(TIMEOUT, TimeUnit.MILLISECONDS).value); + } + + @Test + public void testVersatilityResponseError() throws Exception { + Logger.getLogger(RemoteEndpoint.class.getName()).setLevel(Level.OFF); + // create client side + PipedInputStream in = new PipedInputStream(); + PipedOutputStream out = new PipedOutputStream(); + PipedInputStream in2 = new PipedInputStream(); + PipedOutputStream out2 = new PipedOutputStream(); + + // See https://github.com/eclipse-lsp4j/lsp4j/issues/510 for full details. + // Make sure that the thread that writes to the PipedOutputStream stays alive + // until the read from the PipedInputStream. Using a cached thread pool + // does not 100% guarantee that, but increases the probability that the + // selected thread will exist for the lifetime of the test. + ExecutorService executor = Executors.newCachedThreadPool(); + + in.connect(out2); + out.connect(in2); + + MyClient client = new MyClient() { + private int tries = 0; + + @Override + public CompletableFuture askClient(MyParam param) { + if (tries == 0) { + tries++; + throw new ResponseErrorException(new ResponseError(ResponseErrorCode.InvalidParams, "Direct Throw", "data")); + } + if (tries == 1) { + tries++; + CompletableFuture future = new CompletableFuture<>(); + future.completeExceptionally(new ResponseErrorException(new ResponseError(ResponseErrorCode.InvalidParams, "completeExceptionally", "data"))); + return future; + } + return CompletableFutures.computeAsync(executor, cancelToken -> { + if (tries++ == 2) + throw new ResponseErrorException(new ResponseError(ResponseErrorCode.InvalidParams, "Throw in computeAsync", "data")); + return param; + }); + } + }; + Launcher clientSideLauncher = Launcher.createLauncher(client, MyServer.class, in, out); + + // create server side + MyServer server = new MyServerImpl(); + Launcher serverSideLauncher = Launcher.createLauncher(server, MyClient.class, in2, out2); + + clientSideLauncher.startListening(); + serverSideLauncher.startListening(); + + // tries == 0 + CompletableFuture errorFuture1 = serverSideLauncher.getRemoteProxy().askClient(new MyParam("FOO")); + try { + System.out.println(errorFuture1.get()); + Assert.fail(); + } catch (ExecutionException e) { + Assert.assertEquals("Direct Throw", ((ResponseErrorException)e.getCause()).getResponseError().getMessage()); + } + + // tries == 1 + CompletableFuture errorFuture2 = serverSideLauncher.getRemoteProxy().askClient(new MyParam("FOO")); + try { + errorFuture2.get(); + Assert.fail(); + } catch (ExecutionException e) { + Assert.assertEquals("completeExceptionally", ((ResponseErrorException)e.getCause()).getResponseError().getMessage()); + } + + // tries == 2 + CompletableFuture errorFuture3 = serverSideLauncher.getRemoteProxy().askClient(new MyParam("FOO")); + try { + errorFuture3.get(); + Assert.fail(); + } catch (ExecutionException e) { + Assert.assertEquals("Throw in computeAsync", ((ResponseErrorException)e.getCause()).getResponseError().getMessage()); + } + + // tries == 3 CompletableFuture goodFuture = serverSideLauncher.getRemoteProxy().askClient(new MyParam("FOO")); Assert.assertEquals("FOO", goodFuture.get(TIMEOUT, TimeUnit.MILLISECONDS).value); } diff --git a/org.eclipse.lsp4j.jsonrpc/src/test/java/org/eclipse/lsp4j/jsonrpc/test/RemoteEndpointTest.java b/org.eclipse.lsp4j.jsonrpc/src/test/java/org/eclipse/lsp4j/jsonrpc/test/RemoteEndpointTest.java index f62d9329..d1f7407f 100644 --- a/org.eclipse.lsp4j.jsonrpc/src/test/java/org/eclipse/lsp4j/jsonrpc/test/RemoteEndpointTest.java +++ b/org.eclipse.lsp4j.jsonrpc/src/test/java/org/eclipse/lsp4j/jsonrpc/test/RemoteEndpointTest.java @@ -34,6 +34,7 @@ import org.eclipse.lsp4j.jsonrpc.JsonRpcException; import org.eclipse.lsp4j.jsonrpc.MessageConsumer; import org.eclipse.lsp4j.jsonrpc.RemoteEndpoint; +import org.eclipse.lsp4j.jsonrpc.ResponseErrorException; import org.eclipse.lsp4j.jsonrpc.messages.Either; import org.eclipse.lsp4j.jsonrpc.messages.Message; import org.eclipse.lsp4j.jsonrpc.messages.MessageIssue; @@ -42,6 +43,7 @@ import org.eclipse.lsp4j.jsonrpc.messages.ResponseError; import org.eclipse.lsp4j.jsonrpc.messages.ResponseErrorCode; import org.eclipse.lsp4j.jsonrpc.messages.ResponseMessage; +import org.eclipse.lsp4j.jsonrpc.test.IntegrationTest.MyParam; import org.junit.Assert; import org.junit.Test; @@ -219,6 +221,73 @@ public CompletableFuture request(String method, Object parameter) { logMessages.unregister(); } } + + @Test + public void testResponseErrorExceptionInEndpoint() { + LogMessageAccumulator logMessages = new LogMessageAccumulator(); + try { + // Don't show the exception in the test execution log + logMessages.registerTo(RemoteEndpoint.class); + + TestEndpoint endp = new TestEndpoint() { + @Override + public CompletableFuture request(String method, Object parameter) { + throw new ResponseErrorException(new ResponseError(ResponseErrorCode.InvalidParams, "Direct Throw", "data")); + } + }; + TestMessageConsumer consumer = new TestMessageConsumer(); + RemoteEndpoint endpoint = new RemoteEndpoint(consumer, endp); + + endpoint.consume(init(new RequestMessage(), it -> { + it.setId("1"); + it.setMethod("foo"); + it.setParams("myparam"); + })); + + ResponseMessage response = (ResponseMessage) consumer.messages.get(0); + assertEquals("Direct Throw", response.getError().getMessage()); + assertEquals(ResponseErrorCode.InvalidParams.getValue(), response.getError().getCode()); + String data = (String) response.getError().getData(); + assertEquals("data", data); + } finally { + logMessages.unregister(); + } + } + + @Test + public void testResponseErrorExceptionFromFutureInEndpoint() { + LogMessageAccumulator logMessages = new LogMessageAccumulator(); + try { + // Don't show the exception in the test execution log + logMessages.registerTo(RemoteEndpoint.class); + + TestEndpoint endp = new TestEndpoint() { + @Override + public CompletableFuture request(String method, Object parameter) { + CompletableFuture future = new CompletableFuture<>(); + future.completeExceptionally(new ResponseErrorException( + new ResponseError(ResponseErrorCode.InvalidParams, "completeExceptionally", "data"))); + return future; + } + }; + TestMessageConsumer consumer = new TestMessageConsumer(); + RemoteEndpoint endpoint = new RemoteEndpoint(consumer, endp); + + endpoint.consume(init(new RequestMessage(), it -> { + it.setId("1"); + it.setMethod("foo"); + it.setParams("myparam"); + })); + + ResponseMessage response = (ResponseMessage) consumer.messages.get(0); + assertEquals("completeExceptionally", response.getError().getMessage()); + assertEquals(ResponseErrorCode.InvalidParams.getValue(), response.getError().getCode()); + String data = (String) response.getError().getData(); + assertEquals("data", data); + } finally { + logMessages.unregister(); + } + } @Test public void testExceptionInConsumer() throws Exception {