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.

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

Allow retries to be enabled on default Apache HTTP client #2408

Closed
ksola opened this issue Sep 29, 2023 · 5 comments
Closed

Allow retries to be enabled on default Apache HTTP client #2408

ksola opened this issue Sep 29, 2023 · 5 comments

Comments

@ksola
Copy link

ksola commented Sep 29, 2023

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
Copy link
Member

ptrthomas commented Sep 29, 2023

@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 ptrthomas changed the title Randomly thrown exception in karate test: org.apache.http.NoHttpResponseException: The target server failed to respond, http call failed after 5010 milliseconds for url: ... Allow retries to be enabled on default Apache HTTP client Sep 29, 2023
@ksola
Copy link
Author

ksola commented Oct 2, 2023

@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
Copy link
Member

ptrthomas commented Oct 2, 2023

@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
Copy link
Author

ksola commented Oct 2, 2023

@ptrthomas I created the PR: #2409

@ptrthomas ptrthomas added the fixed label Oct 2, 2023
@ptrthomas ptrthomas added this to the 1.4.1 milestone Oct 2, 2023
@ptrthomas
Copy link
Member

1.4.1 released

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants