From db50b7d6089c6fbc988736863b36368d2a8cdad4 Mon Sep 17 00:00:00 2001 From: Andrew Parmet Date: Thu, 30 May 2024 10:56:46 -0400 Subject: [PATCH] Recreate or wrap exceptions thrown by async transport implementations to preserve caller stack traces (#656) * wrap exceptions thrown by async transport implementations in IOException Signed-off-by: Andrew Parmet * license headers Signed-off-by: Andrew Parmet * changelog Signed-off-by: Andrew Parmet * tolerate null causes Signed-off-by: Andrew Parmet * fix tests Signed-off-by: Andrew Parmet * remove print Signed-off-by: Andrew Parmet * one more license header Signed-off-by: Andrew Parmet * fix last non-aws test Signed-off-by: Andrew Parmet * use multicatch to restrict caught exceptions Signed-off-by: Andrew Parmet * Replicate the RestClient exception wrapping for async invocation flow Signed-off-by: Andriy Redko * replicate hc5 exception extraction strategy in aws transport Signed-off-by: Andrew Parmet * move other tests Signed-off-by: Andrew Parmet * lint Signed-off-by: Andrew Parmet * delete aws test for now; add support for OpenSearchClientException Signed-off-by: Andrew Parmet * reintroduce an aws test, sadly, not extending the abstract case Signed-off-by: Andrew Parmet * java 8 Signed-off-by: Andrew Parmet * replicate ISE Signed-off-by: Andrew Parmet * poke Signed-off-by: Andrew Parmet * handle some netty-specific channel exceptions Signed-off-by: Andrew Parmet * poke Signed-off-by: Andrew Parmet * nevermind netty Signed-off-by: Andrew Parmet * io before rt Signed-off-by: Andrew Parmet * no hc5 Signed-off-by: Andrew Parmet --------- Signed-off-by: Andrew Parmet Signed-off-by: Andriy Redko Co-authored-by: Andriy Redko --- CHANGELOG.md | 3 +- .../transport/aws/AwsSdk2Transport.java | 73 ++++++++++++-- .../ApacheHttpClient5Transport.java | 98 +++++++++++++++++-- .../aws/AwsSdk2AsyncStacktraceIT.java | 32 ++++++ .../integTest/AbstractAsyncStracktraceIT.java | 25 +++++ .../httpclient5/AsyncStacktraceIT.java | 13 +++ 6 files changed, 225 insertions(+), 19 deletions(-) create mode 100644 java-client/src/test/java/org/opensearch/client/opensearch/integTest/aws/AwsSdk2AsyncStacktraceIT.java create mode 100644 java-client/src/test/java11/org/opensearch/client/opensearch/integTest/AbstractAsyncStracktraceIT.java create mode 100644 java-client/src/test/java11/org/opensearch/client/opensearch/integTest/httpclient5/AsyncStacktraceIT.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ba151a371..761c7f9b3b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,7 @@ This section is for maintaining a changelog for all breaking changes for the cli ### Fixed - ApacheHttpClient5Transport requires Apache Commons Logging dependency ([#1003](https://github.com/opensearch-project/opensearch-java/pull/1003)) +- Preserve caller information in stack traces when synchronous callers use asynchronous transports ([#656](https://github.com/opensearch-project/opensearch-java/pull/656)) ### Security @@ -441,4 +442,4 @@ This section is for maintaining a changelog for all breaking changes for the cli [2.5.0]: https://github.com/opensearch-project/opensearch-java/compare/v2.4.0...v2.5.0 [2.4.0]: https://github.com/opensearch-project/opensearch-java/compare/v2.3.0...v2.4.0 [2.3.0]: https://github.com/opensearch-project/opensearch-java/compare/v2.2.0...v2.3.0 -[2.2.0]: https://github.com/opensearch-project/opensearch-java/compare/v2.1.0...v2.2.0 \ No newline at end of file +[2.2.0]: https://github.com/opensearch-project/opensearch-java/compare/v2.1.0...v2.2.0 diff --git a/java-client/src/main/java/org/opensearch/client/transport/aws/AwsSdk2Transport.java b/java-client/src/main/java/org/opensearch/client/transport/aws/AwsSdk2Transport.java index 289399b610..ad6718ec20 100644 --- a/java-client/src/main/java/org/opensearch/client/transport/aws/AwsSdk2Transport.java +++ b/java-client/src/main/java/org/opensearch/client/transport/aws/AwsSdk2Transport.java @@ -14,6 +14,8 @@ import java.io.IOException; import java.io.InputStream; import java.io.UnsupportedEncodingException; +import java.net.ConnectException; +import java.net.SocketTimeoutException; import java.net.URI; import java.net.URISyntaxException; import java.net.URLEncoder; @@ -30,12 +32,14 @@ import javax.annotation.CheckForNull; import javax.annotation.Nonnull; import javax.annotation.Nullable; +import javax.net.ssl.SSLHandshakeException; import org.opensearch.client.json.JsonpDeserializer; import org.opensearch.client.json.JsonpMapper; import org.opensearch.client.json.jackson.JacksonJsonpMapper; import org.opensearch.client.opensearch._types.ErrorCause; import org.opensearch.client.opensearch._types.ErrorResponse; import org.opensearch.client.opensearch._types.OpenSearchException; +import org.opensearch.client.opensearch.generic.OpenSearchClientException; import org.opensearch.client.transport.Endpoint; import org.opensearch.client.transport.GenericEndpoint; import org.opensearch.client.transport.JsonEndpoint; @@ -199,17 +203,14 @@ public ResponseT performRequest( try { return executeAsync((SdkAsyncHttpClient) httpClient, clientReq, requestBody, endpoint, options).get(); } catch (ExecutionException e) { - Throwable cause = e.getCause(); - if (cause != null) { - if (cause instanceof IOException) { - throw (IOException) cause; - } - if (cause instanceof RuntimeException) { - throw (RuntimeException) cause; - } - throw new RuntimeException(cause); + Exception cause = extractAndWrapCause(e); + if (cause instanceof IOException) { + throw (IOException) cause; + } + if (cause instanceof RuntimeException) { + throw (RuntimeException) cause; } - throw new RuntimeException(e); + throw new IllegalStateException("unexpected exception type: must be either RuntimeException or IOException", cause); } catch (InterruptedException e) { throw new IOException("HttpRequest was interrupted", e); } @@ -615,4 +616,56 @@ private static Optional or(Optional opt, Supplier ResponseT performRequest( TransportOptions options ) throws IOException { try { - return performRequestAsync(request, endpoint, options).join(); - } catch (final CompletionException ex) { - if (ex.getCause() instanceof RuntimeException) { - throw (RuntimeException) ex.getCause(); - } else if (ex.getCause() instanceof IOException) { - throw (IOException) ex.getCause(); - } else { - throw new IOException(ex.getCause()); + return performRequestAsync(request, endpoint, options).get(); + } catch (final Exception ex) { + Exception cause = extractAndWrapCause(ex); + if (cause instanceof IOException) { + throw (IOException) cause; + } + if (cause instanceof RuntimeException) { + throw (RuntimeException) cause; } + throw new IllegalStateException("unexpected exception type: must be either RuntimeException or IOException", cause); } } @@ -1083,4 +1092,77 @@ public InputStream asInput() { return new ByteArrayInputStream(this.buf, 0, this.count); } } + + /** + * Wrap the exception so the caller's signature shows up in the stack trace, taking care to copy the original type and message + * where possible so async and sync code don't have to check different exceptions. + */ + private static Exception extractAndWrapCause(Exception exception) { + if (exception instanceof InterruptedException) { + throw new RuntimeException("thread waiting for the response was interrupted", exception); + } + if (exception instanceof ExecutionException) { + ExecutionException executionException = (ExecutionException) exception; + Throwable t = executionException.getCause() == null ? executionException : executionException.getCause(); + if (t instanceof Error) { + throw (Error) t; + } + exception = (Exception) t; + } + if (exception instanceof ConnectTimeoutException) { + ConnectTimeoutException e = new ConnectTimeoutException(exception.getMessage()); + e.initCause(exception); + return e; + } + if (exception instanceof SocketTimeoutException) { + SocketTimeoutException e = new SocketTimeoutException(exception.getMessage()); + e.initCause(exception); + return e; + } + if (exception instanceof ConnectionClosedException) { + ConnectionClosedException e = new ConnectionClosedException(exception.getMessage()); + e.initCause(exception); + return e; + } + if (exception instanceof SSLHandshakeException) { + SSLHandshakeException e = new SSLHandshakeException( + exception.getMessage() + "\nSee https://opensearch.org/docs/latest/clients/java/ for troubleshooting." + ); + e.initCause(exception); + return e; + } + if (exception instanceof ConnectException) { + ConnectException e = new ConnectException(exception.getMessage()); + e.initCause(exception); + return e; + } + if (exception instanceof ResponseException) { + try { + ResponseException e = new ResponseException(((ResponseException) exception).getResponse()); + e.initCause(exception); + return e; + } catch (final IOException ex) { + // We are not able to reconstruct the response, throw IOException instead + return new IOException(exception.getMessage(), exception); + } + } + if (exception instanceof IOException) { + return new IOException(exception.getMessage(), exception); + } + if (exception instanceof OpenSearchException) { + final OpenSearchException e = new OpenSearchException(((OpenSearchException) exception).response()); + e.initCause(exception); + return e; + } + if (exception instanceof OpenSearchClientException) { + final OpenSearchClientException e = new OpenSearchClientException(((OpenSearchClientException) exception).response()); + e.initCause(exception); + return e; + } + if (exception instanceof RuntimeException) { + return new RuntimeException(exception.getMessage(), exception); + } + return new RuntimeException("error while performing request", exception); + } + } diff --git a/java-client/src/test/java/org/opensearch/client/opensearch/integTest/aws/AwsSdk2AsyncStacktraceIT.java b/java-client/src/test/java/org/opensearch/client/opensearch/integTest/aws/AwsSdk2AsyncStacktraceIT.java new file mode 100644 index 0000000000..d620bf133b --- /dev/null +++ b/java-client/src/test/java/org/opensearch/client/opensearch/integTest/aws/AwsSdk2AsyncStacktraceIT.java @@ -0,0 +1,32 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.client.opensearch.integTest.aws; + +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; + +import java.util.List; +import org.apache.logging.log4j.core.util.Throwables; +import org.junit.Test; +import org.opensearch.client.opensearch.OpenSearchClient; + +// It would be nice to extend AbstractAsyncStracktraceIT. +public class AwsSdk2AsyncStacktraceIT extends AwsSdk2TransportTestCase { + @Test + public void testFailureFromClientPreservesStacktraceOfCaller() throws Exception { + final OpenSearchClient client = getClient(false, null, null); + Exception thrown = assertThrows(Exception.class, () -> client.indices().get(g -> g.index("nonexisting-index"))); + + List stacktraceElements = Throwables.toStringList(thrown); + boolean someElementContainsCallerMethodName = stacktraceElements.stream() + .anyMatch(it -> it.contains("testFailureFromClientPreservesStacktraceOfCaller")); + + assertTrue(someElementContainsCallerMethodName); + } +} diff --git a/java-client/src/test/java11/org/opensearch/client/opensearch/integTest/AbstractAsyncStracktraceIT.java b/java-client/src/test/java11/org/opensearch/client/opensearch/integTest/AbstractAsyncStracktraceIT.java new file mode 100644 index 0000000000..590c0b2539 --- /dev/null +++ b/java-client/src/test/java11/org/opensearch/client/opensearch/integTest/AbstractAsyncStracktraceIT.java @@ -0,0 +1,25 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.client.opensearch.integTest; + +import org.apache.logging.log4j.core.util.Throwables; +import org.junit.Test; + +public abstract class AbstractAsyncStracktraceIT extends OpenSearchJavaClientTestCase { + @Test + public void testFailureFromClientPreservesStacktraceOfCaller() throws Exception { + var thrown = assertThrows(Exception.class, () -> javaClient().indices().get(g -> g.index("nonexisting-index"))); + + var stacktraceElements = Throwables.toStringList(thrown); + var someElementContainsCallerMethodName = stacktraceElements.stream() + .anyMatch(it -> it.contains("testFailureFromClientPreservesStacktraceOfCaller")); + + assertTrue(someElementContainsCallerMethodName); + } +} diff --git a/java-client/src/test/java11/org/opensearch/client/opensearch/integTest/httpclient5/AsyncStacktraceIT.java b/java-client/src/test/java11/org/opensearch/client/opensearch/integTest/httpclient5/AsyncStacktraceIT.java new file mode 100644 index 0000000000..1312c767fe --- /dev/null +++ b/java-client/src/test/java11/org/opensearch/client/opensearch/integTest/httpclient5/AsyncStacktraceIT.java @@ -0,0 +1,13 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.client.opensearch.integTest.httpclient5; + +import org.opensearch.client.opensearch.integTest.AbstractAsyncStracktraceIT; + +public class AsyncStacktraceIT extends AbstractAsyncStracktraceIT implements HttpClient5TransportSupport {}