CLDR-15913 Make LogicalGrouping thread-safe. (#2424)

LogicalGrouping.getPaths might be called from different threads created in GenerateProductionData. Wrapping the existing cache into Multimaps.synchronizedMultimap is not enough as underlying ArrayList is also should be synchronized: it is traversed in new TreeSet<>(cache.get()) and modified in cache.putAll() calls. srl295: Also synchronized localeToSubdivisionsToMigrate Co-authored-by: Steven R. Loomis <[email protected]> Co-authored-by: Steven R. Loomis <[email protected]>
unicode-org · Oct 5, 2022 · 45498f1 · 45498f1
1 parent 15fba13
commit 45498f1
Show file tree

Hide file tree

Showing 2 changed files with 22 additions and 11 deletions.
diff --git a/tools/cldr-code/src/main/java/org/unicode/cldr/tool/GenerateProductionData.java b/tools/cldr-code/src/main/java/org/unicode/cldr/tool/GenerateProductionData.java
@@ -139,8 +139,8 @@ public static void main(String[] args) {
         // get directories
 
         Arrays.asList(DtdType.values())
-            //.parallelStream()
-            //.unordered()
+            .parallelStream()
+            .unordered()
             .forEach(type -> {
             boolean isLdmlDtdType = type == DtdType.ldml;
 
@@ -154,6 +154,7 @@ public static void main(String[] args) {
                 copyFilesAndReturnIsEmpty(sourceDir, destinationDir, null, isLdmlDtdType, stats);
             }
         });
+        // should be called from the main thread. Synchronizing to document.
         if (!localeToSubdivisionsToMigrate.isEmpty()) {
             System.err.println("WARNING: Subdivision files not written, " + localeToSubdivisionsToMigrate.size() + " entries\n" +
                     "For locales: " + localeToSubdivisionsToMigrate.keySet());
@@ -229,7 +230,7 @@ private static boolean copyFilesAndReturnIsEmpty(File sourceFile, File destinati
             final Factory theFactory = factory;
             final boolean isLdmlDtdType2 = isLdmlDtdType;
             sorted
-                //.parallelStream()
+                .parallelStream()
                 .forEach(file -> {
                     File sourceFile2 = new File(sourceFile, file);
                     File destinationFile2 = new File(destinationFile, file);
@@ -333,7 +334,9 @@ private static boolean copyFilesAndReturnIsEmpty(File sourceFile, File destinati
 
                 // Move subdivisions elsewhere
                 if (!isSubdivisionDirectory && xpath.startsWith("//ldml/localeDisplayNames/subdivisions/subdivision")) {
-                    localeToSubdivisionsToMigrate.put(localeId, Pair.of(xpath, value));
+                    synchronized(localeToSubdivisionsToMigrate) {
+                        localeToSubdivisionsToMigrate.put(localeId, Pair.of(xpath, value));
+                    }
                     toRemove.add(xpath);
                     continue;
                 }
@@ -377,12 +380,14 @@ private static boolean copyFilesAndReturnIsEmpty(File sourceFile, File destinati
             try (PrintWriter pw = new PrintWriter(destinationFile)) {
                 CLDRFile outCldrFile = cldrFileUnresolved.cloneAsThawed();
                 if (isSubdivisionDirectory) {
-                    Collection<Pair<String, String>> path_values = localeToSubdivisionsToMigrate.get(localeId);
-                    if (path_values != null) {
-                        for (Pair<String, String>path_value : path_values) {
-                            outCldrFile.add(path_value.getFirst(), path_value.getSecond());
+                    synchronized (localeToSubdivisionsToMigrate) {
+                        Collection<Pair<String, String>> path_values = localeToSubdivisionsToMigrate.get(localeId);
+                        if (path_values != null) {
+                            for (Pair<String, String>path_value : path_values) {
+                                outCldrFile.add(path_value.getFirst(), path_value.getSecond());
+                            }
+                            localeToSubdivisionsToMigrate.removeAll(localeId);
                         }
-                        localeToSubdivisionsToMigrate.removeAll(localeId);
                     }
                 }
 

diff --git a/tools/cldr-code/src/main/java/org/unicode/cldr/util/LogicalGrouping.java b/tools/cldr-code/src/main/java/org/unicode/cldr/util/LogicalGrouping.java
@@ -55,7 +55,7 @@ public class LogicalGrouping {
     /**
      * Cache from path (String) to logical group (Set<String>)
      */
-    private static Multimap<String, String> cachePathToLogicalGroup = ArrayListMultimap.create();
+    private static final ConcurrentHashMap<String, Set<String>> cachePathToLogicalGroup = new ConcurrentHashMap<>();
 
     /**
      * Cache from locale and path (<Pair<String, String>), to logical group (Set<String>)
@@ -156,7 +156,13 @@ public static Set<String> getPaths(CLDRFile cldrFile, String path, Output<PathTy
             }
             Set<String> set = new TreeSet<>();
             pathType.addPaths(set, cldrFile, path, parts);
-            cachePathToLogicalGroup.putAll(path, set);
+            cachePathToLogicalGroup.compute(path, (pathKey, cachedPaths) -> {
+                if (cachedPaths == null) {
+                    return Collections.synchronizedSet(new HashSet<>(set));
+                } else {
+                    cachedPaths.addAll(set);
+                    return cachedPaths;
+                }});
             return set;
         }
     }