Fix wrong synchronization on Spring Refresh Context Event (#1168) (#1170)

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

@@ -216,7 +216,7 @@ public class JGitEnvironmentRepository extends AbstractScmEnvironmentRepository
 	}
 
 	@Override
-	public void afterPropertiesSet() throws Exception {
+	public synchronized void afterPropertiesSet() throws Exception {
 		Assert.state(getUri() != null,
 				"You need to configure a uri for the git repository");
 		initialize();

spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryConcurrencyTests.java

@@ -17,17 +17,28 @@
 package org.springframework.cloud.config.server.environment;
 
 import java.io.File;
+import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
+import java.util.concurrent.atomic.AtomicInteger;
 
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.eclipse.jgit.api.CheckoutCommand;
+import org.eclipse.jgit.api.CloneCommand;
+import org.eclipse.jgit.api.FetchCommand;
+import org.eclipse.jgit.api.Git;
+import org.eclipse.jgit.api.errors.*;
+import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.transport.FetchResult;
 import org.eclipse.jgit.util.FileUtils;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
-
 import org.springframework.boot.autoconfigure.context.PropertyPlaceholderAutoConfiguration;
 import org.springframework.boot.builder.SpringApplicationBuilder;
 import org.springframework.boot.context.properties.EnableConfigurationProperties;
@@ -94,6 +105,63 @@ public class JGitEnvironmentRepositoryConcurrencyTests {
 		assertEquals("master", environment.getLabel());
 	}
 
+	protected Log logger = LogFactory.getLog(getClass());
+
+	/**
+	 * Simulates following actions in parallel:
+	 *   - Client tries to obtain configuration with specified label
+	 *   - Spring Refresh Context Event occurs
+	 */
+	@Test
+	public void concurrentRefreshContextAndGetLabels() throws Exception {
+		// Prepare the repo
+		final JGitConfigServerTestData testData = JGitConfigServerTestData.prepareClonedGitRepository(TestConfiguration.class);
+		JGitEnvironmentRepository repository = testData.getRepository();
+		repository.setCloneOnStart(true);
+		repository.setGitFactory(new DelayedGitFactoryMock());
+		repository.setBasedir(testData.getClonedGit().getGitWorkingDirectory());
+		repository.setUri(testData.getServerGit().getGitWorkingDirectory().getAbsolutePath().replace("file://", ""));
+
+		final AtomicInteger errorCount = new AtomicInteger();
+
+		// Prepare two threads to do the parallel work
+		Thread client = new Thread(new Runnable() {
+			@Override
+			public void run() {
+				logger.info("client start.");
+				try {
+					Environment environment = testData.getRepository().findOne("bar", "staging", "master");
+				} catch (Exception e) {
+					errorCount.incrementAndGet();
+					e.printStackTrace();
+				}
+				logger.info("client end.");
+			}
+		});
+
+		Thread refresh = new Thread(new Runnable() {
+			@Override
+			public void run() {
+				try {
+					logger.info("refresh start.");
+					testData.getRepository().afterPropertiesSet();
+					logger.info("refresh end.");
+				} catch (Exception e) {
+					errorCount.incrementAndGet();
+					e.printStackTrace();
+				}
+			}
+		});
+
+		// Start the parallel actions and wait till the end.
+		refresh.start();
+		client.start();
+		refresh.join();
+		client.join();
+
+		assertEquals(0, errorCount.get());
+	}
+
 	@Configuration
 	@EnableConfigurationProperties(ConfigServerProperties.class)
 	@Import({ PropertyPlaceholderAutoConfiguration.class,
@@ -101,4 +169,80 @@ public class JGitEnvironmentRepositoryConcurrencyTests {
 	protected static class TestConfiguration {
 	}
 
+	private static class DelayedGitFactoryMock extends JGitEnvironmentRepository.JGitFactory {
+
+		@Override
+		public Git getGitByOpen(File file) throws IOException {
+			Git originalGit = DelayedGitMock.open(file);
+			return new DelayedGitMock(originalGit.getRepository());
+		}
+
+		@Override
+		public CloneCommand getCloneCommandByCloneRepository() {
+			return new DelayedCloneCommand();
+		}
+	}
+
+	private static class DelayedGitMock extends Git {
+
+		public DelayedGitMock(Repository repo) {
+			super(repo);
+		}
+
+		@Override
+		public FetchCommand fetch() {
+			return new DelayedFetchCommand(getRepository());
+		}
+
+		@Override
+		public CheckoutCommand checkout() {
+			return new DelayedCheckoutCommand(getRepository());
+		}
+	}
+
+	private static class DelayedCloneCommand extends CloneCommand {
+		@Override
+		public Git call() throws GitAPIException, InvalidRemoteException, TransportException {
+			try {
+				Thread.sleep(250);
+			} catch (InterruptedException e) {
+				e.printStackTrace();
+			}
+			return super.call();
+		}
+	}
+
+	private static class DelayedFetchCommand extends FetchCommand {
+
+		public DelayedFetchCommand(Repository repo) {
+			super(repo);
+		}
+
+		@Override
+		public FetchResult call() throws GitAPIException, InvalidRemoteException, TransportException {
+			try {
+				Thread.sleep(250);
+			} catch (InterruptedException e) {
+				e.printStackTrace();
+			}
+			return super.call();
+		}
+	}
+
+	private static class DelayedCheckoutCommand extends CheckoutCommand {
+		public DelayedCheckoutCommand(Repository repo) {
+			super(repo);
+		}
+
+		@Override
+		public Ref call() throws GitAPIException, RefAlreadyExistsException, RefNotFoundException, InvalidRefNameException, CheckoutConflictException {
+			try {
+				Thread.sleep(250);
+			} catch (InterruptedException e) {
+				e.printStackTrace();
+			}
+			return super.call();
+		}
+	}
+
 }