2296 upgrade to apache httpclient 5 #2471
Conversation
…d support of ntlm
| <artifactId>httpclient5</artifactId> | ||
| <!-- Consistent with version from karate-core --> | ||
| <version>5.3</version> | ||
| </dependency> |
There was a problem hiding this comment.
@f-delahaye just curious, why was this needed
There was a problem hiding this comment.
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.
| 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. |
There was a problem hiding this comment.
@f-delahaye some history is explained here (with links to other issues) #2165
| } | ||
| } | ||
| try { | ||
| // client can not be closed/autoclosed as it is referenced accross multiple calls by ScenarioEngine |
There was a problem hiding this comment.
@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 ?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Ok, so calling CloseableHttpClient.close() also closes the client's connection pool.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
@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 |
|
Thanks for the update. It actually makes a lot of sense to wait until the next release to merge it. |
Description
Upgrade to httpclient5.