From 1a66eb5ffb771591536a9e34cec589f0a8b66ecc Mon Sep 17 00:00:00 2001 From: Per Minborg Date: Tue, 3 Jun 2025 22:00:35 +0200 Subject: [PATCH 1/3] Fix concurrency issues --- .../classes/java/util/ResourceBundle.java | 35 +++---- .../BreakIteratorResourceBundle.java | 27 +++--- .../resources/OpenListResourceBundle.java | 96 ++++++++----------- 3 files changed, 71 insertions(+), 87 deletions(-) diff --git a/src/java.base/share/classes/java/util/ResourceBundle.java b/src/java.base/share/classes/java/util/ResourceBundle.java index fb88a38903a62..7df6e91466519 100644 --- a/src/java.base/share/classes/java/util/ResourceBundle.java +++ b/src/java.base/share/classes/java/util/ResourceBundle.java @@ -54,6 +54,7 @@ import java.net.URLConnection; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; +import java.util.function.Supplier; import java.util.jar.JarEntry; import java.util.spi.ResourceBundleControlProvider; import java.util.spi.ResourceBundleProvider; @@ -487,7 +488,22 @@ public String getBaseBundleName() { /** * A Set of the keys contained only in this ResourceBundle. */ - private volatile Set keySet; + private final Supplier> keySet = StableValue.supplier( + new Supplier>() { + @Override + public Set get() { + final Set keys = new HashSet<>(); + final Enumeration enumKeys = getKeys(); + while (enumKeys.hasMoreElements()) { + final String key = enumKeys.nextElement(); + if (handleGetObject(key) != null) { + keys.add(key); + } + } + return Set.copyOf(keys); + } + } + ); /** * Sole constructor. (For invocation by subclass constructors, typically @@ -2298,22 +2314,7 @@ public Set keySet() { * @since 1.6 */ protected Set handleKeySet() { - if (keySet == null) { - synchronized (this) { - if (keySet == null) { - Set keys = new HashSet<>(); - Enumeration enumKeys = getKeys(); - while (enumKeys.hasMoreElements()) { - String key = enumKeys.nextElement(); - if (handleGetObject(key) != null) { - keys.add(key); - } - } - keySet = keys; - } - } - } - return keySet; + return keySet.get(); } diff --git a/src/java.base/share/classes/sun/util/resources/BreakIteratorResourceBundle.java b/src/java.base/share/classes/sun/util/resources/BreakIteratorResourceBundle.java index 3a55e7ccb8c58..9f3eb99a60c09 100644 --- a/src/java.base/share/classes/sun/util/resources/BreakIteratorResourceBundle.java +++ b/src/java.base/share/classes/sun/util/resources/BreakIteratorResourceBundle.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2016, 2024, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2016, 2025, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -30,6 +30,7 @@ import java.util.Enumeration; import java.util.ResourceBundle; import java.util.Set; +import java.util.function.Supplier; /** * BreakIteratorResourceBundle is an abstract class for loading BreakIterator @@ -49,7 +50,17 @@ public abstract class BreakIteratorResourceBundle extends ResourceBundle { // those keys must be added to NON_DATA_KEYS. private static final Set NON_DATA_KEYS = Set.of("BreakIteratorClasses"); - private volatile Set keys; + private final Supplier> keys = StableValue.supplier( + new Supplier>() { + @Override + public Set get() { + final ResourceBundle info = getBreakIteratorInfo(); + final Set k = info.keySet(); + k.removeAll(NON_DATA_KEYS); + return Set.copyOf(k); + } + } + ); /** * Returns an instance of the corresponding {@code BreakIteratorInfo} (basename). @@ -84,16 +95,6 @@ public Enumeration getKeys() { @Override protected Set handleKeySet() { - if (keys == null) { - ResourceBundle info = getBreakIteratorInfo(); - Set k = info.keySet(); - k.removeAll(NON_DATA_KEYS); - synchronized (this) { - if (keys == null) { - keys = k; - } - } - } - return keys; + return keys.get(); } } diff --git a/src/java.base/share/classes/sun/util/resources/OpenListResourceBundle.java b/src/java.base/share/classes/sun/util/resources/OpenListResourceBundle.java index d375930dde066..847ca57f972d0 100644 --- a/src/java.base/share/classes/sun/util/resources/OpenListResourceBundle.java +++ b/src/java.base/share/classes/sun/util/resources/OpenListResourceBundle.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2005, 2022, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2005, 2025, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -44,8 +44,11 @@ import java.util.HashMap; import java.util.HashSet; import java.util.Map; +import java.util.Objects; import java.util.ResourceBundle; import java.util.Set; +import java.util.function.Supplier; + import sun.util.ResourceBundleEnumeration; /** @@ -69,12 +72,9 @@ protected OpenListResourceBundle() { // Implements java.util.ResourceBundle.handleGetObject; inherits javadoc specification. @Override protected Object handleGetObject(String key) { - if (key == null) { - throw new NullPointerException(); - } - - loadLookupTablesIfNecessary(); - return lookup.get(key); // this class ignores locales + Objects.requireNonNull(key); + return lookup.get() + .get(key); // this class ignores locales } /** @@ -93,26 +93,13 @@ public Enumeration getKeys() { */ @Override protected Set handleKeySet() { - loadLookupTablesIfNecessary(); - return lookup.keySet(); + return lookup.get() + .keySet(); } @Override public Set keySet() { - if (keyset != null) { - return keyset; - } - Set ks = createSet(); - ks.addAll(handleKeySet()); - if (parent != null) { - ks.addAll(parent.keySet()); - } - synchronized (this) { - if (keyset == null) { - keyset = ks; - } - } - return keyset; + return keyset.get(); } /** @@ -120,38 +107,6 @@ public Set keySet() { */ protected abstract Object[][] getContents(); - /** - * Load lookup tables if they haven't been loaded already. - */ - void loadLookupTablesIfNecessary() { - if (lookup == null) { - loadLookup(); - } - } - - /** - * We lazily load the lookup hashtable. This function does the - * loading. - */ - private void loadLookup() { - Object[][] contents = getContents(); - Map temp = createMap(contents.length); - for (int i = 0; i < contents.length; ++i) { - // key must be non-null String, value must be non-null - String key = (String) contents[i][0]; - Object value = contents[i][1]; - if (key == null || value == null) { - throw new NullPointerException(); - } - temp.put(key, value); - } - synchronized (this) { - if (lookup == null) { - lookup = temp; - } - } - } - /** * Lets subclasses provide specialized Map implementations. * Default uses HashMap. @@ -164,6 +119,33 @@ protected Set createSet() { return new HashSet<>(); } - private volatile Map lookup; - private volatile Set keyset; + private final Supplier> lookup = StableValue.supplier( + new Supplier>() { + @Override + public Map get() { + final Object[][] contents = getContents(); + final Map temp = createMap(contents.length); + for (Object[] content : contents) { + // key must be non-null String, value must be non-null + final String key = Objects.requireNonNull((String) content[0]); + final Object value = Objects.requireNonNull(content[1]); + temp.put(key, value); + } + return Map.copyOf(temp); + } + } + ); + private final Supplier> keyset = StableValue.supplier( + new Supplier>() { + @Override + public Set get() { + final Set ks = createSet(); + ks.addAll(handleKeySet()); + if (parent != null) { + ks.addAll(parent.keySet()); + } + return Set.copyOf(ks); + } + } + ); } From e36a2773aab2c300565baf2cb26f275da88fd7e9 Mon Sep 17 00:00:00 2001 From: Per Minborg Date: Wed, 4 Jun 2025 15:43:03 +0200 Subject: [PATCH 2/3] Revoke the use of immutable collections --- src/java.base/share/classes/java/util/ResourceBundle.java | 2 +- .../sun/util/resources/BreakIteratorResourceBundle.java | 2 +- .../classes/sun/util/resources/OpenListResourceBundle.java | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/java.base/share/classes/java/util/ResourceBundle.java b/src/java.base/share/classes/java/util/ResourceBundle.java index 7df6e91466519..f87616bae2982 100644 --- a/src/java.base/share/classes/java/util/ResourceBundle.java +++ b/src/java.base/share/classes/java/util/ResourceBundle.java @@ -500,7 +500,7 @@ public Set get() { keys.add(key); } } - return Set.copyOf(keys); + return keys; } } ); diff --git a/src/java.base/share/classes/sun/util/resources/BreakIteratorResourceBundle.java b/src/java.base/share/classes/sun/util/resources/BreakIteratorResourceBundle.java index 9f3eb99a60c09..3b1b28f45fcdc 100644 --- a/src/java.base/share/classes/sun/util/resources/BreakIteratorResourceBundle.java +++ b/src/java.base/share/classes/sun/util/resources/BreakIteratorResourceBundle.java @@ -57,7 +57,7 @@ public Set get() { final ResourceBundle info = getBreakIteratorInfo(); final Set k = info.keySet(); k.removeAll(NON_DATA_KEYS); - return Set.copyOf(k); + return k; } } ); diff --git a/src/java.base/share/classes/sun/util/resources/OpenListResourceBundle.java b/src/java.base/share/classes/sun/util/resources/OpenListResourceBundle.java index 847ca57f972d0..d02ab51b591a9 100644 --- a/src/java.base/share/classes/sun/util/resources/OpenListResourceBundle.java +++ b/src/java.base/share/classes/sun/util/resources/OpenListResourceBundle.java @@ -131,7 +131,7 @@ public Map get() { final Object value = Objects.requireNonNull(content[1]); temp.put(key, value); } - return Map.copyOf(temp); + return temp; } } ); @@ -144,7 +144,7 @@ public Set get() { if (parent != null) { ks.addAll(parent.keySet()); } - return Set.copyOf(ks); + return ks; } } ); From de4fea12f37b9d869230067196f9613b0630384f Mon Sep 17 00:00:00 2001 From: Per Minborg Date: Wed, 4 Jun 2025 20:16:18 +0200 Subject: [PATCH 3/3] Break out logic in separate methods --- .../classes/java/util/ResourceBundle.java | 26 +++++----- .../BreakIteratorResourceBundle.java | 18 +++---- .../resources/OpenListResourceBundle.java | 52 +++++++++---------- 3 files changed, 45 insertions(+), 51 deletions(-) diff --git a/src/java.base/share/classes/java/util/ResourceBundle.java b/src/java.base/share/classes/java/util/ResourceBundle.java index f87616bae2982..b3807ac770ae9 100644 --- a/src/java.base/share/classes/java/util/ResourceBundle.java +++ b/src/java.base/share/classes/java/util/ResourceBundle.java @@ -489,21 +489,19 @@ public String getBaseBundleName() { * A Set of the keys contained only in this ResourceBundle. */ private final Supplier> keySet = StableValue.supplier( - new Supplier>() { - @Override - public Set get() { - final Set keys = new HashSet<>(); - final Enumeration enumKeys = getKeys(); - while (enumKeys.hasMoreElements()) { - final String key = enumKeys.nextElement(); - if (handleGetObject(key) != null) { - keys.add(key); - } - } - return keys; - } + new Supplier<>() { public Set get() { return keySet0(); }}); + + private Set keySet0() { + final Set keys = new HashSet<>(); + final Enumeration enumKeys = getKeys(); + while (enumKeys.hasMoreElements()) { + final String key = enumKeys.nextElement(); + if (handleGetObject(key) != null) { + keys.add(key); } - ); + } + return keys; + } /** * Sole constructor. (For invocation by subclass constructors, typically diff --git a/src/java.base/share/classes/sun/util/resources/BreakIteratorResourceBundle.java b/src/java.base/share/classes/sun/util/resources/BreakIteratorResourceBundle.java index 3b1b28f45fcdc..d7c575962f627 100644 --- a/src/java.base/share/classes/sun/util/resources/BreakIteratorResourceBundle.java +++ b/src/java.base/share/classes/sun/util/resources/BreakIteratorResourceBundle.java @@ -51,16 +51,14 @@ public abstract class BreakIteratorResourceBundle extends ResourceBundle { private static final Set NON_DATA_KEYS = Set.of("BreakIteratorClasses"); private final Supplier> keys = StableValue.supplier( - new Supplier>() { - @Override - public Set get() { - final ResourceBundle info = getBreakIteratorInfo(); - final Set k = info.keySet(); - k.removeAll(NON_DATA_KEYS); - return k; - } - } - ); + new Supplier<>() { public Set get() { return keys0(); }}); + + private Set keys0() { + final ResourceBundle info = getBreakIteratorInfo(); + final Set k = info.keySet(); + k.removeAll(NON_DATA_KEYS); + return k; + } /** * Returns an instance of the corresponding {@code BreakIteratorInfo} (basename). diff --git a/src/java.base/share/classes/sun/util/resources/OpenListResourceBundle.java b/src/java.base/share/classes/sun/util/resources/OpenListResourceBundle.java index d02ab51b591a9..0c16f50801875 100644 --- a/src/java.base/share/classes/sun/util/resources/OpenListResourceBundle.java +++ b/src/java.base/share/classes/sun/util/resources/OpenListResourceBundle.java @@ -120,32 +120,30 @@ protected Set createSet() { } private final Supplier> lookup = StableValue.supplier( - new Supplier>() { - @Override - public Map get() { - final Object[][] contents = getContents(); - final Map temp = createMap(contents.length); - for (Object[] content : contents) { - // key must be non-null String, value must be non-null - final String key = Objects.requireNonNull((String) content[0]); - final Object value = Objects.requireNonNull(content[1]); - temp.put(key, value); - } - return temp; - } - } - ); + new Supplier<>() { public Map get() { return lookup0(); }}); + + private Map lookup0() { + final Object[][] contents = getContents(); + final Map temp = createMap(contents.length); + for (Object[] content : contents) { + // key must be non-null String, value must be non-null + final String key = Objects.requireNonNull((String) content[0]); + final Object value = Objects.requireNonNull(content[1]); + temp.put(key, value); + } + return temp; + } + private final Supplier> keyset = StableValue.supplier( - new Supplier>() { - @Override - public Set get() { - final Set ks = createSet(); - ks.addAll(handleKeySet()); - if (parent != null) { - ks.addAll(parent.keySet()); - } - return ks; - } - } - ); + new Supplier<>() { public Set get() { return keyset0(); }}); + + private Set keyset0() { + final Set ks = createSet(); + ks.addAll(handleKeySet()); + if (parent != null) { + ks.addAll(parent.keySet()); + } + return ks; + } + }