From 296d2eec8484578c66bae372ec766dc852968f5a Mon Sep 17 00:00:00 2001 From: Niklas Keller Date: Mon, 6 Jul 2026 14:34:30 +0200 Subject: [PATCH 1/3] Make TextFileBasedViolationStore thread-safe under parallel test execution Multiple FreezingArchRule instances each create a fresh TextFileBasedViolationStore and load stored.rules into their own private snapshot. Concurrent saves caused a lost-update: the last writer overwrote the file with only its own entries. Fix by sharing one FileSyncedProperties per stored.rules canonical path via a static ConcurrentHashMap, and synchronizing writes with a putIfAbsent method that atomically checks, sets, and flushes to disk under one lock. Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: Niklas Keller --- .../freeze/TextFileBasedViolationStore.java | 40 +++++++++++----- .../TextFileBasedViolationStoreTest.java | 47 +++++++++++++++++-- 2 files changed, 71 insertions(+), 16 deletions(-) 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 9cdaf53b0c..c0b1aa9fbc 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; @@ -74,6 +75,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; @@ -108,10 +111,18 @@ public void initialize(Properties properties) { 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) { @@ -171,17 +182,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 @@ -250,9 +258,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/TextFileBasedViolationStoreTest.java b/archunit/src/test/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStoreTest.java index 9916799d5d..d1b063b344 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 @@ -6,6 +6,11 @@ import java.util.LinkedList; import java.util.List; import java.util.Properties; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; +import java.util.stream.IntStream; import com.google.common.collect.ImmutableList; import com.google.common.io.Files; @@ -33,9 +38,7 @@ public class TextFileBasedViolationStoreTest { public void setUp() throws Exception { configuredFolder = new File(temporaryFolder.newFolder(), "notyetthere"); - store.initialize(propertiesOf( - "default.path", configuredFolder.getAbsolutePath(), - "default.allowStoreCreation", String.valueOf(true))); + store.initialize(defaultStoreProperties()); } @Test @@ -108,6 +111,38 @@ public void stores_violations_of_multiple_rules() { assertThat(store.getViolations(thirdRule)).containsOnly("third violation1", "third violation2"); } + @Test + public void is_safe_when_multiple_instances_save_to_the_same_store_concurrently() throws Exception { + int ruleCount = 20; + try (ExecutorService executor = Executors.newFixedThreadPool(8)) { + CountDownLatch startSignal = new CountDownLatch(1); + + IntStream.range(0, ruleCount).forEach(i -> executor.submit(() -> { + ArchRule concurrentRule = rule("concurrent rule " + i); + ViolationStore instanceStore = new TextFileBasedViolationStore(); + instanceStore.initialize(defaultStoreProperties()); + startSignal.await(); + instanceStore.save(concurrentRule, ImmutableList.of("violation " + i)); + return null; + })); + + startSignal.countDown(); + executor.shutdown(); + assertThat(executor.awaitTermination(30, TimeUnit.SECONDS)).isTrue(); + } + + 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); + } + } + @Test public void stores_violations_with_line_breaks() { List expected = ImmutableList.of(String.format("first with%nlinebreak"), String.format("second with%nlinebreak")); @@ -126,6 +161,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)); From 85656a5e30510c7e2da3f29da8eb1e2508565dd0 Mon Sep 17 00:00:00 2001 From: Niklas Keller Date: Sat, 11 Jul 2026 19:47:26 +0200 Subject: [PATCH 2/3] Refine thread-safety fix and tests for TextFileBasedViolationStore - Move directory creation into FileSyncedProperties.initializePropertiesFile, removing the separate ensureExistence call from initialize() - Use mkdirs() || isDirectory() to handle the race where a concurrent process creates the directory between our mkdirs() call returning false and us checking - Extract concurrency tests into TextFileBasedViolationStoreConcurrencyTest, separate initialize-race and save-race scenarios, and call future.get() to surface exceptions that would otherwise be silently swallowed by the executor - Convert both test classes from JUnit 4 to JUnit 5 Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: Niklas Keller --- .../freeze/TextFileBasedViolationStore.java | 24 ++-- ...ileBasedViolationStoreConcurrencyTest.java | 129 ++++++++++++++++++ .../TextFileBasedViolationStoreTest.java | 55 ++------ 3 files changed, 152 insertions(+), 56 deletions(-) create mode 100644 archunit/src/test/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStoreConcurrencyTest.java 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 c0b1aa9fbc..fd2bcc56e5 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 @@ -33,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; @@ -108,7 +107,6 @@ 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 = getOrCreateStoredRules(storedRulesFile); @@ -133,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)); @@ -231,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) { 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 0000000000..fe1021423f --- /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); + } + } + + private void runConcurrently(int taskCount, ConcurrentTask task) throws Exception { + try (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 d1b063b344..202a786dfa 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,22 +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 java.util.concurrent.CountDownLatch; -import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; -import java.util.concurrent.TimeUnit; -import java.util.stream.IntStream; 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; @@ -28,15 +23,15 @@ 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(defaultStoreProperties()); } @@ -111,38 +106,6 @@ public void stores_violations_of_multiple_rules() { assertThat(store.getViolations(thirdRule)).containsOnly("third violation1", "third violation2"); } - @Test - public void is_safe_when_multiple_instances_save_to_the_same_store_concurrently() throws Exception { - int ruleCount = 20; - try (ExecutorService executor = Executors.newFixedThreadPool(8)) { - CountDownLatch startSignal = new CountDownLatch(1); - - IntStream.range(0, ruleCount).forEach(i -> executor.submit(() -> { - ArchRule concurrentRule = rule("concurrent rule " + i); - ViolationStore instanceStore = new TextFileBasedViolationStore(); - instanceStore.initialize(defaultStoreProperties()); - startSignal.await(); - instanceStore.save(concurrentRule, ImmutableList.of("violation " + i)); - return null; - })); - - startSignal.countDown(); - executor.shutdown(); - assertThat(executor.awaitTermination(30, TimeUnit.SECONDS)).isTrue(); - } - - 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); - } - } - @Test public void stores_violations_with_line_breaks() { List expected = ImmutableList.of(String.format("first with%nlinebreak"), String.format("second with%nlinebreak")); From e8477853ac1c97ff8f52a2d4832e4e06cff953bb Mon Sep 17 00:00:00 2001 From: Niklas Keller Date: Wed, 15 Jul 2026 21:38:09 +0200 Subject: [PATCH 3/3] fix AutoClosable not being implemented in < Java 19 Signed-off-by: Niklas Keller --- ...ileBasedViolationStoreConcurrencyTest.java | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) 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 index fe1021423f..22f222a29c 100644 --- a/archunit/src/test/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStoreConcurrencyTest.java +++ b/archunit/src/test/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStoreConcurrencyTest.java @@ -73,25 +73,25 @@ public void is_safe_when_multiple_instances_save_to_the_same_store_concurrently( } } + @SuppressWarnings("resource") // only available in Java 19 and later private void runConcurrently(int taskCount, ConcurrentTask task) throws Exception { - try (ExecutorService executor = Executors.newFixedThreadPool(8)) { - CountDownLatch startSignal = new CountDownLatch(1); - List> futures = new ArrayList<>(); + 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; - }))); + IntStream.range(0, taskCount).forEach(i -> futures.add(executor.submit(() -> { + task.run(i, startSignal); + return null; + }))); - startSignal.countDown(); - executor.shutdown(); + startSignal.countDown(); + executor.shutdown(); - assertThat(executor.awaitTermination(30, TimeUnit.SECONDS)).isTrue(); + 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(); - } + // exceptions thrown inside submitted tasks are swallowed unless we call get() on each future + for (Future future : futures) { + future.get(); } }