Update RestTemplate Javadoc with regards to setting interceptors on startup vs at runtime

[echolakov]

ConcurrentModificationException Diagram:

Summary:
Hello.

-ConcurrentModificationException occurs when one thread is modifying a collection with some interceptors, and another thread is iterating the same data structure with the elements in the context of the topic "ConcurrentModificationException Spring Framework Rest Template Setting Interceptors".

-The following code is not thread-safe and developers can potentially use the following code in a not thread-safe way.

-The currently shown source code allows thread-safe misusage, logging spikes, degraded performance and security risks.

-Nothing protects developers from abusing the Spring Framework to be used in an anti-design pattern on production servers.

-We can pass a synchronized collection hypothetically. The method responsible for setting the interceptors returns response always as an unsynchronized collection. The setInterceptors function processes the elements of the data structure we pass in a not thread-safe manner using copy action of elements' references, sorting the interceptors redundantly.

Note:
https://docs.oracle.com/javase/7/docs/api/java/util/ConcurrentModificationException.html
"Therefore, it would be wrong to write a program that depended on this exception for its correctness: ConcurrentModificationException should be used only to detect bugs."

Code:
https://github.com/spring-projects/spring-framework/blob/main/spring-web/src/main/java/org/springframework/http/client/support/InterceptingHttpAccessor.java

public abstract class InterceptingHttpAccessor extends HttpAccessor {

    private final List<ClientHttpRequestInterceptor> interceptors = new ArrayList<>();

...

    public void setInterceptors(List<ClientHttpRequestInterceptor> interceptors) {
        Assert.noNullElements(interceptors, "'interceptors' must not contain null elements");
        // Take getInterceptors() List as-is when passed in here
        if (this.interceptors != interceptors) {
            this.interceptors.clear();
            this.interceptors.addAll(interceptors);
            AnnotationAwareOrderComparator.sort(this.interceptors);
        }
    }
...
}

https://github.com/spring-projects/spring-framework/blob/main/spring-web/src/main/java/org/springframework/http/client/InterceptingClientHttpRequest.java

class InterceptingClientHttpRequest extends AbstractBufferingClientHttpRequest {

	private final ClientHttpRequestFactory requestFactory;

	private final List<ClientHttpRequestInterceptor> interceptors;
...
private class InterceptingRequestExecution implements ClientHttpRequestExecution {

		private final Iterator<ClientHttpRequestInterceptor> iterator;

		public InterceptingRequestExecution() {
			this.iterator = interceptors.iterator();
		}

		@Override
		public ClientHttpResponse execute(HttpRequest request, byte[] body) throws IOException {
			if (this.iterator.hasNext()) {
				ClientHttpRequestInterceptor nextInterceptor = this.iterator.next();
				return nextInterceptor.intercept(request, body, this);
			}
			else {...}
...
}}}

Test Scenario:
DemoApplication.txt

package com.example.demo;

import org.springframework.beans.factory.annotation.Qualifier;
import org.springframework.boot.SpringApplication;
import org.springframework.boot.autoconfigure.SpringBootApplication;
import org.springframework.boot.context.event.ApplicationReadyEvent;
import org.springframework.context.annotation.Bean;
import org.springframework.context.event.EventListener;
import org.springframework.http.HttpRequest;
import org.springframework.http.ResponseEntity;
import org.springframework.http.client.ClientHttpRequestExecution;
import org.springframework.http.client.ClientHttpRequestInterceptor;
import org.springframework.http.client.ClientHttpResponse;
import org.springframework.util.CollectionUtils;
import org.springframework.web.client.RestTemplate;

import java.io.IOException;
import java.util.ArrayList;
import java.util.List;

@SpringBootApplication
public class DemoApplication {

    class TestMiddleware implements ClientHttpRequestInterceptor {

        @Override
        public ClientHttpResponse intercept(HttpRequest request, byte[] body, ClientHttpRequestExecution execution) throws IOException {

            System.out.println("Executing " + request.getURI());

            ClientHttpResponse response = execution.execute(request, body);
            return response;
        }
    }

    @Bean
    public RestTemplate restTemplate() {
        RestTemplate restTemplate = new RestTemplate();
        return restTemplate;
    }

    @Bean
    public RestTemplate restTemplateFix() {
        List<ClientHttpRequestInterceptor> interceptors = new ArrayList<>();
        restTemplate.setInterceptors(interceptors);

        RestTemplate restTemplate = new RestTemplate();
        return restTemplate;
    }

    @Qualifier("restTemplate")
    RestTemplate restTemplate = new RestTemplate();

    @Qualifier("restTemplateFix")
    RestTemplate restTemplateFix = new RestTemplate();

    public static void main(String[] args) {
        SpringApplication.run(DemoApplication.class, args);
    }

