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

useSystemProperties breaks custom connection pooling #13

Open
anowell opened this issue Oct 13, 2017 · 1 comment
Open

useSystemProperties breaks custom connection pooling #13

anowell opened this issue Oct 13, 2017 · 1 comment

Comments

@anowell
Copy link
Contributor

anowell commented Oct 13, 2017

In this builder chain, the call to useSystemProperties() effectively results in ignoring the values set by setMaxConnTotal(maxConnections) and setMaxConnPerRoute(maxConnections). And this isn't order-dependent, because the actual properties used by the underlying client are determined during the call to build with this code in the upstream Apache client

So I see basically 3 choices:

  1. Stop calling useSystemProperties(). This will require figuring out how to go back and add proxy support that motivated useSystemProperties.

  2. Explicitly call System.setProperty("http.keepAlive", "true") and System.setProperty("http.maxConnections", maxConnections) before building the client. (Somebody with a bit more java mastery wanna weigh in on the side affects of setting properties like this? Any risks to creating multiple AlgorithmiClient instances with different connection pooling?). This also means that we have to accept that maxConnTotal is always 2x the maxConnPerRoute.

  3. PR to try and convince upstream to replace that block with one where it only uses the system properties for those values if they weren't explicitly set. (I presume we could use the 2nd option in the meantime of waiting for such a PR to land.)

Wanna weigh in @kennydaniel, since I believe you green-lighted the useSystemProperties change?

@kennydaniel
Copy link
Contributor

kennydaniel commented Oct 16, 2017

Hmm, that is tricky. Calling System.setProperty could have sideeffects if someone includes our client in their code base, but have OTHER http clients that have useSystemProperties() enabled. In my opinion this makes option 2 not ideal. From reading the apache source code, I don't see an easy way to work around this.

I think option 1 seems like the least bad option. I grepped the apache source code for System.getProperty and it appear there are only 4 proxy properties used here:
https://github.com/apache/httpcomponents-client/blob/22902593e78d56423709b66abf4757082ddced50/httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/SystemDefaultCredentialsProvider.java#L141

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

No branches or pull requests

2 participants