Deprecate+Remove ability to provide a custom ClientHttpRequestFactory instance in RestTemplateBuilder

[tvahrst]

Problem

When ClientHttpRequestFactory isn't explicitly set, RestTemplateBuilder invokes detectRequestFactory() internally for every new RestTemplate. So every RestTemplate gets it's own 'prototype' ClientHttpRequestFactory.

On the other hand, when ClientHttpRequestFactory is explicitly set in RestTemplateBuilder, all subsequent build RestTemplates get the same 'singleton' ClientHttpRequestFactory.

During ResttemplateBuilder.build() method, the current ClientHttpRequestFactory becomes customized with timout values (connection-timeout and read-timeout). This leads to problems when multiple RestTemplates with different timeout values are created.

We have a Spring-Boot application that provides a RestTemplateBuilder bean which is already provided with a HttpComponentsHttpRequestFactory. Two service beans uses the RestTemplateBuilder to create a custom RestTemplate with different connection-timouts:

public class TestResttemplateBuilderTimeouts {
	private static Logger logger = LoggerFactory.getLogger(TestResttemplateBuilderTimeouts.class);

	@Test
	public void test1(){
		// globally prepared ResttemplateBuilder:
		HttpComponentsClientHttpRequestFactory f = new HttpComponentsClientHttpRequestFactory();
		RestTemplateBuilder builder = new RestTemplateBuilder().requestFactory(f);

		// in service bean 1:
		RestTemplate rt1 = builder.setConnectTimeout(10000).build();

		// in service bean 2
		RestTemplate rt2 = builder.setConnectTimeout(1000).build();


		// service bean 1 uses RestTemplate expecting connectionTime 10 Secs.  
		// but Connection-Timeout is only 1 Sec.
		try {
			logger.debug("start ...");
			String s = rt1.getForObject("http://www.google.com:81", String.class);
		}finally {
			logger.debug("stop ...");
		}
	}
}

Workaround

Our current Workaround: we do not set HttpComponentsHttpRequestFactory explicitly but let RestTemplateBuilder detect this factory. For customizing (i.e. setting max-connections und max-connections-per-route) we use the systemproperties approach of the httpcomponents httpclient.

Possible Solution (breaking the api in spring-web)

As far as I can see, all supported http implementations (httpcommons, netty, okhttp, okhttp3, SimpleClientHttpRequestFactory) allow a per request setting of connection- and read-timeouts. So a solution would be not to inject these timeouts into the ClientHttpRequestFactory but hold it as 'RequestSettings' in the RestTemplateBuilder and give them to every new created RestTemplate.

The RestTemplate in turn has to provide these settings as additional method-parameter to ClientHttpRequestFactory.createRequest(). This will however break the api of ClientHttpRequestFactory...

public interface ClientHttpRequestFactory {

