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..29e23f948bddb 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; @@ -38,6 +39,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 +49,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; @@ -201,6 +204,14 @@ 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 +693,35 @@ 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 ConnectionClosedException) { + ConnectionClosedException e = new ConnectionClosedException(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..a3a834ff3204b 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++) { @@ -142,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; } @@ -162,7 +179,12 @@ 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. + */ + 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 @@ -182,7 +204,12 @@ 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. + */ + 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 @@ -212,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", @@ -220,7 +247,12 @@ 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. + */ + 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 @@ -238,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)); @@ -257,12 +288,17 @@ 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)); 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..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,16 +19,21 @@ package org.elasticsearch.client; +import org.apache.http.ConnectionClosedException; import org.apache.http.HttpHost; import org.apache.http.HttpResponse; 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,13 +42,37 @@ 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) { + // 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())) { + return; + } + } + 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() { RestClient.SyncResponseListener syncResponseListener = new RestClient.SyncResponseListener(10000); try { syncResponseListener.onSuccess(null); fail("onSuccess should have failed"); - } catch(NullPointerException e) { + } catch (NullPointerException e) { assertEquals("response must not be null", e.getMessage()); } } @@ -53,7 +82,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()); } } @@ -68,23 +97,11 @@ 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(); 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 { @@ -94,8 +111,9 @@ public void testOnFailure() throws Exception { try { syncResponseListener.get(); fail("get should have failed"); - } catch(RuntimeException e) { - assertSame(firstException, e); + } catch (RuntimeException e) { + assertEquals(firstException.getMessage(), e.getMessage()); + assertSame(firstException, e.getCause()); } RuntimeException secondException = new RuntimeException("second-test"); @@ -107,8 +125,9 @@ public void testOnFailure() throws Exception { try { syncResponseListener.get(); fail("get should have failed"); - } catch(RuntimeException e) { - assertSame(firstException, e); + } catch (RuntimeException e) { + assertEquals(firstException.getMessage(), e.getMessage()); + assertSame(firstException, e.getCause()); } Response response = mockResponse(); @@ -116,7 +135,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); @@ -124,27 +143,88 @@ 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); try { syncResponseListener.get(); fail("get should have failed"); - } catch(RuntimeException e) { - assertSame(runtimeException, e); + } catch (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 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 testIOExceptionIsNotWrapped() throws Exception { + public void testIOExceptionIsBuiltCorrectly() throws Exception { RestClient.SyncResponseListener syncResponseListener = new RestClient.SyncResponseListener(10000); IOException ioException = new IOException(); syncResponseListener.onFailure(ioException); try { syncResponseListener.get(); fail("get should have failed"); - } catch(IOException e) { - assertSame(ioException, e); + } catch (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); } } @@ -156,9 +236,12 @@ 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()); + // And we do all that so the thrown exception has our method in the stacktrace + assertExceptionStackContainsCallingMethod(e); } }