From 211d31b74f598d02fd97a4a27ba93b13e4983204 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 6 Mar 2018 18:39:12 -0800 Subject: [PATCH 1/9] Client: Wrap synchronous exceptions In the past the Low Level REST Client was super careful not to wrap any exceptions that it throws from synchronous calls so that callers can catch the exceptions and work with them. The trouble with that is that the exceptions are originally thrown on the async thread pool and then transfered back into calling thread. That means that the stack trace of the exception doesn't have the calling method which is *super* *ultra* confusing. This change always wraps exceptions transfered from the async thread pool so that the stack trace of the thrown exception contains the caller's stack. It tries to preserve the type of the throw exception but this is quite a fiddly thing to get right. We have to catch every type of exception that we want to preserve, wrap with the same type and rethrow. I've preserved the types of all exceptions that we had tests mentioning but no other exceptions. The other exceptions are either wrapped in `IOException` or `RuntimeException`. Closes #28399 --- .../client/ResponseException.java | 10 ++ .../org/elasticsearch/client/RestClient.java | 102 +++++++++++------- .../client/RestClientMultipleHostsTests.java | 65 ++++++++--- .../client/RestClientSingleHostTests.java | 28 ++++- .../elasticsearch/client/RestClientTests.java | 90 +++++++++++----- .../client/SyncResponseListenerTests.java | 96 +++++++++++++---- 6 files changed, 292 insertions(+), 99 deletions(-) diff --git a/client/rest/src/main/java/org/elasticsearch/client/ResponseException.java b/client/rest/src/main/java/org/elasticsearch/client/ResponseException.java index 072e45ffb0e97..5e646d975c89c 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/ResponseException.java +++ b/client/rest/src/main/java/org/elasticsearch/client/ResponseException.java @@ -39,6 +39,16 @@ public ResponseException(Response response) throws IOException { this.response = response; } + /** + * Wrap a {@linkplain ResponseException} with another one with the current + * stack trace. This is used during synchronous calls so that the caller + * ends up in the stack trace of the exception thrown. + */ + ResponseException(ResponseException e) throws IOException { + super(e.getMessage(), e); + this.response = e.getResponse(); + } + private static String buildMessage(Response response) throws IOException { String message = String.format(Locale.ROOT, "method [%s], host [%s], URI [%s], status line [%s]", diff --git a/client/rest/src/main/java/org/elasticsearch/client/RestClient.java b/client/rest/src/main/java/org/elasticsearch/client/RestClient.java index e221ed081a597..269e5f732f8cf 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/RestClient.java +++ b/client/rest/src/main/java/org/elasticsearch/client/RestClient.java @@ -38,6 +38,7 @@ import org.apache.http.client.protocol.HttpClientContext; import org.apache.http.client.utils.URIBuilder; import org.apache.http.concurrent.FutureCallback; +import org.apache.http.conn.ConnectTimeoutException; import org.apache.http.impl.auth.BasicScheme; import org.apache.http.impl.client.BasicAuthCache; import org.apache.http.impl.nio.client.CloseableHttpAsyncClient; @@ -47,6 +48,7 @@ import java.io.Closeable; import java.io.IOException; +import java.net.SocketTimeoutException; import java.net.URI; import java.net.URISyntaxException; import java.util.ArrayList; @@ -218,7 +220,8 @@ public Response performRequest(String method, String endpoint, Map requestParams = new HashMap<>(params); - //ignore is a special parameter supported by the clients, shouldn't be sent to es - String ignoreString = requestParams.remove("ignore"); - Set ignoreErrorCodes; - if (ignoreString == null) { - if (HttpHead.METHOD_NAME.equals(method)) { - //404 never causes error if returned for a HEAD request - ignoreErrorCodes = Collections.singleton(404); - } else { - ignoreErrorCodes = Collections.emptySet(); - } + performRequestAsyncNoCatch(method, endpoint, params, entity, httpAsyncResponseConsumerFactory, + responseListener, headers); + } catch (Exception e) { + responseListener.onFailure(e); + } + } + + void performRequestAsyncNoCatch(String method, String endpoint, Map params, + HttpEntity entity, HttpAsyncResponseConsumerFactory httpAsyncResponseConsumerFactory, + ResponseListener responseListener, Header... headers) { + Objects.requireNonNull(params, "params must not be null"); + Map requestParams = new HashMap<>(params); + //ignore is a special parameter supported by the clients, shouldn't be sent to es + String ignoreString = requestParams.remove("ignore"); + Set ignoreErrorCodes; + if (ignoreString == null) { + if (HttpHead.METHOD_NAME.equals(method)) { + //404 never causes error if returned for a HEAD request + ignoreErrorCodes = Collections.singleton(404); } else { - String[] ignoresArray = ignoreString.split(","); - ignoreErrorCodes = new HashSet<>(); - if (HttpHead.METHOD_NAME.equals(method)) { - //404 never causes error if returned for a HEAD request - ignoreErrorCodes.add(404); - } - for (String ignoreCode : ignoresArray) { - try { - ignoreErrorCodes.add(Integer.valueOf(ignoreCode)); - } catch (NumberFormatException e) { - throw new IllegalArgumentException("ignore value should be a number, found [" + ignoreString + "] instead", e); - } + ignoreErrorCodes = Collections.emptySet(); + } + } else { + String[] ignoresArray = ignoreString.split(","); + ignoreErrorCodes = new HashSet<>(); + if (HttpHead.METHOD_NAME.equals(method)) { + //404 never causes error if returned for a HEAD request + ignoreErrorCodes.add(404); + } + for (String ignoreCode : ignoresArray) { + try { + ignoreErrorCodes.add(Integer.valueOf(ignoreCode)); + } catch (NumberFormatException e) { + throw new IllegalArgumentException("ignore value should be a number, found [" + ignoreString + "] instead", e); } } - URI uri = buildUri(pathPrefix, endpoint, requestParams); - HttpRequestBase request = createHttpRequest(method, uri, entity); - setHeaders(request, headers); - FailureTrackingResponseListener failureTrackingResponseListener = new FailureTrackingResponseListener(responseListener); - long startTime = System.nanoTime(); - performRequestAsync(startTime, nextHost(), request, ignoreErrorCodes, httpAsyncResponseConsumerFactory, - failureTrackingResponseListener); - } catch (Exception e) { - responseListener.onFailure(e); } + URI uri = buildUri(pathPrefix, endpoint, requestParams); + HttpRequestBase request = createHttpRequest(method, uri, entity); + setHeaders(request, headers); + FailureTrackingResponseListener failureTrackingResponseListener = new FailureTrackingResponseListener(responseListener); + long startTime = System.nanoTime(); + performRequestAsync(startTime, nextHost(), request, ignoreErrorCodes, httpAsyncResponseConsumerFactory, + failureTrackingResponseListener); } private void performRequestAsync(final long startTime, final HostTuple> hostTuple, final HttpRequestBase request, @@ -674,12 +684,30 @@ Response get() throws IOException { e.addSuppressed(exception); throw e; } - //try and leave the exception untouched as much as possible but we don't want to just add throws Exception clause everywhere + /* + * Wrap and rethrow whatever exception we received, copying the type + * where possible so the synchronous API looks as much as possible + * like the asynchronous API. We wrap the exception so that the caller's + * signature shows up in any exception we throw. + */ + if (exception instanceof ResponseException) { + throw new ResponseException((ResponseException) exception); + } + if (exception instanceof ConnectTimeoutException) { + ConnectTimeoutException e = new ConnectTimeoutException(exception.getMessage()); + e.initCause(exception); + throw e; + } + if (exception instanceof SocketTimeoutException) { + SocketTimeoutException e = new SocketTimeoutException(exception.getMessage()); + e.initCause(exception); + throw e; + } if (exception instanceof IOException) { - throw (IOException) exception; + throw new IOException(exception.getMessage(), exception); } if (exception instanceof RuntimeException){ - throw (RuntimeException) exception; + throw new RuntimeException(exception.getMessage(), exception); } throw new RuntimeException("error while performing request", exception); } diff --git a/client/rest/src/test/java/org/elasticsearch/client/RestClientMultipleHostsTests.java b/client/rest/src/test/java/org/elasticsearch/client/RestClientMultipleHostsTests.java index 6f87a244ff59f..88a62026a0665 100644 --- a/client/rest/src/test/java/org/elasticsearch/client/RestClientMultipleHostsTests.java +++ b/client/rest/src/test/java/org/elasticsearch/client/RestClientMultipleHostsTests.java @@ -35,6 +35,7 @@ import org.apache.http.message.BasicStatusLine; import org.apache.http.nio.protocol.HttpAsyncRequestProducer; import org.apache.http.nio.protocol.HttpAsyncResponseConsumer; +import org.junit.After; import org.junit.Before; import org.mockito.invocation.InvocationOnMock; import org.mockito.stubbing.Answer; @@ -44,6 +45,8 @@ import java.util.Collections; import java.util.HashSet; import java.util.Set; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; import java.util.concurrent.Future; import static org.elasticsearch.client.RestClientTestUtil.randomErrorNoRetryStatusCode; @@ -66,6 +69,7 @@ */ public class RestClientMultipleHostsTests extends RestClientTestCase { + private ExecutorService exec = Executors.newFixedThreadPool(1); private RestClient restClient; private HttpHost[] httpHosts; private HostsTrackingFailureListener failureListener; @@ -79,23 +83,28 @@ public void createRestClient() throws IOException { @Override public Future answer(InvocationOnMock invocationOnMock) throws Throwable { HttpAsyncRequestProducer requestProducer = (HttpAsyncRequestProducer) invocationOnMock.getArguments()[0]; - HttpUriRequest request = (HttpUriRequest)requestProducer.generateRequest(); - HttpHost httpHost = requestProducer.getTarget(); + final HttpUriRequest request = (HttpUriRequest)requestProducer.generateRequest(); + final HttpHost httpHost = requestProducer.getTarget(); HttpClientContext context = (HttpClientContext) invocationOnMock.getArguments()[2]; assertThat(context.getAuthCache().get(httpHost), instanceOf(BasicScheme.class)); - FutureCallback futureCallback = (FutureCallback) invocationOnMock.getArguments()[3]; + final FutureCallback futureCallback = (FutureCallback) invocationOnMock.getArguments()[3]; //return the desired status code or exception depending on the path - if (request.getURI().getPath().equals("/soe")) { - futureCallback.failed(new SocketTimeoutException(httpHost.toString())); - } else if (request.getURI().getPath().equals("/coe")) { - futureCallback.failed(new ConnectTimeoutException(httpHost.toString())); - } else if (request.getURI().getPath().equals("/ioe")) { - futureCallback.failed(new IOException(httpHost.toString())); - } else { - int statusCode = Integer.parseInt(request.getURI().getPath().substring(1)); - StatusLine statusLine = new BasicStatusLine(new ProtocolVersion("http", 1, 1), statusCode, ""); - futureCallback.completed(new BasicHttpResponse(statusLine)); - } + exec.execute(new Runnable() { + @Override + public void run() { + if (request.getURI().getPath().equals("/soe")) { + futureCallback.failed(new SocketTimeoutException(httpHost.toString())); + } else if (request.getURI().getPath().equals("/coe")) { + futureCallback.failed(new ConnectTimeoutException(httpHost.toString())); + } else if (request.getURI().getPath().equals("/ioe")) { + futureCallback.failed(new IOException(httpHost.toString())); + } else { + int statusCode = Integer.parseInt(request.getURI().getPath().substring(1)); + StatusLine statusLine = new BasicStatusLine(new ProtocolVersion("http", 1, 1), statusCode, ""); + futureCallback.completed(new BasicHttpResponse(statusLine)); + } + } + }); return null; } }); @@ -108,6 +117,14 @@ public Future answer(InvocationOnMock invocationOnMock) throws Thr restClient = new RestClient(httpClient, 10000, new Header[0], httpHosts, null, failureListener); } + /** + * Shutdown the executor so we don't leak threads into other test runs. + */ + @After + public void shutdownExec() { + exec.shutdown(); + } + public void testRoundRobinOkStatusCodes() throws IOException { int numIters = RandomNumbers.randomIntBetween(getRandom(), 1, 5); for (int i = 0; i < numIters; i++) { @@ -163,6 +180,11 @@ public void testRoundRobinRetryErrors() throws IOException { restClient.performRequest(randomHttpMethod(getRandom()), retryEndpoint); fail("request should have failed"); } catch(ResponseException e) { + /* + * Unwrap the top level failure that was added so the stack trace contains + * the caller. It wraps the exception that contains the failed hosts. + */ + e = (ResponseException) e.getCause(); Set hostsSet = new HashSet<>(); Collections.addAll(hostsSet, httpHosts); //first request causes all the hosts to be blacklisted, the returned exception holds one suppressed exception each @@ -183,6 +205,11 @@ public void testRoundRobinRetryErrors() throws IOException { } while(e != null); assertEquals("every host should have been used but some weren't: " + hostsSet, 0, hostsSet.size()); } catch(IOException e) { + /* + * Unwrap the top level failure that was added so the stack trace contains + * the caller. It wraps the exception that contains the failed hosts. + */ + e = (IOException) e.getCause(); Set hostsSet = new HashSet<>(); Collections.addAll(hostsSet, httpHosts); //first request causes all the hosts to be blacklisted, the returned exception holds one suppressed exception each @@ -221,6 +248,11 @@ public void testRoundRobinRetryErrors() throws IOException { failureListener.assertCalled(response.getHost()); assertEquals(0, e.getSuppressed().length); } catch(IOException e) { + /* + * Unwrap the top level failure that was added so the stack trace contains + * the caller. It wraps the exception that contains the failed hosts. + */ + e = (IOException) e.getCause(); HttpHost httpHost = HttpHost.create(e.getMessage()); assertTrue("host [" + httpHost + "] not found, most likely used multiple times", hostsSet.remove(httpHost)); //after the first request, all hosts are blacklisted, a single one gets resurrected each time @@ -263,6 +295,11 @@ public void testRoundRobinRetryErrors() throws IOException { assertThat(response.getHost(), equalTo(selectedHost)); failureListener.assertCalled(selectedHost); } catch(IOException e) { + /* + * Unwrap the top level failure that was added so the stack trace contains + * the caller. It wraps the exception that contains the failed hosts. + */ + e = (IOException) e.getCause(); HttpHost httpHost = HttpHost.create(e.getMessage()); assertThat(httpHost, equalTo(selectedHost)); failureListener.assertCalled(selectedHost); diff --git a/client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostTests.java b/client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostTests.java index 541193c733d56..caf9ce6be2e07 100644 --- a/client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostTests.java +++ b/client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostTests.java @@ -47,6 +47,7 @@ import org.apache.http.nio.protocol.HttpAsyncRequestProducer; import org.apache.http.nio.protocol.HttpAsyncResponseConsumer; import org.apache.http.util.EntityUtils; +import org.junit.After; import org.junit.Before; import org.mockito.ArgumentCaptor; import org.mockito.invocation.InvocationOnMock; @@ -61,6 +62,8 @@ import java.util.HashSet; import java.util.Map; import java.util.Set; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; import java.util.concurrent.Future; import static org.elasticsearch.client.RestClientTestUtil.getAllErrorStatusCodes; @@ -68,6 +71,7 @@ import static org.elasticsearch.client.RestClientTestUtil.getOkStatusCodes; import static org.elasticsearch.client.RestClientTestUtil.randomHttpMethod; import static org.elasticsearch.client.RestClientTestUtil.randomStatusCode; +import static org.elasticsearch.client.SyncResponseListenerTests.assertExceptionStackContainsCallingMethod; import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.instanceOf; import static org.junit.Assert.assertArrayEquals; @@ -88,6 +92,7 @@ */ public class RestClientSingleHostTests extends RestClientTestCase { + private ExecutorService exec = Executors.newFixedThreadPool(1); private RestClient restClient; private Header[] defaultHeaders; private HttpHost httpHost; @@ -105,7 +110,8 @@ public Future answer(InvocationOnMock invocationOnMock) throws Thr HttpAsyncRequestProducer requestProducer = (HttpAsyncRequestProducer) invocationOnMock.getArguments()[0]; HttpClientContext context = (HttpClientContext) invocationOnMock.getArguments()[2]; assertThat(context.getAuthCache().get(httpHost), instanceOf(BasicScheme.class)); - FutureCallback futureCallback = (FutureCallback) invocationOnMock.getArguments()[3]; + final FutureCallback futureCallback = + (FutureCallback) invocationOnMock.getArguments()[3]; HttpUriRequest request = (HttpUriRequest)requestProducer.generateRequest(); //return the desired status code or exception depending on the path if (request.getURI().getPath().equals("/soe")) { @@ -116,7 +122,7 @@ public Future answer(InvocationOnMock invocationOnMock) throws Thr int statusCode = Integer.parseInt(request.getURI().getPath().substring(1)); StatusLine statusLine = new BasicStatusLine(new ProtocolVersion("http", 1, 1), statusCode, ""); - HttpResponse httpResponse = new BasicHttpResponse(statusLine); + final HttpResponse httpResponse = new BasicHttpResponse(statusLine); //return the same body that was sent if (request instanceof HttpEntityEnclosingRequest) { HttpEntity entity = ((HttpEntityEnclosingRequest) request).getEntity(); @@ -128,7 +134,13 @@ public Future answer(InvocationOnMock invocationOnMock) throws Thr } //return the same headers that were sent httpResponse.setHeaders(request.getAllHeaders()); - futureCallback.completed(httpResponse); + // Call the callback asynchronous to better simulate how async http client works + exec.execute(new Runnable() { + @Override + public void run() { + futureCallback.completed(httpResponse); + } + }); } return null; } @@ -140,6 +152,14 @@ public Future answer(InvocationOnMock invocationOnMock) throws Thr restClient = new RestClient(httpClient, 10000, defaultHeaders, new HttpHost[]{httpHost}, null, failureListener); } + /** + * Shutdown the executor so we don't leak threads into other test runs. + */ + @After + public void shutdownExec() { + exec.shutdown(); + } + public void testNullPath() throws IOException { for (String method : getHttpMethods()) { try { @@ -258,6 +278,7 @@ public void testErrorStatusCodes() throws IOException { throw e; } assertEquals(errorStatusCode, e.getResponse().getStatusLine().getStatusCode()); + assertExceptionStackContainsCallingMethod(e); } if (errorStatusCode <= 500 || expectedIgnores.contains(errorStatusCode)) { failureListener.assertNotCalled(); @@ -309,6 +330,7 @@ public void testBody() throws IOException { Response response = e.getResponse(); assertThat(response.getStatusLine().getStatusCode(), equalTo(errorStatusCode)); assertThat(EntityUtils.toString(response.getEntity()), equalTo(body)); + assertExceptionStackContainsCallingMethod(e); } } } diff --git a/client/rest/src/test/java/org/elasticsearch/client/RestClientTests.java b/client/rest/src/test/java/org/elasticsearch/client/RestClientTests.java index dd3a88f53513b..33323d39663e2 100644 --- a/client/rest/src/test/java/org/elasticsearch/client/RestClientTests.java +++ b/client/rest/src/test/java/org/elasticsearch/client/RestClientTests.java @@ -26,8 +26,11 @@ import java.io.IOException; import java.net.URI; import java.util.Collections; +import java.util.concurrent.CountDownLatch; +import static org.hamcrest.Matchers.instanceOf; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThat; import static org.junit.Assert.fail; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; @@ -48,50 +51,83 @@ public void testCloseIsIdempotent() throws IOException { } public void testPerformAsyncWithUnsupportedMethod() throws Exception { - RestClient.SyncResponseListener listener = new RestClient.SyncResponseListener(10000); + final CountDownLatch latch = new CountDownLatch(1); try (RestClient restClient = createRestClient()) { - restClient.performRequestAsync("unsupported", randomAsciiLettersOfLength(5), listener); - listener.get(); - - fail("should have failed because of unsupported method"); - } catch (UnsupportedOperationException exception) { - assertEquals("http method not supported: unsupported", exception.getMessage()); + restClient.performRequestAsync("unsupported", randomAsciiLettersOfLength(5), new ResponseListener() { + @Override + public void onSuccess(Response response) { + fail("should have failed because of unsupported method"); + } + + @Override + public void onFailure(Exception exception) { + assertThat(exception, instanceOf(UnsupportedOperationException.class)); + assertEquals("http method not supported: unsupported", exception.getMessage()); + latch.countDown(); + } + }); + latch.await(); } } public void testPerformAsyncWithNullParams() throws Exception { - RestClient.SyncResponseListener listener = new RestClient.SyncResponseListener(10000); + final CountDownLatch latch = new CountDownLatch(1); try (RestClient restClient = createRestClient()) { - restClient.performRequestAsync(randomAsciiLettersOfLength(5), randomAsciiLettersOfLength(5), null, listener); - listener.get(); - - fail("should have failed because of null parameters"); - } catch (NullPointerException exception) { - assertEquals("params must not be null", exception.getMessage()); + restClient.performRequestAsync(randomAsciiLettersOfLength(5), randomAsciiLettersOfLength(5), null, new ResponseListener() { + @Override + public void onSuccess(Response response) { + fail("should have failed because of null parameters"); + } + + @Override + public void onFailure(Exception exception) { + assertThat(exception, instanceOf(NullPointerException.class)); + assertEquals("params must not be null", exception.getMessage()); + latch.countDown(); + } + }); + latch.await(); } } public void testPerformAsyncWithNullHeaders() throws Exception { - RestClient.SyncResponseListener listener = new RestClient.SyncResponseListener(10000); + final CountDownLatch latch = new CountDownLatch(1); try (RestClient restClient = createRestClient()) { + ResponseListener listener = new ResponseListener() { + @Override + public void onSuccess(Response response) { + fail("should have failed because of null headers"); + } + + @Override + public void onFailure(Exception exception) { + assertThat(exception, instanceOf(NullPointerException.class)); + assertEquals("request header must not be null", exception.getMessage()); + latch.countDown(); + } + }; restClient.performRequestAsync("GET", randomAsciiLettersOfLength(5), listener, (Header) null); - listener.get(); - - fail("should have failed because of null headers"); - } catch (NullPointerException exception) { - assertEquals("request header must not be null", exception.getMessage()); + latch.await(); } } public void testPerformAsyncWithWrongEndpoint() throws Exception { - RestClient.SyncResponseListener listener = new RestClient.SyncResponseListener(10000); + final CountDownLatch latch = new CountDownLatch(1); try (RestClient restClient = createRestClient()) { - restClient.performRequestAsync("GET", "::http:///", listener); - listener.get(); - - fail("should have failed because of wrong endpoint"); - } catch (IllegalArgumentException exception) { - assertEquals("Expected scheme name at index 0: ::http:///", exception.getMessage()); + restClient.performRequestAsync("GET", "::http:///", new ResponseListener() { + @Override + public void onSuccess(Response response) { + fail("should have failed because of wrong endpoint"); + } + + @Override + public void onFailure(Exception exception) { + assertThat(exception, instanceOf(IllegalArgumentException.class)); + assertEquals("Expected scheme name at index 0: ::http:///", exception.getMessage()); + latch.countDown(); + } + }); + latch.await(); } } diff --git a/client/rest/src/test/java/org/elasticsearch/client/SyncResponseListenerTests.java b/client/rest/src/test/java/org/elasticsearch/client/SyncResponseListenerTests.java index 154efb4cac34b..78e993a176244 100644 --- a/client/rest/src/test/java/org/elasticsearch/client/SyncResponseListenerTests.java +++ b/client/rest/src/test/java/org/elasticsearch/client/SyncResponseListenerTests.java @@ -24,11 +24,15 @@ import org.apache.http.ProtocolVersion; import org.apache.http.RequestLine; import org.apache.http.StatusLine; +import org.apache.http.conn.ConnectTimeoutException; import org.apache.http.message.BasicHttpResponse; import org.apache.http.message.BasicRequestLine; import org.apache.http.message.BasicStatusLine; import java.io.IOException; +import java.io.PrintWriter; +import java.io.StringWriter; +import java.net.SocketTimeoutException; import java.net.URISyntaxException; import static org.junit.Assert.assertEquals; @@ -37,6 +41,25 @@ import static org.junit.Assert.fail; public class SyncResponseListenerTests extends RestClientTestCase { + static void assertExceptionStackContainsCallingMethod(Exception e) { + boolean foundMyMethod = false; + // 0 is getStackTrace + // 1 is this method + // 2 is the caller, what we want + StackTraceElement myMethod = Thread.currentThread().getStackTrace()[2]; + for (StackTraceElement se : e.getStackTrace()) { + if (se.getClassName().equals(myMethod.getClassName()) + && se.getMethodName().equals(myMethod.getMethodName())) { + foundMyMethod = true; + break; + } + } + if (false == foundMyMethod) { + StringWriter stack = new StringWriter(); + e.printStackTrace(new PrintWriter(stack)); + fail("didn't find my stack trace (looks like " + myMethod + ") in\n" + stack); + } + } public void testOnSuccessNullResponse() { RestClient.SyncResponseListener syncResponseListener = new RestClient.SyncResponseListener(10000); @@ -73,18 +96,6 @@ public void testOnSuccess() throws Exception { } response = syncResponseListener.get(); assertSame(response, mockResponse); - - RuntimeException runtimeException = new RuntimeException("test"); - syncResponseListener.onFailure(runtimeException); - try { - syncResponseListener.get(); - fail("get should have failed"); - } catch(IllegalStateException e) { - assertEquals("response and exception are unexpectedly set at the same time", e.getMessage()); - assertNotNull(e.getSuppressed()); - assertEquals(1, e.getSuppressed().length); - assertSame(runtimeException, e.getSuppressed()[0]); - } } public void testOnFailure() throws Exception { @@ -95,7 +106,8 @@ public void testOnFailure() throws Exception { syncResponseListener.get(); fail("get should have failed"); } catch(RuntimeException e) { - assertSame(firstException, e); + assertEquals(firstException.getMessage(), e.getMessage()); + assertSame(firstException, e.getCause()); } RuntimeException secondException = new RuntimeException("second-test"); @@ -108,7 +120,8 @@ public void testOnFailure() throws Exception { syncResponseListener.get(); fail("get should have failed"); } catch(RuntimeException e) { - assertSame(firstException, e); + assertEquals(firstException.getMessage(), e.getMessage()); + assertSame(firstException, e.getCause()); } Response response = mockResponse(); @@ -124,7 +137,7 @@ public void testOnFailure() throws Exception { } } - public void testRuntimeExceptionIsNotWrapped() throws Exception { + public void testRuntimeIsBuiltCorrectly() throws Exception { RestClient.SyncResponseListener syncResponseListener = new RestClient.SyncResponseListener(10000); RuntimeException runtimeException = new RuntimeException(); syncResponseListener.onFailure(runtimeException); @@ -132,11 +145,50 @@ public void testRuntimeExceptionIsNotWrapped() throws Exception { syncResponseListener.get(); fail("get should have failed"); } catch(RuntimeException e) { - assertSame(runtimeException, e); + // We preserve the original exception in the cause + assertSame(runtimeException, e.getCause()); + // We copy the message + assertEquals(runtimeException.getMessage(), e.getMessage()); + // And we do all that so the thrown exception has our method in the stacktrace + assertExceptionStackContainsCallingMethod(e); + } + } + + public void testConnectTimeoutExceptionIsBuiltCorrectly() throws Exception { + RestClient.SyncResponseListener syncResponseListener = new RestClient.SyncResponseListener(10000); + ConnectTimeoutException timeoutException = new ConnectTimeoutException(); + syncResponseListener.onFailure(timeoutException); + try { + syncResponseListener.get(); + fail("get should have failed"); + } catch(IOException e) { + // We preserve the original exception in the cause + assertSame(timeoutException, e.getCause()); + // We copy the message + assertEquals(timeoutException.getMessage(), e.getMessage()); + // And we do all that so the thrown exception has our method in the stacktrace + assertExceptionStackContainsCallingMethod(e); + } + } + + public void testSocketTimeoutExceptionIsBuiltCorrectly() throws Exception { + RestClient.SyncResponseListener syncResponseListener = new RestClient.SyncResponseListener(10000); + SocketTimeoutException timeoutException = new SocketTimeoutException(); + syncResponseListener.onFailure(timeoutException); + try { + syncResponseListener.get(); + fail("get should have failed"); + } catch(IOException e) { + // We preserve the original exception in the cause + assertSame(timeoutException, e.getCause()); + // We copy the message + assertEquals(timeoutException.getMessage(), e.getMessage()); + // And we do all that so the thrown exception has our method in the stacktrace + assertExceptionStackContainsCallingMethod(e); } } - public void testIOExceptionIsNotWrapped() throws Exception { + public void testIOExceptionIsBuiltCorrectly() throws Exception { RestClient.SyncResponseListener syncResponseListener = new RestClient.SyncResponseListener(10000); IOException ioException = new IOException(); syncResponseListener.onFailure(ioException); @@ -144,7 +196,12 @@ public void testIOExceptionIsNotWrapped() throws Exception { syncResponseListener.get(); fail("get should have failed"); } catch(IOException e) { - assertSame(ioException, e); + // We preserve the original exception in the cause + assertSame(ioException, e.getCause()); + // We copy the message + assertEquals(ioException.getMessage(), e.getMessage()); + // And we do all that so the thrown exception has our method in the stacktrace + assertExceptionStackContainsCallingMethod(e); } } @@ -158,7 +215,10 @@ public void testExceptionIsWrapped() throws Exception { fail("get should have failed"); } catch(RuntimeException e) { assertEquals("error while performing request", e.getMessage()); + // We preserve the original exception in the cause assertSame(exception, e.getCause()); + // And we do all that so the thrown exception has our method in the stacktrace + assertExceptionStackContainsCallingMethod(e); } } From d823914f438fed9f3dcf0c238dac4256dce68493 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Thu, 8 Mar 2018 12:23:36 -0800 Subject: [PATCH 2/9] Fixes from comments --- .../RestClientSingleHostIntegTests.java | 4 +- .../client/SyncResponseListenerTests.java | 43 +++++++++++-------- 2 files changed, 26 insertions(+), 21 deletions(-) diff --git a/client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostIntegTests.java b/client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostIntegTests.java index 3d282a642e0da..f83276b1452bc 100644 --- a/client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostIntegTests.java +++ b/client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostIntegTests.java @@ -180,7 +180,7 @@ public void testHeaders() throws IOException { Response esResponse; try { esResponse = restClient.performRequest(method, "/" + statusCode, Collections.emptyMap(), requestHeaders); - } catch(ResponseException e) { + } catch (ResponseException e) { esResponse = e.getResponse(); } @@ -292,7 +292,7 @@ private Response bodyTest(final RestClient restClient, final String method) thro Response esResponse; try { esResponse = restClient.performRequest(method, "/" + statusCode, Collections.emptyMap(), entity); - } catch(ResponseException e) { + } catch (ResponseException e) { esResponse = e.getResponse(); } assertEquals(method, esResponse.getRequestLine().getMethod()); diff --git a/client/rest/src/test/java/org/elasticsearch/client/SyncResponseListenerTests.java b/client/rest/src/test/java/org/elasticsearch/client/SyncResponseListenerTests.java index 78e993a176244..6d504ba52aa49 100644 --- a/client/rest/src/test/java/org/elasticsearch/client/SyncResponseListenerTests.java +++ b/client/rest/src/test/java/org/elasticsearch/client/SyncResponseListenerTests.java @@ -41,8 +41,16 @@ import static org.junit.Assert.fail; public class SyncResponseListenerTests extends RestClientTestCase { + /** + * Asserts that the provided {@linkplain Exception} contains the method + * that called this somewhere on its stack. This is + * normally the case for synchronous calls but {@link RestClient} performs + * synchronous calls by performing asynchronous calls and blocking the + * current thread until the call returns so it has to take special care + * to make sure that the caller shows up in the exception. We use this + * assertion to make sure that we don't break that "special care". + */ static void assertExceptionStackContainsCallingMethod(Exception e) { - boolean foundMyMethod = false; // 0 is getStackTrace // 1 is this method // 2 is the caller, what we want @@ -50,15 +58,12 @@ static void assertExceptionStackContainsCallingMethod(Exception e) { for (StackTraceElement se : e.getStackTrace()) { if (se.getClassName().equals(myMethod.getClassName()) && se.getMethodName().equals(myMethod.getMethodName())) { - foundMyMethod = true; - break; + return; } } - if (false == foundMyMethod) { - StringWriter stack = new StringWriter(); - e.printStackTrace(new PrintWriter(stack)); - fail("didn't find my stack trace (looks like " + myMethod + ") in\n" + stack); - } + StringWriter stack = new StringWriter(); + e.printStackTrace(new PrintWriter(stack)); + fail("didn't find the calling method (looks like " + myMethod + ") in:\n" + stack); } public void testOnSuccessNullResponse() { @@ -66,7 +71,7 @@ public void testOnSuccessNullResponse() { try { syncResponseListener.onSuccess(null); fail("onSuccess should have failed"); - } catch(NullPointerException e) { + } catch (NullPointerException e) { assertEquals("response must not be null", e.getMessage()); } } @@ -76,7 +81,7 @@ public void testOnFailureNullException() { try { syncResponseListener.onFailure(null); fail("onFailure should have failed"); - } catch(NullPointerException e) { + } catch (NullPointerException e) { assertEquals("exception must not be null", e.getMessage()); } } @@ -91,7 +96,7 @@ public void testOnSuccess() throws Exception { try { syncResponseListener.onSuccess(mockResponse); fail("get should have failed"); - } catch(IllegalStateException e) { + } catch (IllegalStateException e) { assertEquals(e.getMessage(), "response is already set"); } response = syncResponseListener.get(); @@ -105,7 +110,7 @@ public void testOnFailure() throws Exception { try { syncResponseListener.get(); fail("get should have failed"); - } catch(RuntimeException e) { + } catch (RuntimeException e) { assertEquals(firstException.getMessage(), e.getMessage()); assertSame(firstException, e.getCause()); } @@ -119,7 +124,7 @@ public void testOnFailure() throws Exception { try { syncResponseListener.get(); fail("get should have failed"); - } catch(RuntimeException e) { + } catch (RuntimeException e) { assertEquals(firstException.getMessage(), e.getMessage()); assertSame(firstException, e.getCause()); } @@ -129,7 +134,7 @@ public void testOnFailure() throws Exception { try { syncResponseListener.get(); fail("get should have failed"); - } catch(IllegalStateException e) { + } catch (IllegalStateException e) { assertEquals("response and exception are unexpectedly set at the same time", e.getMessage()); assertNotNull(e.getSuppressed()); assertEquals(1, e.getSuppressed().length); @@ -144,7 +149,7 @@ public void testRuntimeIsBuiltCorrectly() throws Exception { try { syncResponseListener.get(); fail("get should have failed"); - } catch(RuntimeException e) { + } catch (RuntimeException e) { // We preserve the original exception in the cause assertSame(runtimeException, e.getCause()); // We copy the message @@ -161,7 +166,7 @@ public void testConnectTimeoutExceptionIsBuiltCorrectly() throws Exception { try { syncResponseListener.get(); fail("get should have failed"); - } catch(IOException e) { + } catch (IOException e) { // We preserve the original exception in the cause assertSame(timeoutException, e.getCause()); // We copy the message @@ -178,7 +183,7 @@ public void testSocketTimeoutExceptionIsBuiltCorrectly() throws Exception { try { syncResponseListener.get(); fail("get should have failed"); - } catch(IOException e) { + } catch (IOException e) { // We preserve the original exception in the cause assertSame(timeoutException, e.getCause()); // We copy the message @@ -195,7 +200,7 @@ public void testIOExceptionIsBuiltCorrectly() throws Exception { try { syncResponseListener.get(); fail("get should have failed"); - } catch(IOException e) { + } catch (IOException e) { // We preserve the original exception in the cause assertSame(ioException, e.getCause()); // We copy the message @@ -213,7 +218,7 @@ public void testExceptionIsWrapped() throws Exception { try { syncResponseListener.get(); fail("get should have failed"); - } catch(RuntimeException e) { + } catch (RuntimeException e) { assertEquals("error while performing request", e.getMessage()); // We preserve the original exception in the cause assertSame(exception, e.getCause()); From fdabee2b8603fa0b4585f2973b5d10ae5ec1a54a Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Thu, 8 Mar 2018 12:25:28 -0800 Subject: [PATCH 3/9] another space --- .../elasticsearch/client/RestClientMultipleHostsIntegTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/rest/src/test/java/org/elasticsearch/client/RestClientMultipleHostsIntegTests.java b/client/rest/src/test/java/org/elasticsearch/client/RestClientMultipleHostsIntegTests.java index da5a960c2e84c..793e3ca2637d4 100644 --- a/client/rest/src/test/java/org/elasticsearch/client/RestClientMultipleHostsIntegTests.java +++ b/client/rest/src/test/java/org/elasticsearch/client/RestClientMultipleHostsIntegTests.java @@ -144,7 +144,7 @@ public void testSyncRequests() throws IOException { Response response; try { response = restClient.performRequest(method, "/" + statusCode); - } catch(ResponseException responseException) { + } catch (ResponseException responseException) { response = responseException.getResponse(); } assertEquals(method, response.getRequestLine().getMethod()); From 966bddecf6f5ca7433a8a9d18241106feba823b5 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Thu, 8 Mar 2018 12:27:04 -0800 Subject: [PATCH 4/9] More catch ( Trying to only touch the files i touched. --- .../client/RestClientMultipleHostsTests.java | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/client/rest/src/test/java/org/elasticsearch/client/RestClientMultipleHostsTests.java b/client/rest/src/test/java/org/elasticsearch/client/RestClientMultipleHostsTests.java index 88a62026a0665..a3a834ff3204b 100644 --- a/client/rest/src/test/java/org/elasticsearch/client/RestClientMultipleHostsTests.java +++ b/client/rest/src/test/java/org/elasticsearch/client/RestClientMultipleHostsTests.java @@ -159,7 +159,7 @@ public void testRoundRobinNoRetryErrors() throws IOException { } else { fail("request should have failed"); } - } catch(ResponseException e) { + } catch (ResponseException e) { if (method.equals("HEAD") && statusCode == 404) { throw e; } @@ -179,7 +179,7 @@ public void testRoundRobinRetryErrors() throws IOException { try { restClient.performRequest(randomHttpMethod(getRandom()), retryEndpoint); fail("request should have failed"); - } catch(ResponseException e) { + } catch (ResponseException e) { /* * Unwrap the top level failure that was added so the stack trace contains * the caller. It wraps the exception that contains the failed hosts. @@ -204,7 +204,7 @@ public void testRoundRobinRetryErrors() throws IOException { } } while(e != null); assertEquals("every host should have been used but some weren't: " + hostsSet, 0, hostsSet.size()); - } catch(IOException e) { + } catch (IOException e) { /* * Unwrap the top level failure that was added so the stack trace contains * the caller. It wraps the exception that contains the failed hosts. @@ -239,7 +239,7 @@ public void testRoundRobinRetryErrors() throws IOException { try { restClient.performRequest(randomHttpMethod(getRandom()), retryEndpoint); fail("request should have failed"); - } catch(ResponseException e) { + } catch (ResponseException e) { Response response = e.getResponse(); assertThat(response.getStatusLine().getStatusCode(), equalTo(Integer.parseInt(retryEndpoint.substring(1)))); assertTrue("host [" + response.getHost() + "] not found, most likely used multiple times", @@ -247,7 +247,7 @@ public void testRoundRobinRetryErrors() throws IOException { //after the first request, all hosts are blacklisted, a single one gets resurrected each time failureListener.assertCalled(response.getHost()); assertEquals(0, e.getSuppressed().length); - } catch(IOException e) { + } catch (IOException e) { /* * Unwrap the top level failure that was added so the stack trace contains * the caller. It wraps the exception that contains the failed hosts. @@ -270,8 +270,7 @@ public void testRoundRobinRetryErrors() throws IOException { Response response; try { response = restClient.performRequest(randomHttpMethod(getRandom()), "/" + statusCode); - } - catch(ResponseException e) { + } catch (ResponseException e) { response = e.getResponse(); } assertThat(response.getStatusLine().getStatusCode(), equalTo(statusCode)); @@ -289,7 +288,7 @@ public void testRoundRobinRetryErrors() throws IOException { try { restClient.performRequest(randomHttpMethod(getRandom()), retryEndpoint); fail("request should have failed"); - } catch(ResponseException e) { + } catch (ResponseException e) { Response response = e.getResponse(); assertThat(response.getStatusLine().getStatusCode(), equalTo(Integer.parseInt(retryEndpoint.substring(1)))); assertThat(response.getHost(), equalTo(selectedHost)); From 17c10f026e0cf3c914e0b9342aeb367815087906 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Thu, 8 Mar 2018 12:29:21 -0800 Subject: [PATCH 5/9] Undo changes on files commit doesnt' touch --- .../client/RestClientMultipleHostsIntegTests.java | 2 +- .../elasticsearch/client/RestClientSingleHostIntegTests.java | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/client/rest/src/test/java/org/elasticsearch/client/RestClientMultipleHostsIntegTests.java b/client/rest/src/test/java/org/elasticsearch/client/RestClientMultipleHostsIntegTests.java index 793e3ca2637d4..da5a960c2e84c 100644 --- a/client/rest/src/test/java/org/elasticsearch/client/RestClientMultipleHostsIntegTests.java +++ b/client/rest/src/test/java/org/elasticsearch/client/RestClientMultipleHostsIntegTests.java @@ -144,7 +144,7 @@ public void testSyncRequests() throws IOException { Response response; try { response = restClient.performRequest(method, "/" + statusCode); - } catch (ResponseException responseException) { + } catch(ResponseException responseException) { response = responseException.getResponse(); } assertEquals(method, response.getRequestLine().getMethod()); diff --git a/client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostIntegTests.java b/client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostIntegTests.java index f83276b1452bc..3d282a642e0da 100644 --- a/client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostIntegTests.java +++ b/client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostIntegTests.java @@ -180,7 +180,7 @@ public void testHeaders() throws IOException { Response esResponse; try { esResponse = restClient.performRequest(method, "/" + statusCode, Collections.emptyMap(), requestHeaders); - } catch (ResponseException e) { + } catch(ResponseException e) { esResponse = e.getResponse(); } @@ -292,7 +292,7 @@ private Response bodyTest(final RestClient restClient, final String method) thro Response esResponse; try { esResponse = restClient.performRequest(method, "/" + statusCode, Collections.emptyMap(), entity); - } catch (ResponseException e) { + } catch(ResponseException e) { esResponse = e.getResponse(); } assertEquals(method, esResponse.getRequestLine().getMethod()); From b5fa54c8526a0f55b3a641a982ab3d202273121d Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Thu, 15 Mar 2018 16:13:38 -0400 Subject: [PATCH 6/9] Javadoc --- .../main/java/org/elasticsearch/client/RestClient.java | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/client/rest/src/main/java/org/elasticsearch/client/RestClient.java b/client/rest/src/main/java/org/elasticsearch/client/RestClient.java index 269e5f732f8cf..addd1e05ff1f3 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/RestClient.java +++ b/client/rest/src/main/java/org/elasticsearch/client/RestClient.java @@ -203,6 +203,14 @@ public Response performRequest(String method, String endpoint, Map Date: Thu, 15 Mar 2018 20:23:46 -0400 Subject: [PATCH 7/9] WIP --- .../src/main/java/org/elasticsearch/client/RestClient.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/client/rest/src/main/java/org/elasticsearch/client/RestClient.java b/client/rest/src/main/java/org/elasticsearch/client/RestClient.java index addd1e05ff1f3..6a9422ef5645c 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/RestClient.java +++ b/client/rest/src/main/java/org/elasticsearch/client/RestClient.java @@ -20,6 +20,7 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.apache.http.ConnectionClosedException; import org.apache.http.Header; import org.apache.http.HttpEntity; import org.apache.http.HttpHost; @@ -711,6 +712,11 @@ Response get() throws IOException { e.initCause(exception); throw e; } + if (exception instanceof ConnectionClosedException) { + ConnectionClosedException e = new ConnectionClosedException(exception.getMessage()); + e.initCause(exception); + throw e; + } if (exception instanceof IOException) { throw new IOException(exception.getMessage(), exception); } From 7212e84f74eee388684e72987769146932f2e12c Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Fri, 16 Mar 2018 10:05:52 -0400 Subject: [PATCH 8/9] Better words --- .../src/main/java/org/elasticsearch/client/RestClient.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/rest/src/main/java/org/elasticsearch/client/RestClient.java b/client/rest/src/main/java/org/elasticsearch/client/RestClient.java index 6a9422ef5645c..29e23f948bddb 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/RestClient.java +++ b/client/rest/src/main/java/org/elasticsearch/client/RestClient.java @@ -204,8 +204,8 @@ public Response performRequest(String method, String endpoint, Map Date: Fri, 16 Mar 2018 10:06:00 -0400 Subject: [PATCH 9/9] Test for wrapping exception --- .../client/SyncResponseListenerTests.java | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/client/rest/src/test/java/org/elasticsearch/client/SyncResponseListenerTests.java b/client/rest/src/test/java/org/elasticsearch/client/SyncResponseListenerTests.java index 6d504ba52aa49..f9406a6c4902d 100644 --- a/client/rest/src/test/java/org/elasticsearch/client/SyncResponseListenerTests.java +++ b/client/rest/src/test/java/org/elasticsearch/client/SyncResponseListenerTests.java @@ -19,6 +19,7 @@ package org.elasticsearch.client; +import org.apache.http.ConnectionClosedException; import org.apache.http.HttpHost; import org.apache.http.HttpResponse; import org.apache.http.ProtocolVersion; @@ -193,6 +194,23 @@ public void testSocketTimeoutExceptionIsBuiltCorrectly() throws Exception { } } + public void testConnectionClosedExceptionIsWrapped() throws Exception { + RestClient.SyncResponseListener syncResponseListener = new RestClient.SyncResponseListener(10000); + ConnectionClosedException closedException = new ConnectionClosedException(randomAsciiAlphanumOfLength(5)); + syncResponseListener.onFailure(closedException); + try { + syncResponseListener.get(); + fail("get should have failed"); + } catch (ConnectionClosedException e) { + // We preserve the original exception in the cause + assertSame(closedException, e.getCause()); + // We copy the message + assertEquals(closedException.getMessage(), e.getMessage()); + // And we do all that so the thrown exception has our method in the stacktrace + assertExceptionStackContainsCallingMethod(e); + } + } + public void testIOExceptionIsBuiltCorrectly() throws Exception { RestClient.SyncResponseListener syncResponseListener = new RestClient.SyncResponseListener(10000); IOException ioException = new IOException();