#340 remove "synchronized" modifiers that seemed unneeded to me

in some places, I replaced them with `synchronizedMap()` or `ConcurrentHashMap`. Not 100% sure nothing gets broken. :) But at least all tests are still green.
flyingsaucerproject · Jun 30, 2024 · f8b331c · f8b331c
1 parent 6c6f3ee
commit f8b331c
Show file tree

Hide file tree

Showing 9 changed files with 62 additions and 79 deletions.
diff --git a/flying-saucer-core/src/main/java/org/xhtmlrenderer/context/StylesheetFactoryImpl.java b/flying-saucer-core/src/main/java/org/xhtmlrenderer/context/StylesheetFactoryImpl.java
@@ -40,6 +40,8 @@
 import java.util.Map;
 import java.util.logging.Level;
 
+import static java.util.Collections.synchronizedMap;
+
 /**
  * A Factory class for Cascading Style Sheets. Sheets are parsed using a single
  * parser instance for all sheets. Sheets are cached by URI using LRU test,
@@ -56,15 +58,15 @@ public class StylesheetFactoryImpl implements StylesheetFactory {
     /**
      * an LRU cache
      */
-    private final Map<String, Stylesheet> _cache = new StylesheetCache();
+    private final Map<String, Stylesheet> _cache = synchronizedMap(new StylesheetCache());
     private final CSSParser _cssParser;
 
     public StylesheetFactoryImpl(UserAgentCallback userAgentCallback) {
         _userAgentCallback = userAgentCallback;
         _cssParser = new CSSParser((uri, message) -> XRLog.cssParse(Level.WARNING, "(" + uri + ") " + message));
     }
 
