Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Properly wait for entire result in JettyClientCall #1101

Open
wants to merge 1 commit into
base: 2.3
Choose a base branch
from
Open

Properly wait for entire result in JettyClientCall #1101

wants to merge 1 commit into from

Commits on Jun 16, 2015

  1. 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`.
    Chad Gatesman committed Jun 16, 2015
    Configuration menu
    Copy the full SHA
    c58d5e4 View commit details
    Browse the repository at this point in the history