Skip to content

Commit ffe107b

Browse files
committed
Revert dataStoreMap from ConcurrentHashMap to LinkedHashMap
Based on feedback that dataStoreMap is not accessed by multiple threads, reverted the ConcurrentHashMap change back to LinkedHashMap. Changes: - Reverted dataStoreMap from ConcurrentHashMap to LinkedHashMap - Removed ConcurrentHashMap import - Removed dataStoreMap-related concurrency tests: * test_add_concurrentAccess * test_getDataStore_concurrentWithAdd * test_nullSafety_concurrentAccess Kept volatile fields and synchronized getDataStoreNames(): - lastLoadedTime and dataStoreNames remain volatile (for cache visibility) - getDataStoreNames() remains synchronized (for cache refresh safety) - Related tests for getDataStoreNames() concurrency are maintained The dataStoreMap is accessed in single-threaded context during application initialization and data store registration.
1 parent b399d14 commit ffe107b

File tree

2 files changed

+2
-133
lines changed

2 files changed

+2
-133
lines changed

src/main/java/org/codelibs/fess/ds/DataStoreFactory.java

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@
2727
import java.util.Locale;
2828
import java.util.Map;
2929
import java.util.Set;
30-
import java.util.concurrent.ConcurrentHashMap;
3130
import java.util.stream.Collectors;
3231

3332
import javax.xml.parsers.DocumentBuilder;
@@ -65,9 +64,8 @@ public class DataStoreFactory {
6564
/**
6665
* Map containing registered data store instances indexed by their names and class simple names.
6766
* All keys are stored in lowercase for case-insensitive lookup.
68-
* Thread-safe implementation using ConcurrentHashMap for concurrent access.
6967
*/
70-
protected Map<String, DataStore> dataStoreMap = new ConcurrentHashMap<>();
68+
protected Map<String, DataStore> dataStoreMap = new LinkedHashMap<>();
7169

7270
/**
7371
* Cached array of available data store names discovered from plugin JAR files.

src/test/java/org/codelibs/fess/ds/DataStoreFactoryTest.java

Lines changed: 1 addition & 130 deletions
Original file line numberDiff line numberDiff line change
@@ -338,105 +338,7 @@ protected void storeData(DataConfig dataConfig, IndexUpdateCallback callback, Da
338338
}
339339
}
340340

341-
// ========== Thread Safety Tests ==========
342-
343-
/**
344-
* Test concurrent add operations to verify ConcurrentHashMap thread safety.
345-
* Multiple threads simultaneously add different data stores.
346-
*/
347-
public void test_add_concurrentAccess() throws Exception {
348-
final int threadCount = 10;
349-
final Thread[] threads = new Thread[threadCount];
350-
final Exception[] exceptions = new Exception[threadCount];
351-
352-
for (int i = 0; i < threadCount; i++) {
353-
final int index = i;
354-
threads[i] = new Thread(() -> {
355-
try {
356-
TestDataStore dataStore = new TestDataStore("Store" + index);
357-
dataStoreFactory.add("store" + index, dataStore);
358-
} catch (Exception e) {
359-
exceptions[index] = e;
360-
}
361-
});
362-
}
363-
364-
// Start all threads
365-
for (Thread thread : threads) {
366-
thread.start();
367-
}
368-
369-
// Wait for all threads to complete
370-
for (Thread thread : threads) {
371-
thread.join();
372-
}
373-
374-
// Verify no exceptions occurred
375-
for (int i = 0; i < threadCount; i++) {
376-
assertNull("Thread " + i + " threw exception", exceptions[i]);
377-
}
378-
379-
// Verify all data stores were registered
380-
for (int i = 0; i < threadCount; i++) {
381-
assertNotNull("DataStore " + i + " not found", dataStoreFactory.getDataStore("store" + i));
382-
}
383-
}
384-
385-
/**
386-
* Test concurrent get operations while adding new data stores.
387-
* Verifies that reads and writes can happen concurrently without issues.
388-
*/
389-
public void test_getDataStore_concurrentWithAdd() throws Exception {
390-
// Pre-populate with some data stores
391-
for (int i = 0; i < 5; i++) {
392-
TestDataStore dataStore = new TestDataStore("InitialStore" + i);
393-
dataStoreFactory.add("initial" + i, dataStore);
394-
}
395-
396-
final int threadCount = 10;
397-
final Thread[] threads = new Thread[threadCount];
398-
final Exception[] exceptions = new Exception[threadCount];
399-
final int[] successCount = new int[threadCount];
400-
401-
for (int i = 0; i < threadCount; i++) {
402-
final int index = i;
403-
threads[i] = new Thread(() -> {
404-
try {
405-
for (int j = 0; j < 100; j++) {
406-
// Half threads read, half threads write
407-
if (index % 2 == 0) {
408-
DataStore ds = dataStoreFactory.getDataStore("initial" + (j % 5));
409-
if (ds != null) {
410-
successCount[index]++;
411-
}
412-
} else {
413-
TestDataStore dataStore = new TestDataStore("ConcurrentStore" + index + "_" + j);
414-
dataStoreFactory.add("concurrent" + index + "_" + j, dataStore);
415-
successCount[index]++;
416-
}
417-
}
418-
} catch (Exception e) {
419-
exceptions[index] = e;
420-
}
421-
});
422-
}
423-
424-
// Start all threads
425-
for (Thread thread : threads) {
426-
thread.start();
427-
}
428-
429-
// Wait for all threads to complete
430-
for (Thread thread : threads) {
431-
thread.join();
432-
}
433-
434-
// Verify no exceptions occurred
435-
for (int i = 0; i < threadCount; i++) {
436-
assertNull("Thread " + i + " threw exception", exceptions[i]);
437-
assertEquals("Thread " + i + " didn't complete all operations", 100, successCount[i]);
438-
}
439-
}
341+
// ========== Thread Safety Tests for getDataStoreNames() ==========
440342

441343
/**
442344
* Test concurrent getDataStoreNames calls to verify synchronized method works correctly.
@@ -582,35 +484,4 @@ protected List<String> loadDataStoreNameList() {
582484
// The count might be 2 (one initial + one reload) or slightly higher due to timing
583485
assertTrue("Load count should be small due to synchronization", loadCount[0] <= 3);
584486
}
585-
586-
/**
587-
* Test null safety with concurrent access.
588-
* Verifies that null checks work correctly under concurrent load.
589-
*/
590-
public void test_nullSafety_concurrentAccess() throws Exception {
591-
final int threadCount = 10;
592-
final Thread[] threads = new Thread[threadCount];
593-
final DataStore[] results = new DataStore[threadCount];
594-
595-
for (int i = 0; i < threadCount; i++) {
596-
final int index = i;
597-
threads[i] = new Thread(() -> {
598-
// Try to get non-existent data store
599-
results[index] = dataStoreFactory.getDataStore(null);
600-
});
601-
}
602-
603-
for (Thread thread : threads) {
604-
thread.start();
605-
}
606-
607-
for (Thread thread : threads) {
608-
thread.join();
609-
}
610-
611-
// All should return null without exceptions
612-
for (int i = 0; i < threadCount; i++) {
613-
assertNull("Thread " + i + " should get null", results[i]);
614-
}
615-
}
616487
}

0 commit comments

Comments
 (0)