Skip to content

Commit 7518e4c

Browse files
kelunikclaude
andcommitted
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 <noreply@anthropic.com>
1 parent 4c35e53 commit 7518e4c

2 files changed

Lines changed: 71 additions & 16 deletions

File tree

archunit/src/main/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStore.java

Lines changed: 27 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import java.util.List;
2424
import java.util.Properties;
2525
import java.util.UUID;
26+
import java.util.concurrent.ConcurrentHashMap;
2627
import java.util.regex.Pattern;
2728

2829
import com.google.common.base.Splitter;
@@ -74,6 +75,8 @@ public final class TextFileBasedViolationStore implements ViolationStore {
7475
private static final String ALLOW_STORE_UPDATE_PROPERTY_NAME = "default.allowStoreUpdate";
7576
private static final String ALLOW_STORE_UPDATE_DEFAULT = "true";
7677

78+
private static final ConcurrentHashMap<String, FileSyncedProperties> STORED_RULES_BY_PATH = new ConcurrentHashMap<>();
79+
7780
private final RuleViolationFileNameStrategy ruleViolationFileNameStrategy;
7881

7982
private boolean storeCreationAllowed;
@@ -108,10 +111,18 @@ public void initialize(Properties properties) {
108111
ensureExistence(storeFolder);
109112
File storedRulesFile = getStoredRulesFile();
110113
log.trace("Initializing {} at {}", TextFileBasedViolationStore.class.getSimpleName(), storedRulesFile.getAbsolutePath());
111-
storedRules = new FileSyncedProperties(storedRulesFile);
114+
storedRules = getOrCreateStoredRules(storedRulesFile);
112115
checkInitialization(storedRules.initializationSuccessful(), "Cannot create rule store at %s", storedRulesFile.getAbsolutePath());
113116
}
114117

118+
private FileSyncedProperties getOrCreateStoredRules(File storedRulesFile) {
119+
try {
120+
return STORED_RULES_BY_PATH.computeIfAbsent(storedRulesFile.getCanonicalPath(), path -> new FileSyncedProperties(storedRulesFile));
121+
} catch (IOException e) {
122+
throw new StoreInitializationFailedException(e);
123+
}
124+
}
125+
115126
private File getStoredRulesFile() {
116127
File rulesFile = new File(storeFolder, STORED_RULES_FILE_NAME);
117128
if (!rulesFile.exists() && !storeCreationAllowed) {
@@ -171,17 +182,14 @@ private String unescape(String violation) {
171182

172183
private String ensureRuleFileName(ArchRule rule) {
173184
String ruleDescription = rule.getDescription();
174-
175-
String ruleFileName;
176-
if (storedRules.containsKey(ruleDescription)) {
177-
ruleFileName = storedRules.getProperty(ruleDescription);
178-
log.trace("Rule '{}' is already stored in file {}", ruleDescription, ruleFileName);
179-
} else {
180-
ruleFileName = ruleViolationFileNameStrategy.createRuleFileName(ruleDescription);
181-
log.trace("Assigning new file {} to rule '{}'", ruleFileName, ruleDescription);
182-
storedRules.setProperty(ruleDescription, ruleFileName);
185+
String candidateFileName = ruleViolationFileNameStrategy.createRuleFileName(ruleDescription);
186+
String existingFileName = storedRules.putIfAbsent(ruleDescription, candidateFileName);
187+
if (existingFileName == null) {
188+
log.trace("Assigning new file {} to rule '{}'", candidateFileName, ruleDescription);
189+
return candidateFileName;
183190
}
184-
return ruleFileName;
191+
log.trace("Rule '{}' is already stored in file {}", ruleDescription, existingFileName);
192+
return existingFileName;
185193
}
186194

187195
@Override
@@ -250,9 +258,15 @@ String getProperty(String propertyName) {
250258
return loadedProperties.getProperty(ensureUnixLineBreaks(propertyName));
251259
}
252260

253-
void setProperty(String propertyName, String value) {
254-
loadedProperties.setProperty(ensureUnixLineBreaks(propertyName), ensureUnixLineBreaks(value));
261+
synchronized String putIfAbsent(String key, String value) {
262+
String normalizedKey = ensureUnixLineBreaks(key);
263+
String existing = loadedProperties.getProperty(normalizedKey);
264+
if (existing != null) {
265+
return existing;
266+
}
267+
loadedProperties.setProperty(normalizedKey, ensureUnixLineBreaks(value));
255268
syncFileSystem();
269+
return null;
256270
}
257271

258272
private void syncFileSystem() {

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

Lines changed: 44 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,11 @@
66
import java.util.LinkedList;
77
import java.util.List;
88
import java.util.Properties;
9+
import java.util.concurrent.CountDownLatch;
10+
import java.util.concurrent.ExecutorService;
11+
import java.util.concurrent.Executors;
12+
import java.util.concurrent.TimeUnit;
13+
import java.util.stream.IntStream;
914

1015
import com.google.common.collect.ImmutableList;
1116
import com.google.common.io.Files;
@@ -33,9 +38,7 @@ public class TextFileBasedViolationStoreTest {
3338
public void setUp() throws Exception {
3439
configuredFolder = new File(temporaryFolder.newFolder(), "notyetthere");
3540

36-
store.initialize(propertiesOf(
37-
"default.path", configuredFolder.getAbsolutePath(),
38-
"default.allowStoreCreation", String.valueOf(true)));
41+
store.initialize(defaultStoreProperties());
3942
}
4043

4144
@Test
@@ -108,6 +111,38 @@ public void stores_violations_of_multiple_rules() {
108111
assertThat(store.getViolations(thirdRule)).containsOnly("third violation1", "third violation2");
109112
}
110113

114+
@Test
115+
public void is_safe_when_multiple_instances_save_to_the_same_store_concurrently() throws Exception {
116+
int ruleCount = 20;
117+
try (ExecutorService executor = Executors.newFixedThreadPool(8)) {
118+
CountDownLatch startSignal = new CountDownLatch(1);
119+
120+
IntStream.range(0, ruleCount).forEach(i -> executor.submit(() -> {
121+
ArchRule concurrentRule = rule("concurrent rule " + i);
122+
ViolationStore instanceStore = new TextFileBasedViolationStore();
123+
instanceStore.initialize(defaultStoreProperties());
124+
startSignal.await();
125+
instanceStore.save(concurrentRule, ImmutableList.of("violation " + i));
126+
return null;
127+
}));
128+
129+
startSignal.countDown();
130+
executor.shutdown();
131+
assertThat(executor.awaitTermination(30, TimeUnit.SECONDS)).isTrue();
132+
}
133+
134+
Properties storedRules = readProperties(new File(configuredFolder, "stored.rules"));
135+
assertThat(storedRules).hasSize(ruleCount);
136+
137+
ViolationStore readerStore = new TextFileBasedViolationStore();
138+
readerStore.initialize(defaultStoreProperties());
139+
for (int i = 0; i < ruleCount; i++) {
140+
ArchRule concurrentRule = rule("concurrent rule " + i);
141+
assertThat(readerStore.contains(concurrentRule)).as("store contains " + concurrentRule.getDescription()).isTrue();
142+
assertThat(readerStore.getViolations(concurrentRule)).containsOnly("violation " + i);
143+
}
144+
}
145+
111146
@Test
112147
public void stores_violations_with_line_breaks() {
113148
List<String> expected = ImmutableList.of(String.format("first with%nlinebreak"), String.format("second with%nlinebreak"));
@@ -126,6 +161,12 @@ private Properties readProperties(File file) throws IOException {
126161
return properties;
127162
}
128163

164+
private Properties defaultStoreProperties() {
165+
return propertiesOf(
166+
"default.path", configuredFolder.getAbsolutePath(),
167+
"default.allowStoreCreation", String.valueOf(true));
168+
}
169+
129170
private Properties propertiesOf(String... keyValuePairs) {
130171
Properties result = new Properties();
131172
LinkedList<String> keyValues = new LinkedList<>(asList(keyValuePairs));

0 commit comments

Comments
 (0)