	/**
	 * Create a new {@link ClientHttpRequest} for the specified URI and HTTP method.
	 * <p>The returned request can be written to, and then executed by calling
	 * {@link ClientHttpRequest#execute()}.
	 * @param uri the URI to create a request for
	 * @param httpMethod the HTTP method to execute
         * @param requestSettings Request-Settings, containing for instance connection- and read-timeouts
	 * @return the created request
	 * @throws IOException in case of I/O errors
	 */
	ClientHttpRequest createRequest(URI uri, HttpMethod httpMethod, RequestSettings requestSettings) throws IOException;

[wilkinsona]

Perhaps we could make the requestFactory(Class<? extends ClientHttpRequestFactory> requestFactory) method behave as factory detection does?

[bclozel]

I think what you're describing here is the expected behavior, since:

• RestTemplateBuilder instances are immutable (calling a method on it returns a new instance)
• RestTemplate and ClientHttpRequestFactory don't support a per-request configuration model
• The ClientHttpRequestFactory is here to instantiate requests with the given configuration and holds the shared resources (such as connection pools)

In most cases, sharing a ClientHttpRequestFactory between RestTemplate instances is a good idea to optimize resource usage. If you wish to have different configuration timeouts for RestTemplate, then separate ClientHttpRequestFactory instances is the way to go - resource usage will be the price to pay. Again, RestTemplate doesn't support a per-request configuration model.

One could also argue the following: mutating the request factory options at runtime is supposed to change the request behaviour as well.

I don't think Spring Boot can change anything in this model. If we were to replicate the factory detection for that (by cloning/creating request factory instances for each builder call), we'd be breaking the expected behaviour for many: resources wouldn't be shared anymore between RestTemplate that share the same request factory instance.

[tvahrst]

I agree that the ClientHttpRequestFactory cannot be cloned for every new RestTemplateBuilderInstance (since ClientHttpRequestFactory could be a Spring bean and cloning a singleton bean isn't a good idea).

On the other hand, the two set...Timeout() methods (in contrast to all other RestTemplateBuilder properties) leads to a direct manipulation of the singleton ClientHttpRequestFactory, and this is not easy to solve...

One could also argue the following: mutating the request factory options at runtime is supposed to change the request behaviour as well.

It may be not very obvious to the user of RestTemplateBuilder to determine, which setter method mutates the request factory. We in fact were surprised by this behavior...

I would at least suppose to add a warning to the javadoc of setConnectionTimeout() and setReadTimeout(), like

 * WARNING:
 * Invocations of this method may change the timeout behavior of 
 * all allready build RestTemplates!!
 *

However, our concrete problem isn't solved this way ;-). We (lvm-it) like the RestTemplateBuilder pattern and use it to define a default RestTemplate configuration for all our teams (implemented as autoconfiguration, for instance setting some ClientHttpRequestInterceptors and a SSLContext for client-side certificates). But the problematic side-effects of timeouts prevents us from using RestTemplateBuilder at the moment ...

[bclozel]

I changed my mind a bit and I think @wilkinsona is right.

Look at the following example; the RestTemplateBuilder behaviour is not consistent depending on the use case:

// auto-detect the client factory
RestTemplateBuilder builder = this.builder;
RestTemplate t1 = builder.setConnectTimeout(2_000).build();
RestTemplate t2 = builder.setConnectTimeout(1_000).build();
// separate connection factories, so each connection timeout is configured as expected
assertThat(t1.getRequestFactory()).isNotEqualTo(t2.getRequestFactory());

// Choose the client factory class
RestTemplateBuilder selectFactoryBuilder = this.builder.requestFactory(OkHttp3ClientHttpRequestFactory.class);
RestTemplate t3 = selectFactoryBuilder.setConnectTimeout(1_000).build();
RestTemplate t4 = selectFactoryBuilder.setConnectTimeout(2_000).build();
// Factory instance is shared, this test fails currently
assertThat(t3.getRequestFactory()).isNotEqualTo(t4.getRequestFactory());

// directly provide a client factory instance
ClientHttpRequestFactory factory = new SimpleClientHttpRequestFactory();
RestTemplateBuilder customFactoryBuilder = this.builder.requestFactory(factory);
RestTemplate t5 = customFactoryBuilder.setConnectTimeout(1_000).build();
RestTemplate t6 = customFactoryBuilder.setConnectTimeout(2_000).build();
// Factory instance is shared, connection timeout will be 2000 for both!
assertThat(t5.getRequestFactory()).isEqualTo(t6.getRequestFactory());

I agree that providing your own factory instance and changing the connection timeout down the line is a bit strange. But I think that, in general, the setConnectTimeout and setReadTimeout are the problematic ones. They break the "stateless" API contract.

[bclozel]

After discussing that with the team (and given my previous comment), we've found that the inconsistency comes from the requestFactory(ClientHttpRequestFactory) method. We should indeed be consistent here and create an instance of that for each build() call.

Here's what we should do:

1. Deprecate requestFactory(ClientHttpRequestFactory requestFactory) in 1.5.x
2. Remove requestFactory(ClientHttpRequestFactory requestFactory) in 2.0.0
3. Add requestFactory(Supplier<ClientHttpRequestFactory> supplier) in 2.0.0
4. Make requestFactory(Class<? extends ClientHttpRequestFactory> requestFactory) consistent with the rest and only create the instance on the build() call.

After some thoughts, you should indeed share and reuse RestTemplate instances in your application, but the main goal of RestTemplateBuilder is for Spring Boot to provide some sensible defaults for creating such clients and avoid creating those as beans. Client customizers declared as beans are automatically detected and applied. We shouldn't add to much levels of indirection there. If you need something more specific,

[philwebb]

We might have trouble deprecating requestFactory(ClientHttpRequestFactory requestFactory) in 1.5.x since we can't add the Supplier<ClientHttpRequestFactory> version there. I guess we could add a ClientHttpRequestFactoryFactory interface.... but... yuk :)

I'm tempted to leave 1.5 as it is, perhaps with some Javadoc warning that the instance is shared.