Properly wait for entire result in JettyClientCall #1101
Conversation
`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`.
|
Hi @cgatesman, thanks a lot for your contribution! Do you mind send us the signed Joint Copyright Assignement? |
|
Hi @cgatesman, we received the signed JCA, thanks. |
|
Hi @cgatesman, I've made some test with a client that downloads a file that weights 20kb, and it seems to lock. |
|
Sorry. I have been away on vacation... This is interesting, @thboileau. I'm not sure. I was going by Jetty's InputStreamResponseListener documentation. This original issue came up in our unit tests which were using the client to drive our REST API. We were originally unaware that the default client had changed from Apache HttpClient to Jetty's HttpClient after upgrading Restlet. Making this fix seemed to fix our particular issue in our unit tests. After submitting this fix, we switched back to the Apache HttpClient implementation which we had been using with the older version of Restlet, and decided we would look into possibly using the new Jetty's HttpClient implementation down the road when we upgrade Restlet again. So, I can't say if there is other issues around this client implementation as we haven't been using it after we ran into this initial problem. |
thboileau
force-pushed
the
2.3
branch
3 times, most recently
from
e4d4aad to
a72a924
Compare
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,JettyClientCallis incorrectly returning a 1001 error when it catchesExecutionException.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 sinceInputStreamResponseListener.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 theInputStreamResponseListener.get()to throwExecutionException.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 ofInputStreamResponseListener.get()to properly wait for the entire request/response cycle, and remove the catch forExecutionExceptionwhich was incorrectly setting theStatusto 1001 in these cases. Note,InputStreamResponseListener.await()does not throwExecutionException.NOTE: I did not see any Jetty extension related unit tests to contribute to. If there are, and I just missed them, please point them out to me.