Allow retries to be enabled on default Apache HTTP client

[ksola]

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?

[ptrthomas]

@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.

[ksola]

@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.

[ptrthomas]

@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

[ksola]

@ptrthomas I created the PR: #2409

[ptrthomas]

1.4.1 released