    @EventListener(ApplicationReadyEvent.class)
    public void doSomethingAfterStartup() {
        //search for ConcurrentModificationException

        //what is ConcurrentModificationException
        //testConcurrentModificationExceptionInReadingIteration();

        //trigger ConcurrentModificationException
        testConcurrentModificationExceptionInRestTemplate();

        //fix one
        //testConcurrentModificationExceptionInRestTemplateFixWithSharedCollection();

        //fix two
        //testConcurrentModificationExceptionInRestTemplateFixWithInterceptorsCopy();

        //fix three
        //testConcurrentModificationExceptionInRestTemplateFixWithCheckInterceptorsCollection();

        //fix four
        //testConcurrentModificationExceptionInRestTemplateFixWithSetInterceptorsOnce();
    }

    private void testConcurrentModificationExceptionInRestTemplate() {
        for (int i = 0; i < 2000; i++) {
            new Thread(() -> {
                List<ClientHttpRequestInterceptor> interceptors = new ArrayList<>();
                interceptors.add(new TestMiddleware());
                restTemplate.setInterceptors(interceptors);
                ResponseEntity<String> response = restTemplate.getForEntity("http://ip.jsontest.com/", String.class);
                System.out.println(response);
            }).start();
        }
    }

    final List<ClientHttpRequestInterceptor> sharedCollection = new ArrayList<>(List.of(new ClientHttpRequestInterceptor[]{new TestMiddleware()}));

    private void testConcurrentModificationExceptionInRestTemplateFixWithSharedCollection() {
        for (int i = 0; i < 2000; i++) {
            new Thread(() -> {

                List<ClientHttpRequestInterceptor> interceptorsReference = restTemplate.getInterceptors();
                interceptorsReference = sharedCollection;

                ResponseEntity<String> response = restTemplate.getForEntity("http://ip.jsontest.com/", String.class);
                System.out.println(response);
            }).start();
        }
    }

    private void testConcurrentModificationExceptionInRestTemplateFixWithInterceptorsCopy() {
        for (int i = 0; i < 2000; i++) {
            new Thread(() -> {

                List<ClientHttpRequestInterceptor> interceptorsCopy = new ArrayList<>(List.of(new ClientHttpRequestInterceptor[]{new TestMiddleware()}));
                List<ClientHttpRequestInterceptor> interceptorsReference = restTemplate.getInterceptors();
                interceptorsReference = interceptorsCopy;

                ResponseEntity<String> response = restTemplate.getForEntity("http://ip.jsontest.com/", String.class);
                System.out.println(response);
            }).start();
        }
    }

    private void testConcurrentModificationExceptionInRestTemplateFixWithCheckInterceptorsCollection() {
        for (int i = 0; i < 2000; i++) {
            new Thread(() -> {
                List<ClientHttpRequestInterceptor> interceptors = restTemplate.getInterceptors();
                if (CollectionUtils.isEmpty(interceptors)) {
                    interceptors = new ArrayList<>();
                    interceptors.add(new TestMiddleware());
                }
                restTemplate.setInterceptors(interceptors);
                ResponseEntity<String> response = restTemplate.getForEntity("http://ip.jsontest.com/", String.class);
                System.out.println(response);
            }).start();
        }
    }

    private void testConcurrentModificationExceptionInRestTemplateFixWithSetInterceptorsOnce() {
        for (int i = 0; i < 2000; i++) {
            new Thread(() -> {
                ResponseEntity<String> response = restTemplateFix.getForEntity("http://ip.jsontest.com/", String.class);
                System.out.println(response);
            }).start();
        }
    }

    private void testConcurrentModificationExceptionInReadingIteration() {
        List<Integer> integers = new ArrayList();
        System.out.println("before");
        for (int i = 0; i < 20; i++) {
            int finalI = i;
            new Thread(() -> {
                System.out.println("thread" + finalI + " add item");
                integers.add(finalI);
            }).start();
        }
        new Thread(() -> {
            for (Integer integer : integers) {
                System.out.println("reading " + integers.get(integer));
            }
        }).start();
        System.out.println("after");
    }

}

Debugging steps:
-Run Spring Boot Demo Server

@SpringBootApplication
public class DemoApplication {
...
    public static void main(String[] args) {
        SpringApplication.run(DemoApplication.class, args);
    }

    @EventListener(ApplicationReadyEvent.class)
    public void doSomethingAfterStartup() {
        testConcurrentModificationExceptionInRestTemplate();
    }
...
}

-Create restTemplate bean

    @Bean
    public RestTemplate restTemplate() {
        RestTemplate restTemplate = new RestTemplate();
        return restTemplate;
    }

-Execute testConcurrentModificationExceptionInRestTemplate() method

    private void testConcurrentModificationExceptionInRestTemplate() {
        for (int i = 0; i < 2000; i++) {
            new Thread(() -> {
                List<ClientHttpRequestInterceptor> interceptors = new ArrayList<>();
                interceptors.add(new TestMiddleware());
                restTemplate.setInterceptors(interceptors);
                ResponseEntity<String> response = restTemplate.getForEntity("http://ip.jsontest.com/", String.class);
                System.out.println(response);
            }).start();
        }
    }

