Race condition between environment and resources repository when using jgit repository

[dpozinen]

Context

Most of the logic of ResourceController is contained in a synchronized retrieve method.

spring-cloud-config/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/resource/ResourceController.java

Lines 143 to 151 in d6aad0f

	name = Environment.normalize(name);
	label = Environment.normalize(label);
	Resource resource = this.resourceRepository.findOne(name, profile, label, path);
	if (checkNotModified(request, resource)) {
	// Content was not modified. Just return.
	return null;
	}
	// ensure InputStream will be closed to prevent file locks on Windows
	try (InputStream is = resource.getInputStream()) {

This method, in turn calls ResourceRepository#findOne and then a SearchPathLocator is used to find locations for the resources:

spring-cloud-config/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/resource/GenericResourceRepository.java

Line 67 in d6aad0f

String[] locations = this.service.getLocations(application, profile, label).getLocations();

In our context the SearchPathLocator is the jgit one, which modifies the content of the local git repo:

spring-cloud-config/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepository.java

Lines 257 to 263 in d6aad0f

	if (label == null) {
	label = this.defaultLabel;
	}
	String version;
	try {
	version = refresh(label);
	}

Problem

The ResourceController#retrieve only starts reading from disk once it has received the Resource from the repository.

At the same time another request may come in, but to fetch an Environment. Since these are different synchronized blocks (resources vs environment) the request proceeds to the same JGitEnvironmentRepository#getLocations.

So, in effect what can happen is that while fetching a resource, an environment request can modify the local repo (different label, or new commits have been fetched) before the resource file has been read.

I have got this issue on version 4.1.4

Steps to (consistently) reproduce:

1. Add a thread sleep before the resource is actually read in resource controller.
2. Create a remote repository with two branches (let's say work and master)
3. Add an application.properties file in work with from=work
4. Add an application.properties file in master with from=master
5. Make two Requests in parallel:

        var resourceFromWorkWithConcurrency = new AtomicReference<String>();
        var propsFromMaster = new AtomicReference<Properties>();

        String resourceFromWorkNoConcurrency = fetchResource("work", "application.properties");

        var workThread = new Thread(() -> resourceFromWorkWithConcurrency.set(fetchResource("work", "application.properties")));
        var masterThread = new Thread(() -> propsFromMaster.set(fetchEnv("master")));
        workThread.start();
        masterThread.start();
        masterThread.join();
        workThread.join();

        assertThat(resourceFromWorkNoConcurrency).isEqualTo("from=work\n"); // expected value
        assertThat(resourceFromWorkWithConcurrency.get()).isEqualTo("from=master\n"); // bad value
        assertThat(propsFromMaster.get().get("from")).isEqualTo("master");