diff --git a/archunit/src/main/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStore.java b/archunit/src/main/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStore.java index 9cdaf53b0..fd2bcc56e 100644 --- a/archunit/src/main/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStore.java +++ b/archunit/src/main/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStore.java @@ -23,6 +23,7 @@ import java.util.List; import java.util.Properties; import java.util.UUID; +import java.util.concurrent.ConcurrentHashMap; import java.util.regex.Pattern; import com.google.common.base.Splitter; @@ -32,7 +33,6 @@ import org.slf4j.LoggerFactory; import static com.google.common.base.Preconditions.checkArgument; -import static com.google.common.base.Preconditions.checkState; import static com.google.common.io.Files.toByteArray; import static com.tngtech.archunit.PublicAPI.Usage.ACCESS; import static com.tngtech.archunit.PublicAPI.Usage.INHERITANCE; @@ -74,6 +74,8 @@ public final class TextFileBasedViolationStore implements ViolationStore { private static final String ALLOW_STORE_UPDATE_PROPERTY_NAME = "default.allowStoreUpdate"; private static final String ALLOW_STORE_UPDATE_DEFAULT = "true"; + private static final ConcurrentHashMap STORED_RULES_BY_PATH = new ConcurrentHashMap<>(); + private final RuleViolationFileNameStrategy ruleViolationFileNameStrategy; private boolean storeCreationAllowed; @@ -105,13 +107,20 @@ public void initialize(Properties properties) { storeUpdateAllowed = Boolean.parseBoolean(properties.getProperty(ALLOW_STORE_UPDATE_PROPERTY_NAME, ALLOW_STORE_UPDATE_DEFAULT)); String path = properties.getProperty(STORE_PATH_PROPERTY_NAME, STORE_PATH_DEFAULT); storeFolder = new File(path); - ensureExistence(storeFolder); File storedRulesFile = getStoredRulesFile(); log.trace("Initializing {} at {}", TextFileBasedViolationStore.class.getSimpleName(), storedRulesFile.getAbsolutePath()); - storedRules = new FileSyncedProperties(storedRulesFile); + storedRules = getOrCreateStoredRules(storedRulesFile); checkInitialization(storedRules.initializationSuccessful(), "Cannot create rule store at %s", storedRulesFile.getAbsolutePath()); } + private FileSyncedProperties getOrCreateStoredRules(File storedRulesFile) { + try { + return STORED_RULES_BY_PATH.computeIfAbsent(storedRulesFile.getCanonicalPath(), path -> new FileSyncedProperties(storedRulesFile)); + } catch (IOException e) { + throw new StoreInitializationFailedException(e); + } + } + private File getStoredRulesFile() { File rulesFile = new File(storeFolder, STORED_RULES_FILE_NAME); if (!rulesFile.exists() && !storeCreationAllowed) { @@ -122,10 +131,6 @@ private File getStoredRulesFile() { return rulesFile; } - private void ensureExistence(File folder) { - checkState(folder.exists() && folder.isDirectory() || folder.mkdirs(), "Cannot create folder %s", folder.getAbsolutePath()); - } - private void checkInitialization(boolean initializationSuccessful, String message, Object... args) { if (!initializationSuccessful) { throw new StoreInitializationFailedException(String.format(message, args)); @@ -171,17 +176,14 @@ private String unescape(String violation) { private String ensureRuleFileName(ArchRule rule) { String ruleDescription = rule.getDescription(); - - String ruleFileName; - if (storedRules.containsKey(ruleDescription)) { - ruleFileName = storedRules.getProperty(ruleDescription); - log.trace("Rule '{}' is already stored in file {}", ruleDescription, ruleFileName); - } else { - ruleFileName = ruleViolationFileNameStrategy.createRuleFileName(ruleDescription); - log.trace("Assigning new file {} to rule '{}'", ruleFileName, ruleDescription); - storedRules.setProperty(ruleDescription, ruleFileName); + String candidateFileName = ruleViolationFileNameStrategy.createRuleFileName(ruleDescription); + String existingFileName = storedRules.putIfAbsent(ruleDescription, candidateFileName); + if (existingFileName == null) { + log.trace("Assigning new file {} to rule '{}'", candidateFileName, ruleDescription); + return candidateFileName; } - return ruleFileName; + log.trace("Rule '{}' is already stored in file {}", ruleDescription, existingFileName); + return existingFileName; } @Override @@ -223,13 +225,23 @@ boolean initializationSuccessful() { } private File initializePropertiesFile(File file) { - boolean fileAvailable; try { - fileAvailable = file.exists() || file.createNewFile(); + File directory = file.getParentFile(); + + // mkdirs() returns false both on failure and if another process concurrently + // created the directory, so isDirectory() distinguishes the two + if (!directory.mkdirs() && !directory.isDirectory()) { + return null; + } + + if (!file.exists() && !file.createNewFile()) { + return null; + } + + return file; } catch (IOException e) { - fileAvailable = false; + return null; } - return fileAvailable ? file : null; } private Properties loadRulesFrom(File file) { @@ -250,9 +262,15 @@ String getProperty(String propertyName) { return loadedProperties.getProperty(ensureUnixLineBreaks(propertyName)); } - void setProperty(String propertyName, String value) { - loadedProperties.setProperty(ensureUnixLineBreaks(propertyName), ensureUnixLineBreaks(value)); + synchronized String putIfAbsent(String key, String value) { + String normalizedKey = ensureUnixLineBreaks(key); + String existing = loadedProperties.getProperty(normalizedKey); + if (existing != null) { + return existing; + } + loadedProperties.setProperty(normalizedKey, ensureUnixLineBreaks(value)); syncFileSystem(); + return null; } private void syncFileSystem() { diff --git a/archunit/src/test/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStoreConcurrencyTest.java b/archunit/src/test/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStoreConcurrencyTest.java new file mode 100644 index 000000000..22f222a29 --- /dev/null +++ b/archunit/src/test/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStoreConcurrencyTest.java @@ -0,0 +1,129 @@ +package com.tngtech.archunit.library.freeze; + +import java.io.File; +import java.io.FileInputStream; +import java.io.IOException; +import java.nio.file.Path; +import java.util.LinkedList; +import java.util.Properties; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.stream.IntStream; + +import com.google.common.collect.ImmutableList; +import com.tngtech.archunit.lang.ArchRule; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.RepeatedTest; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.classes; +import static java.util.Arrays.asList; +import static org.assertj.core.api.Assertions.assertThat; + +public class TextFileBasedViolationStoreConcurrencyTest { + + @TempDir + Path temporaryFolder; + + private File configuredFolder; + + @BeforeEach + public void setUp() { + configuredFolder = new File(temporaryFolder.toFile(), "notyetthere"); + } + + @RepeatedTest(5) + public void is_safe_when_multiple_instances_initialize_concurrently() throws Exception { + runConcurrently(8, (i, startSignal) -> { + ViolationStore instanceStore = new TextFileBasedViolationStore(); + startSignal.await(); + instanceStore.initialize(defaultStoreProperties()); + }); + + // The real test here is that StoreInitializationFailedException is not thrown + assertThat(configuredFolder).exists(); + } + + @Test + public void is_safe_when_multiple_instances_save_to_the_same_store_concurrently() throws Exception { + int ruleCount = 20; + + runConcurrently(ruleCount, (i, startSignal) -> { + ViolationStore instanceStore = new TextFileBasedViolationStore(); + instanceStore.initialize(defaultStoreProperties()); + startSignal.await(); + instanceStore.save(rule("concurrent rule " + i), ImmutableList.of("violation " + i)); + }); + + Properties storedRules = readProperties(new File(configuredFolder, "stored.rules")); + assertThat(storedRules).hasSize(ruleCount); + + ViolationStore readerStore = new TextFileBasedViolationStore(); + readerStore.initialize(defaultStoreProperties()); + for (int i = 0; i < ruleCount; i++) { + ArchRule concurrentRule = rule("concurrent rule " + i); + assertThat(readerStore.contains(concurrentRule)).as("store contains " + concurrentRule.getDescription()).isTrue(); + assertThat(readerStore.getViolations(concurrentRule)).containsOnly("violation " + i); + } + } + + @SuppressWarnings("resource") // only available in Java 19 and later + private void runConcurrently(int taskCount, ConcurrentTask task) throws Exception { + ExecutorService executor = Executors.newFixedThreadPool(8); + CountDownLatch startSignal = new CountDownLatch(1); + List> futures = new ArrayList<>(); + + IntStream.range(0, taskCount).forEach(i -> futures.add(executor.submit(() -> { + task.run(i, startSignal); + return null; + }))); + + startSignal.countDown(); + executor.shutdown(); + + assertThat(executor.awaitTermination(30, TimeUnit.SECONDS)).isTrue(); + + // exceptions thrown inside submitted tasks are swallowed unless we call get() on each future + for (Future future : futures) { + future.get(); + } + } + + private Properties defaultStoreProperties() { + return propertiesOf( + "default.path", configuredFolder.getAbsolutePath(), + "default.allowStoreCreation", String.valueOf(true)); + } + + private Properties propertiesOf(String... keyValuePairs) { + Properties result = new Properties(); + LinkedList keyValues = new LinkedList<>(asList(keyValuePairs)); + while (!keyValues.isEmpty()) { + result.setProperty(keyValues.poll(), keyValues.poll()); + } + return result; + } + + private Properties readProperties(File file) throws IOException { + Properties properties = new Properties(); + try (FileInputStream inputStream = new FileInputStream(file)) { + properties.load(inputStream); + } + return properties; + } + + private ArchRule rule(String description) { + return classes().should().bePublic().as(description); + } + + @FunctionalInterface + private interface ConcurrentTask { + void run(int ruleIndex, CountDownLatch startSignal) throws Exception; + } +} diff --git a/archunit/src/test/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStoreTest.java b/archunit/src/test/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStoreTest.java index 9916799d5..202a786df 100644 --- a/archunit/src/test/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStoreTest.java +++ b/archunit/src/test/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStoreTest.java @@ -3,6 +3,7 @@ import java.io.File; import java.io.FileInputStream; import java.io.IOException; +import java.nio.file.Path; import java.util.LinkedList; import java.util.List; import java.util.Properties; @@ -10,10 +11,9 @@ import com.google.common.collect.ImmutableList; import com.google.common.io.Files; import com.tngtech.archunit.lang.ArchRule; -import org.junit.Before; -import org.junit.Rule; -import org.junit.Test; -import org.junit.rules.TemporaryFolder; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.classes; import static java.nio.charset.StandardCharsets.UTF_8; @@ -23,19 +23,17 @@ public class TextFileBasedViolationStoreTest { - @Rule - public final TemporaryFolder temporaryFolder = new TemporaryFolder(); + @TempDir + Path temporaryFolder; private final ViolationStore store = new TextFileBasedViolationStore(); private File configuredFolder; - @Before - public void setUp() throws Exception { - configuredFolder = new File(temporaryFolder.newFolder(), "notyetthere"); + @BeforeEach + public void setUp() { + configuredFolder = new File(temporaryFolder.toFile(), "notyetthere"); - store.initialize(propertiesOf( - "default.path", configuredFolder.getAbsolutePath(), - "default.allowStoreCreation", String.valueOf(true))); + store.initialize(defaultStoreProperties()); } @Test @@ -126,6 +124,12 @@ private Properties readProperties(File file) throws IOException { return properties; } + private Properties defaultStoreProperties() { + return propertiesOf( + "default.path", configuredFolder.getAbsolutePath(), + "default.allowStoreCreation", String.valueOf(true)); + } + private Properties propertiesOf(String... keyValuePairs) { Properties result = new Properties(); LinkedList keyValues = new LinkedList<>(asList(keyValuePairs));