From be4155eb12a243a349ec417aeda1dd34ca5bd5b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Vyhl=C3=ADdka?= Date: Thu, 25 Oct 2018 15:56:51 +0200 Subject: [PATCH] Fix wrong synchronization on Spring Refresh Context Event (#1168) (#1170) --- .../JGitEnvironmentRepository.java | 2 +- ...EnvironmentRepositoryConcurrencyTests.java | 146 +++++++++++++++++- 2 files changed, 146 insertions(+), 2 deletions(-) diff --git a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepository.java b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepository.java index 41d325a3..9c4d7905 100644 --- a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepository.java +++ b/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(); diff --git a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryConcurrencyTests.java b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryConcurrencyTests.java index 84e359cd..6da158be 100644 --- a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryConcurrencyTests.java +++ b/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(); + } + } + }