Debugging results:

Exception in thread "Thread-730" java.util.ConcurrentModificationException
	at java.base/java.util.ArrayList$Itr.checkForComodification(ArrayList.java:1043)
	at java.base/java.util.ArrayList$Itr.next(ArrayList.java:997)
	at org.springframework.http.client.InterceptingClientHttpRequest$InterceptingRequestExecution.execute(InterceptingClientHttpRequest.java:92)
	at com.example.demo.DemoApplication$TestMiddleware.intercept(DemoApplication.java:31)
	at org.springframework.http.client.InterceptingClientHttpRequest$InterceptingRequestExecution.execute(InterceptingClientHttpRequest.java:93)
	at org.springframework.http.client.InterceptingClientHttpRequest.executeInternal(InterceptingClientHttpRequest.java:77)
	at org.springframework.http.client.AbstractBufferingClientHttpRequest.executeInternal(AbstractBufferingClientHttpRequest.java:48)
	at org.springframework.http.client.AbstractClientHttpRequest.execute(AbstractClientHttpRequest.java:66)
	at org.springframework.web.client.RestTemplate.doExecute(RestTemplate.java:776)
	at org.springframework.web.client.RestTemplate.execute(RestTemplate.java:711)
	at org.springframework.web.client.RestTemplate.getForEntity(RestTemplate.java:361)
	at com.example.demo.DemoApplication.lambda$testConcurrentModificationExceptionInRestTemplate$0(DemoApplication.java:105)
	at java.base/java.lang.Thread.run(Thread.java:834)

...

Exception in thread "Thread-1855" java.util.ConcurrentModificationException
	at java.base/java.util.ArrayList.sort(ArrayList.java:1752)
	at org.springframework.core.annotation.AnnotationAwareOrderComparator.sort(AnnotationAwareOrderComparator.java:111)
	at org.springframework.http.client.support.InterceptingHttpAccessor.setInterceptors(InterceptingHttpAccessor.java:66)
	at com.example.demo.DemoApplication.lambda$testConcurrentModificationExceptionInRestTemplate$0(DemoApplication.java:104)
	at java.base/java.lang.Thread.run(Thread.java:834)

[rstoyanchev]

The RestTemplate like many other Spring infrastructure components, was designed for startup initialization when the Spring ApplicationContext creates and initializes framework and application components and injects them with dependencies. Once this is done, which is typically before requests are processed, it is not designed to be re-configured at runtime again. That's a general assumption and pattern of usage. The Javadoc would call it out otherwise when a particular configuration setting is explicitly designed for change at runtime. This is also something that's easy to work out by taking a look at the field and setter implementation.

The more recently designed WebCilent comes with a Builder interface and a mutate() method that let's you drop back into the Builder and build a new WebClient that's pre-configured to match the original, and that gives you two independent client instances. The only safe way where concurrency is expected. You can use Boot's RestTemplateBuilder to achieve a similar effect.

Aside from that we are not looking to make further changes to the RestTemplate and this would be a significant change. At best we could add something to the Javadoc to indicate more explicitly that configuration shouldn't be changed at runtime.

[echolakov]

Thank you for your answer and spent time on this issue, Rossen Stoyanchev. I had a task to fix a ConcurrentModificationException, so other microservices/projects may have/face the same wrong behavior/way. I understand that the Spring team built the RestTemplate a long time ago. Not-so-skillful developers must use a particular mind pattern when using the legacy RestTemplate. I acknowledge now that the Spring team cannot make such a significant fix change because of the legacy code and the corporations' usage. But it is easy and natural to go with the anti-design pattern if you come from a scripting experience like the Laravel framework. Yes, it will be great if there are a new tutorial and new java docs comments in the space of the Spring community. Emphasizing the marked behavior of the RestTemplate will be enough.

[rstoyanchev]

I want to emphasize again, this isn't some isolated pattern, but a general design approach. In a Spring application, components are typically initialized and configured at startup. For people who come from a very different language background and concurrency model, this does present a learning challenge, but in my opinion that challenge has to do with much broader concepts. The RestTemplate is just the specific instance where they hit the problem, but it's bound to happen elsewhere too if these general concepts are not well understood.

Note also that using a synchronized collection or otherwise allowing interceptors to be added at runtime would be dangerous, as the RestTemplate is a shared component, and it would lead to side effects in other places. So I'm not sure what the expectations were but at best this is a programming error. The only outcome really should be to have differently configured instances, which can be done even without explicit support (like the WebClient mutable builder), either on startup or at runtime, and for that Boot's RestTemplateBuilder should be quite helpful.

In any case, we'll add something to the Javadoc if it helps.

[echolakov]

Thank you, Rossen Stoyanchev.