-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 improve reuse connection strategy #2487
base: develop
Are you sure you want to change the base?
2296 improve reuse connection strategy #2487
Conversation
…d support of ntlm
…ctionManager in Apache client which is now shared
Sorry, @ptrthomas I was hoping to have this PR target the branch used in #2471 to show only the changes relevant for the new reuse connection strategy (one line) but I could not. Sorry for the confusion. You can merge directly this PR and disregard 2471 , or merge 2471 first and then this one, whatever is more convenient for you. |
@f-delahaye I don't mind closing 2471 and just focus on this one, good to target a new version of hc, does that sound ok, anyway I will park this for 2-3 weeks while I release 1.5.0 final |
Description
Improves the brutal no reuse connection strategy initially implemented in #2471.
The default reuse connection strategy was causing ProxyServerTest to fail and I think the reason is:
when a proxy is used,
ProxyRemoteHandler
is called, but it closes the channel when the response is set (See last line inProxyRemoteHandler.channelRead0
).But the apache http client is not notified and tries to re-use the connection, hence the error on subsequent calls.
The new proposed strategy therefore closes the connection when a proxy is used, and delegates to the default strategy in all other cases. It might be possibly to be even smarter and take nonProxyHosts and a few other parameters into account too, but this implementation should be good enough, I believe.
Logs from this PR, using hc5 version 5.4.alpha1 because version 5.3 has an annoying bug causing NPEs in DEBUG:
mvn test -Dtest=ProxyServerTest
Connection is closed after each call. This is on purpose whenever a proxy is used, as ProxyRemoteHandler closes the channel, hence causing SocketException if the connection is re-used.
mvn test -Dtest=FeatureServerTest
the first 6 calls, all within the same Scenario, use the same http-outgoing-0 connection.
The connection is released after the call, but not closed.
Calls #7 and #8, which belong to different Scenarios, will create one connection per Scenario.
mvn test -Dtest=FeatureServerTest
against current master:Connection is closed after each call. So is the connection manager, by the way.