-    public synchronized Stylesheet parse(Reader reader, StylesheetInfo info) {
+    public Stylesheet parse(Reader reader, StylesheetInfo info) {
         try {
             return _cssParser.parseStylesheet(info.getUri(), info.getOrigin(), reader);
         } catch (IOException e) {
@@ -96,7 +98,7 @@ private Stylesheet parse(StylesheetInfo info) {
         }
     }
 
-    public synchronized Ruleset parseStyleDeclaration(int origin, String styleDeclaration) {
+    public Ruleset parseStyleDeclaration(int origin, String styleDeclaration) {
         return _cssParser.parseDeclaration(origin, styleDeclaration);
     }
 
@@ -108,7 +110,7 @@ public synchronized Ruleset parseStyleDeclaration(int origin, String styleDeclar
      *              factory.
      * @param sheet The sheet to cache.
      */
-    public synchronized void putStylesheet(String key, Stylesheet sheet) {
+    public void putStylesheet(String key, Stylesheet sheet) {
         _cache.put(key, sheet);
     }
 
@@ -117,32 +119,21 @@ public synchronized void putStylesheet(String key, Stylesheet sheet) {
      *         Note that the Stylesheet may be null.
      */
     //TODO: work out how to handle caching properly, with cache invalidation
-    public synchronized boolean containsStylesheet(String key) {
+    public boolean containsStylesheet(String key) {
         return _cache.containsKey(key);
     }
 
-    /**
-     * Returns a cached sheet by its key; null if no entry for that key.
-     *
-     * @param key The key for this sheet; same as key passed to
-     *            putStylesheet();
-     * @return The stylesheet
-     */
-    public synchronized Stylesheet getCachedStylesheet(String key) {
-        return _cache.get(key);
-    }
-
     /**
      * Removes a cached sheet by its key.
      *
      * @param key The key for this sheet; same as key passed to
      *            putStylesheet();
      */
-    public synchronized void removeCachedStylesheet(String key) {
+    public void removeCachedStylesheet(String key) {
         _cache.remove(key);
     }
 
-    synchronized void flushCachedStylesheets() {
+    void flushCachedStylesheets() {
         _cache.clear();
     }
 
@@ -157,7 +148,7 @@ synchronized void flushCachedStylesheets() {
     public Stylesheet getStylesheet(StylesheetInfo info) {
         XRLog.load("Requesting stylesheet: " + info.getUri());
 
-        Stylesheet s = getCachedStylesheet(info.getUri());
+        Stylesheet s = _cache.get(info.getUri());
         if (s == null && !containsStylesheet(info.getUri())) {
             s = parse(info);
             putStylesheet(info.getUri(), s);

diff --git a/flying-saucer-core/src/main/java/org/xhtmlrenderer/css/constants/IdentValue.java b/flying-saucer-core/src/main/java/org/xhtmlrenderer/css/constants/IdentValue.java
@@ -24,8 +24,8 @@
 import org.xhtmlrenderer.css.style.FSDerivedValue;
 import org.xhtmlrenderer.util.XRRuntimeException;
 
-import java.util.HashMap;
 import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
 
 
 /**
@@ -53,7 +53,7 @@
  * @author Patrick Wright
  */
 public class IdentValue implements FSDerivedValue {
-    private static final Map<String, IdentValue> ALL_IDENT_VALUES = new HashMap<>();
+    private static final Map<String, IdentValue> ALL_IDENT_VALUES = new ConcurrentHashMap<>();
     private static int maxAssigned;
 
     private final String ident;
@@ -265,7 +265,7 @@ public static int getIdentCount() {
      *
      * @param ident The feature to be added to the Value attribute
      */
-    private static synchronized IdentValue addValue(String ident) {
+    private static IdentValue addValue(String ident) {
         IdentValue val = new IdentValue(ident);
         ALL_IDENT_VALUES.put(ident, val);
         return val;

diff --git a/flying-saucer-core/src/main/java/org/xhtmlrenderer/css/style/CalculatedStyle.java b/flying-saucer-core/src/main/java/org/xhtmlrenderer/css/style/CalculatedStyle.java
@@ -47,10 +47,10 @@
 import javax.annotation.ParametersAreNonnullByDefault;
 import java.awt.*;
 import java.util.ArrayList;
-import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.logging.Level;
 
 import static org.xhtmlrenderer.css.style.CssKnowledge.BLOCK_EQUIVALENTS;
@@ -106,7 +106,7 @@ public class CalculatedStyle {
     /**
      * Cache child styles of this style that have the same cascaded properties
      */
-    private final Map<String, CalculatedStyle> _childCache = new HashMap<>();
+    private final Map<String, CalculatedStyle> _childCache = new ConcurrentHashMap<>();
 
     /**
      * Our main array of property values defined in this style, keyed
@@ -149,7 +149,7 @@ private CalculatedStyle(CalculatedStyle parent, CascadedStyle matched) {
 
     private boolean checkPaddingAllowed(IdentValue display) {
         return display != IdentValue.TABLE_HEADER_GROUP && display != IdentValue.TABLE_ROW_GROUP &&
-                display != IdentValue.TABLE_FOOTER_GROUP && display != IdentValue.TABLE_ROW && 
+                display != IdentValue.TABLE_FOOTER_GROUP && display != IdentValue.TABLE_ROW &&
                 (!isTable(display) || !isCollapseBorders());
     }
 
@@ -173,15 +173,9 @@ private static boolean checkBordersAllowed(IdentValue display) {
      * @param matched the CascadedStyle to apply
      * @return The derived child style
      */
-    public synchronized CalculatedStyle deriveStyle(CascadedStyle matched) {
+    public CalculatedStyle deriveStyle(CascadedStyle matched) {
         String fingerprint = matched.getFingerprint();
-        CalculatedStyle cs = _childCache.get(fingerprint);
-
-        if (cs == null) {
-            cs = new CalculatedStyle(this, matched);
-            _childCache.put(fingerprint, cs);
-        }
-        return cs;
+        return _childCache.computeIfAbsent(fingerprint, (key) -> new CalculatedStyle(this, matched));
     }
 
     public CalculatedStyle getParent() {

diff --git a/flying-saucer-core/src/main/java/org/xhtmlrenderer/css/style/EmptyStyle.java b/flying-saucer-core/src/main/java/org/xhtmlrenderer/css/style/EmptyStyle.java
@@ -27,13 +27,5 @@
  * @author Torbjoern Gannholm
  */
 public class EmptyStyle extends CalculatedStyle {
-
-    /**
-     * Creates a new instance of EmptyStyle
-     */
-    public EmptyStyle() {
-        super();
-    }
-
 }
 
diff --git a/flying-saucer-core/src/main/java/org/xhtmlrenderer/render/Box.java b/flying-saucer-core/src/main/java/org/xhtmlrenderer/render/Box.java
@@ -228,17 +228,17 @@ public List<Box> getChildren() {
     public static final int CHILDREN_FLUX = 2;
     public static final int DONE = 3;
 
-    private int _state = NOTHING;
+    private volatile int _state = NOTHING;
 
     public static final int DUMP_RENDER = 2;
 
     public static final int DUMP_LAYOUT = 1;
 
-    public synchronized int getState() {
+    public int getState() {
         return _state;
     }
 
-    public synchronized void setState(int state) {
+    public void setState(int state) {
         _state = state;
     }
 

diff --git a/...ucer-core/src/main/java/org/xhtmlrenderer/simple/extend/XhtmlCssOnlyNamespaceHandler.java b/...ucer-core/src/main/java/org/xhtmlrenderer/simple/extend/XhtmlCssOnlyNamespaceHandler.java
@@ -52,7 +52,7 @@
 public class XhtmlCssOnlyNamespaceHandler extends NoNamespaceHandler {
 
     private static final String _namespace = "http://www.w3.org/1999/xhtml";
-    private static StylesheetInfo _defaultStylesheet;
+    private static volatile StylesheetInfo _defaultStylesheet;
     private static boolean _defaultStylesheetError;
 
     /**
@@ -363,6 +363,10 @@ public List<StylesheetInfo> getStylesheets(Document doc) {
     @Nullable
     @CheckReturnValue
     public StylesheetInfo getDefaultStylesheet(StylesheetFactory factory) {
+        if (_defaultStylesheet != null) {
+            return _defaultStylesheet;
+        }
+
         synchronized (XhtmlCssOnlyNamespaceHandler.class) {
             if (_defaultStylesheet != null) {
                 return _defaultStylesheet;

diff --git a/flying-saucer-core/src/main/java/org/xhtmlrenderer/util/XRLog.java b/flying-saucer-core/src/main/java/org/xhtmlrenderer/util/XRLog.java
@@ -24,6 +24,8 @@
 import java.util.List;
 import java.util.logging.Level;
 
+import static java.util.Collections.unmodifiableList;
+
 
 /**
  * Utility class for using the java.util.logging package. Relies on the standard
@@ -48,7 +50,7 @@ public class XRLog {
     public static final String LAYOUT = registerLoggerByName("org.xhtmlrenderer.layout");
     public static final String RENDER = registerLoggerByName("org.xhtmlrenderer.render");
 
-    private static boolean initPending = true;
+    private static volatile boolean initPending = true;
     private static XRLogger loggerImpl;
 
     private static boolean loggingEnabled = true;
@@ -66,8 +68,7 @@ private static String registerLoggerByName(final String loggerName) {
      * @return List of loggers, never null.
      */
     public static List<String> listRegisteredLoggers() {
-        // defensive copy
-        return new ArrayList<>(LOGGER_NAMES);
+        return unmodifiableList(LOGGER_NAMES);
     }
 
 
@@ -199,19 +200,15 @@ public static void render(Level level, String msg, Throwable th) {
         log(RENDER, level, msg, th);
     }
 
-    public static synchronized void log(String where, Level level, String msg) {
-        if (initPending) {
-            init();
-        }
+    public static void log(String where, Level level, String msg) {
+        init();
         if (isLoggingEnabled()) {
             loggerImpl.log(where, level, msg);
         }
     }
 
-    public static synchronized void log(String where, Level level, String msg, Throwable th) {
-        if (initPending) {
-            init();
-        }
+    public static void log(String where, Level level, String msg, Throwable th) {
+        init();
         if (isLoggingEnabled()) {
             loggerImpl.log(where, level, msg, th);
         }
@@ -237,25 +234,23 @@ public static void main(String[] args) {
     }
 
     private static void init() {
-        synchronized (XRLog.class) {
-            if (!initPending) {
-                return;
-            }
+        if (initPending) {
+            synchronized (XRLog.class) {
+                if (initPending) {
+                    XRLog.setLoggingEnabled(Configuration.isTrue("xr.util-logging.loggingEnabled", true));
 
-            XRLog.setLoggingEnabled(Configuration.isTrue("xr.util-logging.loggingEnabled", true));
+                    if (loggerImpl == null) {
+                        loggerImpl = new JDKXRLogger();
+                    }
 
-            if (loggerImpl == null) {
-                loggerImpl = new JDKXRLogger();
+                    initPending = false;
+                }
             }
-
-            initPending = false;
         }
     }
 
-    public static synchronized void setLevel(String log, Level level) {
-        if (initPending) {
-            init();
-        }
+    public static void setLevel(String log, Level level) {
+        init();
         loggerImpl.setLevel(log, level);
     }
 
@@ -266,7 +261,7 @@ public static synchronized void setLevel(String log, Level level) {
      * to configuration file property xr.util-logging.loggingEnabled, or to
      * value passed to setLoggingEnabled(bool).
      */
-    public static synchronized boolean isLoggingEnabled() {
+    public static boolean isLoggingEnabled() {
         return loggingEnabled;
     }
 
@@ -277,15 +272,15 @@ public static synchronized boolean isLoggingEnabled() {
      *                       if false, all logging calls fail silently. Corresponds
      *                       to configuration file property xr.util-logging.loggingEnabled
      */
-    public static synchronized void setLoggingEnabled(boolean loggingEnabled) {
+    public static void setLoggingEnabled(boolean loggingEnabled) {
         XRLog.loggingEnabled = loggingEnabled;
     }
 
-    public static synchronized XRLogger getLoggerImpl() {
+    public static XRLogger getLoggerImpl() {
         return loggerImpl;
     }
 
-    public static synchronized void setLoggerImpl(XRLogger loggerImpl) {
+    public static void setLoggerImpl(XRLogger loggerImpl) {
         XRLog.loggerImpl = loggerImpl;
     }
 }
diff --git a/flying-saucer-core/src/main/java/org/xhtmlrenderer/util/XRSimpleLogFormatter.java b/flying-saucer-core/src/main/java/org/xhtmlrenderer/util/XRSimpleLogFormatter.java
@@ -93,7 +93,7 @@ public String format(LogRecord record) {
      * Localize and format the message string from a log record.
      */
     @Override
-    public synchronized String formatMessage(LogRecord record) {
+    public String formatMessage(LogRecord record) {
         return super.formatMessage(record);
     }
 

diff --git a/flying-saucer-pdf/src/main/java/org/xhtmlrenderer/pdf/ITextFontResolver.java b/flying-saucer-pdf/src/main/java/org/xhtmlrenderer/pdf/ITextFontResolver.java
@@ -52,18 +52,23 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 
 import static java.util.Objects.requireNonNull;
 
 public class ITextFontResolver implements FontResolver {
     private static final Logger log = LoggerFactory.getLogger(ITextFontResolver.class);
 
     private final Map<String, FontFamily> _fontFamilies = new HashMap<>();
-    private final Map<String, FontDescription> _fontCache = new HashMap<>();
+    private final Map<String, FontDescription> _fontCache = new ConcurrentHashMap<>();
 
-    public synchronized Map<String, FontFamily> getFonts() {
+    public Map<String, FontFamily> getFonts() {
         if (_fontFamilies.isEmpty()) {
-            _fontFamilies.putAll(loadFonts());
+            synchronized (_fontFamilies) {
+                if (_fontFamilies.isEmpty()) {
+                    _fontFamilies.putAll(loadFonts());
+                }
+            }
         }
         return _fontFamilies;
     }
@@ -97,7 +102,9 @@ public FSFont resolveFont(SharedContext renderingContext, FontSpecification spec
 
     @Override
     public void flushCache() {
-        _fontFamilies.clear();
+        synchronized (_fontFamilies) {
+            _fontFamilies.clear();
+        }
         _fontCache.clear();
     }
 
@@ -269,7 +276,7 @@ private boolean fontSupported(String uri) {
     }
 
     private void addFontFaceFont(
-            String fontFamilyNameOverride, IdentValue fontWeightOverride, IdentValue fontStyleOverride, String uri, String encoding, boolean embedded, 
+            String fontFamilyNameOverride, IdentValue fontWeightOverride, IdentValue fontStyleOverride, String uri, String encoding, boolean embedded,
             byte[] afmttf, byte[] pfb)
             throws DocumentException, IOException {
         String lower = uri.toLowerCase();