From ec94a579f4fd45f1f5da0dcb043e155515a65eab Mon Sep 17 00:00:00 2001 From: Dave Syer Date: Thu, 16 Jul 2015 09:25:58 +0100 Subject: [PATCH] Synchronize access to code that deletes basedir on startup When the first request comes in there is a potential race while the basedir is prepared (if another request comes in at that crucial time they collide). This change just synchronizes the method where the basedir is prepared (it's a once only operation). Fixes gh-187 --- .../server/JGitEnvironmentRepository.java | 16 +-- ...EnvironmentRepositoryConcurrencyTests.java | 99 +++++++++++++++++++ 2 files changed, 108 insertions(+), 7 deletions(-) create mode 100644 spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/JGitEnvironmentRepositoryConcurrencyTests.java diff --git a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/JGitEnvironmentRepository.java b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/JGitEnvironmentRepository.java index b0a86f46..3d60fb8f 100644 --- a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/JGitEnvironmentRepository.java +++ b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/JGitEnvironmentRepository.java @@ -71,8 +71,7 @@ public class JGitEnvironmentRepository extends AbstractScmEnvironmentRepository private boolean cloneOnStart = false; - private JGitEnvironmentRepository.JGitFactory gitFactory = - new JGitEnvironmentRepository.JGitFactory(); + private JGitEnvironmentRepository.JGitFactory gitFactory = new JGitEnvironmentRepository.JGitFactory(); public JGitEnvironmentRepository(ConfigurableEnvironment environment) { super(environment); @@ -157,7 +156,8 @@ public class JGitEnvironmentRepository extends AbstractScmEnvironmentRepository private synchronized Environment loadEnvironment(Git git, String application, String profile, String label) throws GitAPIException { - NativeEnvironmentRepository environment = new NativeEnvironmentRepository(getEnvironment()); + NativeEnvironmentRepository environment = new NativeEnvironmentRepository( + getEnvironment()); git.getRepository().getConfig().setString("branch", label, "merge", label); Ref ref = checkout(git, label); if (shouldPull(git, ref)) { @@ -223,7 +223,10 @@ public class JGitEnvironmentRepository extends AbstractScmEnvironmentRepository } } - private Git copyRepository() throws IOException, GitAPIException { + // Synchronize here so that multiple requests don't all try and delete the base dir + // together (this is a once only operation, so it only holds things up on the first + // request). + private synchronized Git copyRepository() throws IOException, GitAPIException { deleteBaseDirIfExists(); getBasedir().mkdirs(); Assert.state(getBasedir().exists(), "Could not create basedir: " + getBasedir()); @@ -304,7 +307,7 @@ public class JGitEnvironmentRepository extends AbstractScmEnvironmentRepository getPassword())); } - private void setTimeout(TransportCommand pull) { + private void setTimeout(TransportCommand pull) { pull.setTimeout(this.timeout); } @@ -339,8 +342,7 @@ public class JGitEnvironmentRepository extends AbstractScmEnvironmentRepository /** * Wraps the static method calls to {@link org.eclipse.jgit.api.Git} and - * {@link org.eclipse.jgit.api.CloneCommand} allowing for easier unit - * testing. + * {@link org.eclipse.jgit.api.CloneCommand} allowing for easier unit testing. */ static class JGitFactory { diff --git a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/JGitEnvironmentRepositoryConcurrencyTests.java b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/JGitEnvironmentRepositoryConcurrencyTests.java new file mode 100644 index 00000000..3eaf5bd1 --- /dev/null +++ b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/JGitEnvironmentRepositoryConcurrencyTests.java @@ -0,0 +1,99 @@ +/* + * Copyright 2013-2015 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.cloud.config.server; + +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertEquals; + +import java.io.File; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; + +import org.eclipse.jgit.util.FileUtils; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.springframework.boot.autoconfigure.PropertyPlaceholderAutoConfiguration; +import org.springframework.boot.builder.SpringApplicationBuilder; +import org.springframework.cloud.config.environment.Environment; +import org.springframework.cloud.config.server.config.EnvironmentRepositoryConfiguration; +import org.springframework.context.ConfigurableApplicationContext; +import org.springframework.context.annotation.Configuration; +import org.springframework.context.annotation.Import; + +/** + * @author Dave Syer + * @author Roy Clarkson + */ +public class JGitEnvironmentRepositoryConcurrencyTests { + + private ConfigurableApplicationContext context; + + private File basedir = new File("target/config"); + + @Before + public void init() throws Exception { + if (this.basedir.exists()) { + FileUtils.delete(this.basedir, FileUtils.RECURSIVE); + } + ConfigServerTestUtils.deleteLocalRepo("config-copy"); + } + + @After + public void close() { + if (this.context != null) { + this.context.close(); + } + } + + @Test + public void vanilla() throws Exception { + String uri = ConfigServerTestUtils.prepareLocalRepo(); + this.context = new SpringApplicationBuilder(TestConfiguration.class).web(false) + .properties("spring.cloud.config.server.git.uri:" + uri).run(); + final EnvironmentRepository repository = this.context + .getBean(EnvironmentRepository.class); + ExecutorService threads = Executors.newFixedThreadPool(4); + List> tasks = new ArrayList>(); + for (int i=0; i<30; i++) { + tasks.add(threads.submit(new Runnable() { + @Override + public void run() { + repository.findOne("bar", "staging", "master"); + } + }, true)); + } + for (Future future : tasks) { + future.get(); + } + Environment environment = repository.findOne("bar", "staging", "master"); + assertEquals(2, environment.getPropertySources().size()); + assertEquals("bar", environment.getName()); + assertArrayEquals(new String[] { "staging" }, environment.getProfiles()); + assertEquals("master", environment.getLabel()); + } + + @Configuration + @Import({ PropertyPlaceholderAutoConfiguration.class, + EnvironmentRepositoryConfiguration.class }) + protected static class TestConfiguration { + } + +}