CODEWITHSUNDEEP
javamultithreadingconcurrencythread-safety
sbelvadi
sbelvadi

Reputation: 47

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

Upvotes: 1

Views: 436

Answers (3)

erickson
erickson

Reputation: 269737

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.

Upvotes: 0

Spencer
Spencer

Reputation: 174

You are correct in that each thread has its own stack, so each thread will have its own copies of local variables and method arguments when it calls update(). When it runs it will save those local variables and method arguments to its stack.

However, the method argument criteria is a reference to a mutable object that will be stored on the heap where Java objects reside. If the threads can call update() on the same ArrayList, or the elements contained in the ArrayList could be contained in more than one ArrayList passed into different invocations of update() by different threads then synchronization errors could occur.

Upvotes: 0

Peter Lawrey
Peter Lawrey

Reputation: 533570

You have a problem with a race condition. At least the underlying Properties data structure will never be corrupted but it could have an incorrect value. In particular, any number of threads could be in this section meaning the final value could be anything from any thread.

        String oldValue = sc.getProperty(key);
        if ((oldValue != null) && (oldValue.equals(token))) 
            sc.setProperty(key, newValue);

I am assuming your List is never altered, but if it is, you have to have synchronized. You could lock on the class, but locking on the collection you are altering might be a better choice.

Upvotes: 1

Related Questions