From c58d5e46a927cad5ffe31e8fc9c6eb030b96a304 Mon Sep 17 00:00:00 2001 From: Chad Gatesman Date: Tue, 16 Jun 2015 14:46:49 -0400 Subject: [PATCH] Properly wait for entire result in JettyClientCall `JettyClientCall.sendRequest()` is randomly reporting error status code responses (e.g. HTTP 401) as 1001 Communication Error. This is occurring, because it is using `org.eclipse.jetty.client.util.InputStreamResponseListener.get()` which is documented to only wait for the header response and not the entire content. `InputStreamResponseListener.await()` is documented to wait for the whole request/response cycle to be finished. Using `InputStreamResponseListener.get()` means that it may or may not get the failure information which is handled by a separate thread and latch. In addition to this problem, `JettyClientCall` is incorrectly returning a 1001 error when it catches `ExecutionException`. `InputStreamResponseListener.get()` is throwing this exception even if there is simply an HTTP error (300+ HTTP status codes). It could be argued that this is incorrect behavior on Jetty's part. It just so happens that since `InputStreamResponseListener.get()` is used, on an HTTP error code case, many times the code beats the resultLatch in getting the header response and the exception is not thrown. In some cases, the failure gets recorded in time (in a separate thread) resulting in the `InputStreamResponseListener.get()` to throw `ExecutionException`. In summary, anytime there is a failure HTTP status code on a call, there is a random chance for this to fail with 1001 Communication Error instead of the correct HTTP status code. The fix includes using `InputStreamResponseListener.await()` instead of `InputStreamResponseListener.get()` to properly wait for the entire request/response cycle, and remove the catch for `ExecutionException` which was incorrectly setting the `Status` to 1001 in these cases. Note, `InputStreamResponseListener.await()` does not throw `ExecutionException`. --- .../ext/jetty/internal/JettyClientCall.java | 35 ++++++++----------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/modules/org.restlet.ext.jetty/src/org/restlet/ext/jetty/internal/JettyClientCall.java b/modules/org.restlet.ext.jetty/src/org/restlet/ext/jetty/internal/JettyClientCall.java index 176db78c22..aec8101bc8 100644 --- a/modules/org.restlet.ext.jetty/src/org/restlet/ext/jetty/internal/JettyClientCall.java +++ b/modules/org.restlet.ext.jetty/src/org/restlet/ext/jetty/internal/JettyClientCall.java @@ -29,12 +29,12 @@ import java.io.OutputStream; import java.nio.channels.ReadableByteChannel; import java.nio.channels.WritableByteChannel; -import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import java.util.logging.Level; import org.eclipse.jetty.client.HttpRequest; +import org.eclipse.jetty.client.api.Result; import org.eclipse.jetty.client.util.InputStreamContentProvider; import org.eclipse.jetty.client.util.InputStreamResponseListener; import org.eclipse.jetty.http.HttpField; @@ -70,9 +70,9 @@ public class JettyClientCall extends ClientCall { private final HttpRequest httpRequest; /** - * The wrapped HTTP response. + * The wrapped HTTP result. */ - private volatile org.eclipse.jetty.client.api.Response httpResponse; + private volatile Result result; /** * The wrapped input stream response listener. @@ -128,7 +128,7 @@ public HttpRequest getHttpRequest() { * @return The HTTP response. */ public org.eclipse.jetty.client.api.Response getHttpResponse() { - return this.httpResponse; + return (this.result != null) ? this.result.getResponse() : null; } /** @@ -203,7 +203,7 @@ public Representation getResponseEntity(Response response) { */ @Override public Series
getResponseHeaders() { - final Series
result = super.getResponseHeaders(); + final Series
responseHeaders = super.getResponseHeaders(); if (!this.responseHeadersAdded) { final org.eclipse.jetty.client.api.Response httpResponse = getHttpResponse(); @@ -211,14 +211,14 @@ public Series
getResponseHeaders() { final HttpFields headers = httpResponse.getHeaders(); if (headers != null) { for (HttpField header : headers) - result.add(header.getName(), header.getValue()); + responseHeaders.add(header.getName(), header.getValue()); } } this.responseHeadersAdded = true; } - return result; + return responseHeaders; } /** @@ -253,7 +253,7 @@ public int getStatusCode() { */ @Override public Status sendRequest(Request request) { - Status result = null; + Status status = null; try { final Representation entity = request.getEntity(); @@ -282,41 +282,34 @@ public Status sendRequest(Request request) { // Ensure that the connection is active this.inputStreamResponseListener = new InputStreamResponseListener(); this.httpRequest.send(this.inputStreamResponseListener); - this.httpResponse = this.inputStreamResponseListener.get( + this.result = this.inputStreamResponseListener.await( clientHelper.getIdleTimeout(), TimeUnit.MILLISECONDS); - result = new Status(getStatusCode(), getReasonPhrase()); + status = new Status(getStatusCode(), this.result.getFailure(), getReasonPhrase()); } catch (IOException e) { this.clientHelper.getLogger().log(Level.WARNING, "An error occurred while reading the request entity.", e); - result = new Status(Status.CONNECTOR_ERROR_INTERNAL, e); + status = new Status(Status.CONNECTOR_ERROR_INTERNAL, e); // Release the connection getHttpRequest().abort(e); } catch (TimeoutException e) { this.clientHelper.getLogger().log(Level.WARNING, "The HTTP request timed out.", e); - result = new Status(Status.CONNECTOR_ERROR_COMMUNICATION, e); + status = new Status(Status.CONNECTOR_ERROR_COMMUNICATION, e); // Release the connection getHttpRequest().abort(e); } catch (InterruptedException e) { this.clientHelper.getLogger().log(Level.WARNING, "The HTTP request thread was interrupted.", e); - result = new Status(Status.CONNECTOR_ERROR_COMMUNICATION, e); - - // Release the connection - getHttpRequest().abort(e); - } catch (ExecutionException e) { - this.clientHelper.getLogger().log(Level.WARNING, - "An error occurred while processing the HTTP request.", e); - result = new Status(Status.CONNECTOR_ERROR_COMMUNICATION, e); + status = new Status(Status.CONNECTOR_ERROR_COMMUNICATION, e); // Release the connection getHttpRequest().abort(e); } - return result; + return status; } @Override