Fixed connection pooling in the UrlConnectionHttpClient. #2660
Conversation
| response.responseBody().ifPresent(IoUtils::drainInputStream); | ||
| } | ||
|
|
||
| assertThat(CONNECTION_COUNTER.openedConnections()).isLessThan(initialOpenedConnections + 5); |
There was a problem hiding this comment.
Can we be more precise than lessThan here? It seems like this should be deterministic. Do we expect exactly initialOpenedConnections, initialOpenedConnections + 1?
There was a problem hiding this comment.
I guess the number may depend (and thus vary) upon the client implementation, and how many times we're willing to reuse a connection. Is it fair to say that no client ought to use more than 1 new connection here though? Can we do: isLessThanOrEqualTo(initialOpenedConnections + 1)?
There was a problem hiding this comment.
In my tests it was fairly consistently +1 when pooling was working, or +5 when it was not working.
Because the test is so indirect (involves cooperation of various HTTP client implementations as well as Wiremock/Jetty, I wasn't confident that hard-coding it to +1 was going to create a 100% deterministic test, and non-deterministic tests are worse than no test (IMO). I think if it's < 5 we can confidently say that connection pooling is working, but you're right that we don't know to what extent it's working. With the test how it currently is, we could make a change that makes pooling "worse" (whatever that means) without this test catching it, unless it completely breaks pooling altogether.
Let me try running it a few thousand times to see if it ever fails at +1.
There was a problem hiding this comment.
I switched it to +1, it seemed to work just fine. We can reassess if it becomes a problem.
| // Note: This socket factory MUST be reused between requests because the connection pool in the JVM is keyed by both | ||
| // URL and SSLSocketFactory. If the socket factory is not reused, connections will not be reused between requests. | ||
| SSLSocketFactory socketFactory = getSslContext(options).getSocketFactory(); | ||
|
|
||
| this.connectionFactory = url -> createDefaultConnection(url, socketFactory); |
There was a problem hiding this comment.
Awesome find and nice documentation.
millems
force-pushed
the
millem/fix-url-connection-pooling
branch
from
fe88941 to
76f808b
Compare
|
Kudos, SonarCloud Quality Gate passed! |
…a9ddd34b8 Pull request: release <- staging/c325cf33-678b-449e-b810-b49a9ddd34b8
No description provided.