Make TextFileBasedViolationStore thread-safe under parallel test execution

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<String, FileSyncedProperties> 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() {

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<Future<?>> 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<String> 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;
+    }
+}

archunit/src/test/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStoreTest.java

@@ -3,17 +3,17 @@
 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;
 
 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<String> keyValues = new LinkedList<>(asList(keyValuePairs));