Is this static method thread safe or is synchronization needed

I have a utility class that has one static method to modify values of the input Array List. This static method is invoked by a caller. The caller is used to process web service requests. For each request(per thread), the caller creates a new ArrayList and invokes the static method.

public class Caller{

   public void callingMethod(){

     //Get Cloned criteria clones a preset search criteria that has place holders for values and returns a new ArrayList of the original criteria. Not included code for the clone
     ArrayList<Properties> clonedCriteria = getClonedCriteria();

     CriteriaUpdater.update(clonedCriteria , "key1", "old_value1", "key1_new_value");
     CriteriaUpdater.update(clonedCriteria , "key2", "old_value2", "key2_new_value");

    //do something after the this call with the modified criteria arraylist
   }

}

public class CriteriaUpdater
{
    //updates the criteria, in the form of array of property objects, by replacing the token with the new value passed in
    public static void update(ArrayList<Properties> criteria, String key, String token, String newValue)
    {
        for (Properties sc: criteria)
        {
            String oldValue = sc.getProperty(key);
            if ((oldValue != null) && (oldValue.equals(token))) 
                sc.setProperty(key, newValue);
        }
    }
}

This is how the criteria are cloned:

public synchronized static ArrayList<Properties> cloneSearchCriteria(ArrayList<Properties> criteria) { 
  if (criteria == null) return null; 
  ArrayList<Properties> criteriaClone = new ArrayList<Properties>(); 
  for (Properties sc : criteria) { 
    Properties clone = new Properties(); 
    Enumeration propertyNames = sc.propertyNames(); 
    while (propertyNames.hasMoreElements()) { 
      String key = (String) propertyNames.nextElement(); 
      clone.put(key, (String) sc.get(key)); 
    } 
    criteriaClone.add(clone);
  } 
  return criteriaClone; 
}

Given the above definitions, by not synchronizing the static method, would it still be able to correctly process concurrent method calls. My understanding is I have to synchronize this method for concurrency but wanted to confirm. I understand each thread will have its own stack, but for static method it would be common to all threads – so in this case if we don’t synchronize would it not cause a problem? Appreciate suggestions and any corrections.

Thanks

Answer

It all depends on your getClonedCriteria() method. That’s the method that is accessing shared state.

You are creating a “deep copy” of the criteria, so that every clone is independent from the original and from each other.

But there’s a more subtle problem, which is that whatever initialization is performed on the prototype criteria must happen-before any thread that reads the criteria to clone it. Otherwise, the cloning thread may read an uninitialized version of the data structure.

One way to achieve this is to initialize the prototype criteria in a static initializer and assign it to a class member variable. Another is to initialize the criteria and then assign it to a volatile variable. Or, you could initialize and assign the prototype (in either order) to an ordinary class or instance member variable inside a synchronized block (or using a Lock), and then read the variable from another block synchronized on the same lock.

Leave a Reply

Your email address will not be published. Required fields are marked *