Description
Brandon Zeeb opened SPR-12166 and commented
The class HttpComponentsClientHttpRequestFactory in the createRequest method attempts to create a custom RequestConfig if one is not provided by the createHttpContext (by way of HttpContext). This custom RequestConfig is created with a connect timeout, a socket timeout, but not a timeout value for the connection pool. In a production application, this opens up the possibility for the clients of RestTemplate to hang indefinitely waiting for a connection from the connection pool if all connection leases from the pool are currently occupied. If a value is not provided for the connection request timeout value of RequestConfig, HttpClient will wait infinitely long for a connection from the connection pool.
The code in question is:
if (this.socketTimeout > 0 || this.connectTimeout > 0) {
config = RequestConfig.custom()
.setConnectTimeout(this.connectTimeout)
.setSocketTimeout(this.socketTimeout)
.build();
}
When it should perhaps be:
if (this.socketTimeout > 0 || this.connectTimeout > 0) {
config = RequestConfig.custom()
.setConnectTimeout(this.connectTimeout)
.setSocketTimeout(this.socketTimeout)
.setConnectionRequestTimeout(this.connectionRequestTimeout)
.build();
}
Aside: I'm not sure if the if statement is necessary, HttpClient might already sanitize the values, I leave this to you to decide. I have initially made this a Major bug given the behavior that can manifest in an application (particularly under heavy load), also leave this to you do properly prioritize.
I also understand that RequestConfig contains many attributes, and perhaps not all of them should be mirrored in the HttpComponentsClientHttpRequestFactory. To this end, it's possible that perhaps a client should be forced to always supply one for this class, and thereby this is not a thing Spring Framework needs to solve.
Let me know your thoughts, and thanks in advance.
Affects: 4.1 GA
Issue Links:
- Configured RequestConfig in HttpClient gets overwritten by default [SPR-12540] #17144 Configured RequestConfig in HttpClient gets overwritten by default