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

2296 upgrade to apache httpclient 5 #2471

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

f-delahaye
Copy link
Contributor

Description

Upgrade to httpclient5.

  • Relevant Issues : (2296)
  • Relevant PRs : (optional)
  • Type of change :
    • New feature
    • Bug fix for existing feature
    • Code quality improvement
    • Addition or Improvement of tests
    • Addition or Improvement of documentation

@ptrthomas
Copy link
Member

tagging @mj11pyqe - if possible can you review the connection reuse strategy based on something you reported in #2114

<artifactId>httpclient5</artifactId>
<!-- Consistent with version from karate-core -->
<version>5.3</version>
</dependency>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@f-delahaye just curious, why was this needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spring-boot comes with httpclient5 5.1.3 and overrides the version defined in karate-core (5.3)

$ mvn help:effective-pom -Dverbose | grep -A2 -B2 \<artifactId\>httpclient5\</artifactId\>
      <dependency>
        <groupId>org.apache.httpcomponents.client5</groupId>  <!-- org.springframework.boot:spring-boot-dependencies:2.6.6, line 953 -->
        <artifactId>httpclient5</artifactId>  <!-- org.springframework.boot:spring-boot-dependencies:2.6.6, line 954 -->
        <version>5.1.3</version>  <!-- org.springframework.boot:spring-boot-dependencies:2.6.6, line 955 -->
      </dependency>

Unfortunately, it looks like karate-demo does not start with my changes against httpclient 5.1.3 - it hangs.
Not sure why, I didn't investigate further -

I could not get it to work with the latest spring-boot either. But forcing to version 5.3 in karate-demo solves the issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok thanks

private final ScenarioEngine engine;
private final Logger logger;
private final HttpLogger httpLogger;

private HttpClientBuilder clientBuilder;
private CookieStore cookieStore;

public static class LenientCookieSpec extends DefaultCookieSpec {
// Not sure what the rationale was behind this class.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@f-delahaye some history is explained here (with links to other issues) #2165

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

}
}
try {
// client can not be closed/autoclosed as it is referenced accross multiple calls by ScenarioEngine
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@f-delahaye really appreciate all your comments and research above. for this comment on "client can not be autoclosed" - could you explain a bit more ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically,

try (CloseableHttpClient client = clientBuilder.build()) 

causes Connection pool shut down errors.

For some reason, I thought it was not auto closed in previous version either, so I didn't really look into this, but reading the code again, I can see that it was auto-closed, my bad.
I will try to take a deeper look.
Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so calling CloseableHttpClient.close() also closes the client's connection pool.

image

So the next call to ApacheHttpClient.invoke will create a new CloseableHttpClient but will re-use the same connection pool created in configure, which is closed, hence the error.

Previous version of ApacheHttpClient would not create the connection pool in configure and re-use it, instead a new one is created by every call to invoke.
It probably makes sense to have the same implementation ( a new connection pool for each call) in the new version as well.

If you prefer to reuse the same ConnectionPool, and therefore the same CloseableHttpClient, one solution would be to add a close() method in ApacheHttpClient that would call CloseableHttpClient.close. But, then, the question is how/when ApacheHttpClient.close should be called...

@ptrthomas
Any thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first thought is have the connection pool as a thread-local - maybe this is the most efficient solution, so it never needs to be closed (will die with the JVM)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hummm ... interesting idea. But not sure how to implement it: whenever the CloseableHttpClient is closed, its connectionManager is automatically closed too.

This post explains the issue better than I did:
https://stackoverflow.com/questions/76761703/correct-way-to-use-poolinghttpclientconnectionmanager-in-httpclient4-getting-iss

IMHO, setConnectionManagerShared(true) mentioned there is the way to go, along with a new close method on HttpClient that would get called from ScenarioEngine.stop and would close the connection manager.
If no objection, I will commit these changes today or tomorrow for review.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes that sounds good

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ptrthomas , changes have been pushed.
Note the new HttpClient.close() method, with no op implementation in all sub-classes except ApacheHttpClient.

As far as I am concerned, this PR is ready to be merged, if you're happy with it.

I'd still like to investigate if the connection reuse strategy may be improved. But any changes for that will come in a different PR.

…ctionManager in Apache client which is now shared
@ptrthomas
Copy link
Member

@f-delahaye sorry to hold on this, I'm thinking of releasing 1.5.0 final without this to be safe and then merge this

@f-delahaye
Copy link
Contributor Author

f-delahaye commented Jan 18, 2024

Thanks for the update. It actually makes a lot of sense to wait until the next release to merge it.

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

Successfully merging this pull request may close these issues.

2 participants