algorithmia-java icon indicating copy to clipboard operation
algorithmia-java copied to clipboard

useSystemProperties breaks custom connection pooling

Open anowell opened this issue 8 years ago • 1 comments

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?

anowell avatar Oct 13 '17 21:10 anowell

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

kennydaniel avatar Oct 16 '17 17:10 kennydaniel