Repository: searchbox-io/Jest
Pull request #404 (2016-10-17 12:52:39)

Fix hang on invalid response
Improve error checking when server sends invalid responses. 
This could cause JestHttpClient#executeAsync to hang.

You may not agree with throwing a gson JsonSyntaxException in AbstractAction#parseResponseBody, I did it this way because there are already some tests looking for this exception. Happy to change it to some other exception.



Patch:
diff --git a/jest-common/src/main/java/io/searchbox/action/AbstractAction.java b/jest-common/src/main/java/io/searchbox/action/AbstractAction.java
index 8310069b..dce475c3 100644
--- a/jest-common/src/main/java/io/searchbox/action/AbstractAction.java
+++ b/jest-common/src/main/java/io/searchbox/action/AbstractAction.java
@@ -3,8 +3,10 @@ package io.searchbox.action;
 import com.google.common.collect.LinkedHashMultimap;
 import com.google.common.collect.Multimap;
 import com.google.gson.Gson;
+import com.google.gson.JsonElement;
 import com.google.gson.JsonObject;
 import com.google.gson.JsonParser;
+import com.google.gson.JsonSyntaxException;
 import io.searchbox.annotations.JestId;
 import io.searchbox.client.JestResult;
 import io.searchbox.params.Parameters;
@@ -90,10 +92,16 @@ public abstract class AbstractAction<T extends JestResult> implements Action<T>
     }
 
     protected JsonObject parseResponseBody(String responseBody) {
-        if (responseBody != null && !responseBody.trim().isEmpty()) {
-            return new JsonParser().parse(responseBody).getAsJsonObject();
+        if (responseBody == null || responseBody.trim().isEmpty()) {
+            return new JsonObject();
+        }
+
+        JsonElement parsed = new JsonParser().parse(responseBody);
+        if (parsed.isJsonObject()) {
+            return parsed.getAsJsonObject();
+        } else {
+            throw new JsonSyntaxException("Response did not contain a JSON Object");
         }
-        return new JsonObject();
     }
 
     public static String getIdFromSource(Object source) {
diff --git a/jest-common/src/main/java/io/searchbox/core/Cat.java b/jest-common/src/main/java/io/searchbox/core/Cat.java
index 4c256f97..b554f727 100644
--- a/jest-common/src/main/java/io/searchbox/core/Cat.java
+++ b/jest-common/src/main/java/io/searchbox/core/Cat.java
@@ -1,8 +1,11 @@
 package io.searchbox.core;
 
 import com.google.gson.Gson;
+import com.google.gson.JsonElement;
 import com.google.gson.JsonObject;
 import com.google.gson.JsonParser;
+import com.google.gson.JsonSyntaxException;
+
 import io.searchbox.action.AbstractAction;
 import io.searchbox.action.AbstractMultiIndexActionBuilder;
 import io.searchbox.action.AbstractMultiTypeActionBuilder;
@@ -44,12 +47,18 @@ public class Cat extends AbstractAction<CatResult> {
 
     @Override
     protected JsonObject parseResponseBody(String responseBody) {
-        JsonObject result = new JsonObject();
-        if (responseBody != null && !responseBody.trim().isEmpty()) {
-            result.add(PATH_TO_RESULT, new JsonParser().parse(responseBody).getAsJsonArray());
+        if (responseBody == null || responseBody.trim().isEmpty()) {
+            return new JsonObject();
         }
 
-        return result;
+        JsonElement parsed = new JsonParser().parse(responseBody);
+        if (parsed.isJsonArray()) {
+            JsonObject result = new JsonObject();
+            result.add(PATH_TO_RESULT, parsed.getAsJsonArray());
+            return result;
+        } else {
+            throw new JsonSyntaxException("Cat response did not contain a JSON Array");
+        }
     }
 
     @Override
diff --git a/jest/src/main/java/io/searchbox/client/http/JestHttpClient.java b/jest/src/main/java/io/searchbox/client/http/JestHttpClient.java
index 395d4888..43db1b88 100755
--- a/jest/src/main/java/io/searchbox/client/http/JestHttpClient.java
+++ b/jest/src/main/java/io/searchbox/client/http/JestHttpClient.java
@@ -231,8 +231,10 @@ public class JestHttpClient extends AbstractJestClient {
             T jestResult = null;
             try {
                 jestResult = deserializeResponse(response, request, clientRequest);
-            } catch (IOException e) {
+            } catch (Exception e) {
                 failed(e);
+            } catch (Throwable t) {
+                failed(new Exception("Problem during request processing", t));
             }
             if (jestResult != null) resultHandler.completed(jestResult);
         }


Test patch:
diff --git a/jest-common/src/test/java/io/searchbox/action/AbstractActionTest.java b/jest-common/src/test/java/io/searchbox/action/AbstractActionTest.java
index 9d168e8c..2d7ee8e6 100644
--- a/jest-common/src/test/java/io/searchbox/action/AbstractActionTest.java
+++ b/jest-common/src/test/java/io/searchbox/action/AbstractActionTest.java
@@ -181,10 +181,15 @@ public class AbstractActionTest {
     }
 
     @Test(expected = JsonSyntaxException.class)
-    public void propagateExceptionWhenTheResponseIsNotJson() {
+    public void propagateExceptionWhenTheResponseIsNotJson1() {
         new DummyAction.Builder().build().parseResponseBody("401 Unauthorized");
     }
 
+    @Test(expected = JsonSyntaxException.class)
+    public void propagateExceptionWhenTheResponseIsNotJson2() {
+        new DummyAction.Builder().build().parseResponseBody("banana");
+    }
+
 
     @Test
     public void getSuccessIndexResult() {
@@ -262,4 +267,4 @@ public class AbstractActionTest {
         }
     }
 
-}
\ No newline at end of file
+}
diff --git a/jest/src/test/java/io/searchbox/client/http/FailingProxy.java b/jest/src/test/java/io/searchbox/client/http/FailingProxy.java
index 7569aee1..0d204ee6 100644
--- a/jest/src/test/java/io/searchbox/client/http/FailingProxy.java
+++ b/jest/src/test/java/io/searchbox/client/http/FailingProxy.java
@@ -1,6 +1,8 @@
 package io.searchbox.client.http;
 
+import java.io.IOException;
 import java.net.InetSocketAddress;
+import java.net.Socket;
 import java.nio.charset.Charset;
 
 import org.littleshoot.proxy.HttpFilters;
@@ -20,20 +22,40 @@ import io.netty.handler.codec.http.HttpResponseStatus;
 import io.netty.handler.codec.http.HttpVersion;
 
 public class FailingProxy {
-    public static final int PROXY_PORT = 54321;
-
     private final HttpProxyServer server;
 
-    public FailingProxy() {
+    private String errorContentType = "text/html";
+    private HttpResponseStatus errorStatus = new HttpResponseStatus(500, "This proxy always fails");
+    private String errorMessage = "<html>"
+            + "  <head><title>This proxy always fails</title></head>"
+            + "<body>"
+            + "  <h1>This proxy always fails</h1>"
+            + "</body>"
+            + "</html>";
+
+    public FailingProxy() throws IOException {
         final HttpProxyServerBootstrap bootstrap = DefaultHttpProxyServer
                 .bootstrap()
-                .withPort(PROXY_PORT)
+                .withPort(getUnusedPort())
                 .withTransparent(true)
                 .withFiltersSource(new FailingSourceAdapter())
                 ;
         server = bootstrap.start();
     }
 
+
+    public void setErrorStatus(final HttpResponseStatus errorStatus) {
+        this.errorStatus = errorStatus;
+    }
+
+    public void setErrorContentType(final String errorContentType) {
+        this.errorContentType = errorContentType;
+    }
+
+    public void setErrorMessage(final String errorMessage) {
+        this.errorMessage = errorMessage;
+    }
+
     public String getUrl() {
         final InetSocketAddress listenAddress = server.getListenAddress();
         final String host = listenAddress.getHostName();
@@ -45,14 +67,22 @@ public class FailingProxy {
         server.stop();
     }
 
-    private static class FailingSourceAdapter extends HttpFiltersSourceAdapter {
+    private static int getUnusedPort() throws IOException {
+        final Socket deadSocket = new Socket();
+        deadSocket.bind(null);
+        final int port = deadSocket.getLocalPort();
+        deadSocket.close();
+        return port;
+    }
+
+    private class FailingSourceAdapter extends HttpFiltersSourceAdapter {
         @Override
         public HttpFilters filterRequest(final HttpRequest originalRequest) {
             return new FailingFilterAdapter(originalRequest);
         }
     }
 
-    private static class FailingFilterAdapter extends HttpFiltersAdapter {
+    private class FailingFilterAdapter extends HttpFiltersAdapter {
 
         public FailingFilterAdapter(final HttpRequest originalRequest) {
             super(originalRequest);
@@ -61,18 +91,10 @@ public class FailingProxy {
         @Override
         public HttpResponse requestPre(final HttpObject httpObject) {
 
-            final HttpResponseStatus status = new HttpResponseStatus(500, "This proxy always fails");
-
-            final String message = "<html>"
-                    + "  <head><title>This proxy always fails</title></head>"
-                    + "<body>"
-                    + "  <h1>This proxy always fails</h1>"
-                    + "</body>"
-                    + "</html>";
-            ByteBuf buf = Unpooled.wrappedBuffer(message.getBytes(Charset.forName("UTF-8")));
+            ByteBuf buf = Unpooled.wrappedBuffer(errorMessage.getBytes(Charset.forName("UTF-8")));
 
-            final DefaultFullHttpResponse response = new DefaultFullHttpResponse(HttpVersion.HTTP_1_1, status, buf);
-            response.headers().set("Content-Type", "text/html");
+            final DefaultFullHttpResponse response = new DefaultFullHttpResponse(HttpVersion.HTTP_1_1, errorStatus, buf);
+            response.headers().set("Content-Type", errorContentType);
 
             return response;
         }
diff --git a/jest/src/test/java/io/searchbox/client/http/FailingProxyTest.java b/jest/src/test/java/io/searchbox/client/http/FailingProxyTest.java
index e5f5a148..2e5d2f18 100644
--- a/jest/src/test/java/io/searchbox/client/http/FailingProxyTest.java
+++ b/jest/src/test/java/io/searchbox/client/http/FailingProxyTest.java
@@ -6,9 +6,9 @@ import java.util.concurrent.Semaphore;
 import org.hamcrest.Matchers;
 import org.junit.After;
 import org.junit.Before;
-import org.junit.Ignore;
 import org.junit.Test;
 
+import io.netty.handler.codec.http.HttpResponseStatus;
 import io.searchbox.client.JestClientFactory;
 import io.searchbox.client.JestResult;
 import io.searchbox.client.JestResultHandler;
@@ -50,24 +50,17 @@ public class FailingProxyTest {
 
     @Test
     public void testWithFailingProxy() throws InterruptedException, IOException {
-        Exception exception = null;
-        try {
-            final JestResult result = client.execute(status);
-        } catch (Exception e) {
-            exception = e;
-        }
-        validateException(exception);
+        Exception exception = runSynchronously();
+        validateFailingProxyException(exception);
     }
 
     @Test
     public void testAsyncWithFailingProxy() throws InterruptedException, IOException {
-        final ResultHandler resultHandler = new ResultHandler();
-        client.executeAsync(status, resultHandler);
-        Exception exception = resultHandler.get();
-        validateException(exception);
+        Exception exception = runAsynchronously();
+        validateFailingProxyException(exception);
     }
 
-    private void validateException(final Exception e) {
+    private void validateFailingProxyException(final Exception e) {
         assertThat(e, is(not(Matchers.nullValue())));
         final String message = e.toString();
         assertThat(message, not(containsString("Use JsonReader.setLenient(true)")));
@@ -75,6 +68,47 @@ public class FailingProxyTest {
         assertThat(message, containsString("should be json"));
     }
 
+    @Test
+    public void testWithBrokenResponse() throws InterruptedException, IOException {
+        proxy.setErrorStatus(HttpResponseStatus.FORBIDDEN);
+        proxy.setErrorContentType("application/json");
+        proxy.setErrorMessage("banana");   // <-- this is not json at all!
+        Exception exception = runSynchronously();
+        validateBrokenResponseException(exception);
+    }
+
+    @Test
+    public void testAsyncWithBrokenResponse() throws InterruptedException, IOException {
+        proxy.setErrorStatus(HttpResponseStatus.FORBIDDEN);
+        proxy.setErrorContentType("application/json");
+        proxy.setErrorMessage("banana");   // <-- this is not json at all!
+        Exception exception = runAsynchronously();
+        validateBrokenResponseException(exception);
+    }
+
+    private void validateBrokenResponseException(final Exception e) {
+        assertThat(e, is(not(Matchers.nullValue())));
+        final String message = e.toString();
+        assertThat(message, not(containsString("Use JsonReader.setLenient(true)")));
+        assertThat(message, containsString("did not contain a JSON Object"));
+    }
+
+    private Exception runSynchronously() {
+        Exception exception = null;
+        try {
+            final JestResult result = client.execute(status);
+        } catch (Exception e) {
+            exception = e;
+        }
+        return exception;
+    }
+
+    private Exception runAsynchronously() throws InterruptedException {
+        final ResultHandler resultHandler = new ResultHandler();
+        client.executeAsync(status, resultHandler);
+        return resultHandler.get();
+    }
+
     private class ResultHandler implements JestResultHandler {
         private final Semaphore sema = new Semaphore(0);
         private Exception exception = null;