[PDI-8938] Synchronization in Variables class

pentaho · Feb 5, 2016 · a7dbabe · a7dbabe
1 parent eedbeee
commit a7dbabe
Show file tree

Hide file tree

Showing 3 changed files with 24 additions and 26 deletions.
diff --git a/core/src/org/pentaho/di/core/util/StringUtil.java b/core/src/org/pentaho/di/core/util/StringUtil.java
@@ -2,7 +2,7 @@
  *
  * Pentaho Data Integration
  *
- * Copyright (C) 2002-2013 by Pentaho : http://www.pentaho.com
+ * Copyright (C) 2002-2016 by Pentaho : http://www.pentaho.com
  *
  *******************************************************************************
  *
@@ -28,11 +28,11 @@
 import java.text.DecimalFormatSymbols;
 import java.text.NumberFormat;
 import java.text.SimpleDateFormat;
-import java.util.Collections;
 import java.util.Date;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
 
 import org.pentaho.di.core.Const;
 import org.pentaho.di.core.exception.KettleValueException;
@@ -231,17 +231,14 @@ public static String substituteHex( String aString ) {
    *          the system properties to use
    * @return the string with the substitution applied.
    */
-  public static final synchronized String environmentSubstitute( String aString,
+  public static final String environmentSubstitute( String aString,
-    Map<String, String> systemProperties ) {
+    ConcurrentHashMap<String, String> systemProperties ) {
-    Map<String, String> sysMap = new HashMap<String, String>();
+    Map<String, String> sysMap = new HashMap<String, String>( systemProperties );
-    synchronized ( sysMap ) {
+
-      sysMap.putAll( Collections.synchronizedMap( systemProperties ) );
+    aString = substituteWindows( aString, sysMap );
-
+    aString = substituteUnix( aString, sysMap );
-      aString = substituteWindows( aString, sysMap );
+    aString = substituteHex( aString );
-      aString = substituteUnix( aString, sysMap );
+    return aString;
-      aString = substituteHex( aString );
-      return aString;
-    }
   }
 
   /**

diff --git a/core/src/org/pentaho/di/core/variables/Variables.java b/core/src/org/pentaho/di/core/variables/Variables.java
@@ -2,7 +2,7 @@
  *
  * Pentaho Data Integration
  *
- * Copyright (C) 2002-2013 by Pentaho : http://www.pentaho.com
+ * Copyright (C) 2002-2016 by Pentaho : http://www.pentaho.com
  *
  *******************************************************************************
  *
@@ -25,6 +25,7 @@
 import java.util.Hashtable;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 
 import org.pentaho.di.core.Const;
 import org.pentaho.di.core.exception.KettleValueException;
@@ -39,7 +40,7 @@
  * @author Sven Boden
  */
 public class Variables implements VariableSpace {
-  private Map<String, String> properties;
+  private final ConcurrentHashMap<String, String> properties = new ConcurrentHashMap<String, String>();
 
   private VariableSpace parent;
 
@@ -48,7 +49,6 @@ public class Variables implements VariableSpace {
   private boolean initialized;
 
   public Variables() {
-    properties = new Hashtable<String, String>();
     parent = null;
     injection = null;
     initialized = false;

diff --git a/core/test-src/org/pentaho/di/core/util/StringUtilTest.java b/core/test-src/org/pentaho/di/core/util/StringUtilTest.java
@@ -2,7 +2,7 @@
  *
  * Pentaho Data Integration
  *
- * Copyright (C) 2002-2013 by Pentaho : http://www.pentaho.com
+ * Copyright (C) 2002-2016 by Pentaho : http://www.pentaho.com
  *
  *******************************************************************************
  *
@@ -24,6 +24,7 @@
 
 import java.util.HashMap;
 import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
 
 import junit.framework.TestCase;
 
@@ -116,17 +117,17 @@ public void testSubstituteBasic() {
     } catch ( RuntimeException rex ) {
     }
 
-    map = createVariables1( "${", "}" );
+    ConcurrentHashMap<String, String> chMap = new ConcurrentHashMap<String, String>( createVariables1( "${", "}" ) );
-    assertEquals( "||", StringUtil.environmentSubstitute( "|%%EMPTY%%|", map ) );
+    assertEquals( "||", StringUtil.environmentSubstitute( "|%%EMPTY%%|", chMap ) );
-    assertEquals( "|case1|", StringUtil.environmentSubstitute( "|%%checkcase%%|", map ) );
+    assertEquals( "|case1|", StringUtil.environmentSubstitute( "|%%checkcase%%|", chMap ) );
-    assertEquals( "|case2|", StringUtil.environmentSubstitute( "|%%CheckCase%%|", map ) );
+    assertEquals( "|case2|", StringUtil.environmentSubstitute( "|%%CheckCase%%|", chMap ) );
-    assertEquals( "|case3|", StringUtil.environmentSubstitute( "|%%CHECKCASE%%|", map ) );
+    assertEquals( "|case3|", StringUtil.environmentSubstitute( "|%%CHECKCASE%%|", chMap ) );
-    assertEquals( "|Arecurse|", StringUtil.environmentSubstitute( "|%%recursive1%%|", map ) );
+    assertEquals( "|Arecurse|", StringUtil.environmentSubstitute( "|%%recursive1%%|", chMap ) );
-    assertEquals( "|recurseB|", StringUtil.environmentSubstitute( "|%%recursive3%%|", map ) );
+    assertEquals( "|recurseB|", StringUtil.environmentSubstitute( "|%%recursive3%%|", chMap ) );
-    assertEquals( "|ZfinalB|", StringUtil.environmentSubstitute( "|%%recursive5%%|", map ) );
+    assertEquals( "|ZfinalB|", StringUtil.environmentSubstitute( "|%%recursive5%%|", chMap ) );
 
     try {
-      StringUtil.environmentSubstitute( "|%%recursive_all%%|", map );
+      StringUtil.environmentSubstitute( "|%%recursive_all%%|", chMap );
       fail( "recursive check is failing" );
     } catch ( RuntimeException rex ) {
     }