Allow retries to be enabled on default Apache HTTP client
ksola opened this issue · 5 comments
In our company we are using Karate test to monitor our production environment every 30 minutes. We encounter at least 5-10 times every week a failing test because of this exception:
org.apache.http.NoHttpResponseException: The target server failed to respond, http call failed after 5010 milliseconds for url:..
After reruning that test this exception does not occur anymore. It was not possible to reproduce that exception because this happen in random tests in our 30 minutes monitors from time to time. That's why it is hard to provide code which will reproduce the problem.
Luckily I found that this is a problem related with org.apache.httpcomponents:httpclient
which is not the best in handling steal connections. In stack overflow is a proposed solution: https://stackoverflow.com/a/10680629
I applied that solution from above stackoverflow to our monitoring tests and the problem does not occur anymore since 3 months.
The commit can be found here: master...ksola:karate:master
Is this fix worth to put int the master branch of karate?
@ksola PRs can go against the develop
branch.
the use of disableAutomaticRetries()
is intentional by default, else what happens is the logs get confusing when the http client goes off on its own and does retries. also as a best practice - teams should not depend on retries, and fix the root-cause.
by the way - are you aware of retry until
perhaps that solves your problem ? looking at your error message I suspect that karate.configure('readTimeout', 5000)
has been done, maybe all you need to do is increase that timeout ?
but if you feel this custom HttpRequestRetryHandler
is useful, I'm open to merging it. but I want it behind a configure
flag, and I propose the name: httpRetryEnabled
which defaults to false
.
@ptrthomas Thanks for the fast replay.
We have setup a timeout of 1 minute and anyway we get this exception.
Yes I know the retry until
but when the org.apache.http.NoHttpResponseException
is thrown the test just fail even there is a retry. I think there is no way to catch this exception in the karate tests as there is thrown a RuntimeException in case this exception is thrown (please correct me if I am wrong):
try (CloseableHttpClient client = clientBuilder.build()) {
httpResponse = client.execute(requestBuilder.build());
HttpEntity responseEntity = httpResponse.getEntity();
if (responseEntity == null || responseEntity.getContent() == null) {
bytes = Constants.ZERO_BYTES;
} else {
InputStream is = responseEntity.getContent();
bytes = FileUtils.toBytes(is);
}
request.setEndTime(System.currentTimeMillis());
httpResponse.close();
} catch (Exception e) {
if (e instanceof ClientProtocolException && e.getCause() != null) { // better error message
throw new RuntimeException(e.getCause());
} else {
throw new RuntimeException(e);
}
}
I will create a PR to make this retry optional as you suggested. My only concern is the naming of the filed. As we will retry on a concert exception then maybe the name should be httpRetryEnabledOnNoHttpResponseException
. This is only a suggestion but I am also open for httpRetryEnabled
if you say that is better.
@ksola you are right, retry until
applies only for successful http responses
please proceed with httpRetryEnabled
as it should apply to any http client exception that triggers the built-in automatic retries, although I have no idea what all exceptions are in scope here
@ptrthomas I created the PR: #2409
1.4.1 released