Bugzilla #44887:

Fixed potential multi-threading problem concerning the use of DecimalFormat.

Results from performance measurements in a separate test (operation repeated 100'000 times, exemplary):
shared static variable: ~220ms (old choice, problematic!)
always create new instance: ~480ms
ThreadLocal: ~220ms (new choice)

git-svn-id: https://svn.apache.org/repos/asf/xmlgraphics/fop/branches/fop-0_95@669173 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/src/java/org/apache/fop/fonts/MultiByteFont.java b/src/java/org/apache/fop/fonts/MultiByteFont.java
index e40c40985eff686916b08e9f69184fef60cd9a10..63497f653ab042cb7e1a27bb9219de6aa6068483 100644 (file)
--- a/src/java/org/apache/fop/fonts/MultiByteFont.java
+++ b/src/java/org/apache/fop/fonts/MultiByteFont.java
@@ -29,7 +29,6 @@ import java.util.Map;
 public class MultiByteFont extends CIDFont {
 
     private static int uniqueCounter = -1;
-    private static final DecimalFormat COUNTER_FORMAT = new DecimalFormat("00000");
 
     private String ttcName = null;
     private String encoding = "Identity-H";
@@ -63,7 +62,8 @@ public class MultiByteFont extends CIDFont {
                 uniqueCounter = 0; //We need maximum 5 character then we start again
             }
         }
-        String cntString = COUNTER_FORMAT.format(uniqueCounter);
+        DecimalFormat counterFormat = new DecimalFormat("00000");
+        String cntString = counterFormat.format(uniqueCounter);
         
         //Subset prefix as described in chapter 5.5.3 of PDF 1.4
         StringBuffer sb = new StringBuffer("E");

diff --git a/src/java/org/apache/fop/pdf/PDFNumber.java b/src/java/org/apache/fop/pdf/PDFNumber.java
index 2a8d6c47246cbdba4ebb52661171f1b01d489419..588f3ce12be1a62fde41c018c63c8aa43376b086 100644 (file)
--- a/src/java/org/apache/fop/pdf/PDFNumber.java
+++ b/src/java/org/apache/fop/pdf/PDFNumber.java
@@ -67,10 +67,32 @@ public class PDFNumber extends PDFObject {
         return doubleOut(doubleDown, 6);
     }
 
-    // Static cache. Possible concurrency implications. See comment in doubleOut(double, int).
-    private static DecimalFormat[] decimalFormatCache = new DecimalFormat[17];
-    
     private static final String BASE_FORMAT = "0.################";
+
+    private static class DecimalFormatThreadLocal extends ThreadLocal {
+        
+        private int dec;
+        
+        public DecimalFormatThreadLocal(int dec) {
+            this.dec = dec;
+        }
+        
+        protected synchronized Object initialValue() {
+            String s = "0";
+            if (dec > 0) {
+                s = BASE_FORMAT.substring(0, dec + 2);
+            }
+            DecimalFormat df = new DecimalFormat(s, new DecimalFormatSymbols(Locale.US));
+            return df;
+        }
+    };
+    //DecimalFormat is not thread-safe!
+    private static final ThreadLocal[] DECIMAL_FORMAT_CACHE = new DecimalFormatThreadLocal[17];
+    static {
+        for (int i = 0, c = DECIMAL_FORMAT_CACHE.length; i < c; i++) {
+            DECIMAL_FORMAT_CACHE[i] = new DecimalFormatThreadLocal(i);
+        }
+    }
     
     /**
      * Output a double value to a string suitable for PDF.
@@ -82,29 +104,15 @@ public class PDFNumber extends PDFObject {
      * @return the value as a string
      */
     public static String doubleOut(double doubleDown, int dec) {
-        if (dec < 0 || dec >= decimalFormatCache.length) {
+        if (dec < 0 || dec >= DECIMAL_FORMAT_CACHE.length) {
             throw new IllegalArgumentException("Parameter dec must be between 1 and " 
-                    + (decimalFormatCache.length + 1));
+                    + (DECIMAL_FORMAT_CACHE.length + 1));
         }
-        if (decimalFormatCache[dec] == null) {
-            //We don't care about the rare case where a DecimalFormat might be replaced in
-            //a multi-threaded environment, so we don't synchronize the access to the static
-            //array (mainly for performance reasons). After all, the DecimalFormat instances
-            //read-only objects so it doesn't matter which instance is used as long as one
-            //is available.
-            String s = "0";
-            if (dec > 0) {
-                s = BASE_FORMAT.substring(0, dec + 2);
-            }
-            DecimalFormat df = new DecimalFormat(s, new DecimalFormatSymbols(Locale.US));
-            decimalFormatCache[dec] = df;
-        }
-        return decimalFormatCache[dec].format(doubleDown);
+        DecimalFormat df = (DecimalFormat)DECIMAL_FORMAT_CACHE[dec].get();
+        return df.format(doubleDown);
     }
 
-    /**
-     * {@inheritDoc}
-     */
+    /** {@inheritDoc} */
     protected String toPDFString() {
         if (getNumber() == null) {
             throw new IllegalArgumentException(

diff --git a/status.xml b/status.xml
index 7a9862cecddc5c802123c8dd4b165b202dc1e116..66cd2bbf7a93206cd806e0c09184d0b184cf2519 100644 (file)
--- a/status.xml
+++ b/status.xml
@@ -60,8 +60,11 @@
       -->
     <!--/release-->
     <release version="0.95" date="TBD">
+      <action context="Code" dev="JM" type="fix" fixes-bug="44887">
+        Fixed potential multi-threading problem concerning the use of DecimalFormat.
+      </action>
       <action context="Layout" dev="JM" type="fix" fixes-bug="44412">
-        Regression bugfix: Multiple collapsable breaks don't cause empty pages anymore.
+        Regression bugfix: Multiple collapsible breaks don't cause empty pages anymore.
       </action>
       <action context="Renderers" dev="JM" type="fix">
         Fixed resolution handling inside AWT preview dialog.