Multi threading issues with this code?

[Jes Sie]

Hi folks,

A lead developer in my team mentioned that the occasional exception is caused by multi-threading issues with this piece of code. We agree with that, but I was wondering which part of the code is the problem. Pls show me.

Also, can we solve this problem by removing static and putting sync in the init method?

 
public class ListCompareHelper
{
    private static Log log = LogFactory.getLog(ListCompareHelper.class);
    
    private static final SimpleDateFormat dateFormatter = new SimpleDateFormat(Constants.DATE_FORMAT);
private static NumberFormat numberFormat;

private static boolean isInitialized = false;
 
    private ListCompareHelper ()
    {
    }
    
    protected static void init() {    
dateFormatter.setLenient(false);
        
boolean usePoint = false;
if (usePoint) {
numberFormat = NumberFormat.getNumberInstance(Locale.US);
} else {
numberFormat = NumberFormat.getNumberInstance(Locale.GERMAN);
}

isInitialized = true;
    }
 

    public static final boolean isValid (String value1, CompareType compareType, String value2)
                                  throws FilterException
    {
if (!isInitialized) {
        init();        
}
 

if (compareType.getDataType().getId() == DataType.NUMBER.longValue())
        {
 
double doubleValue1 = numberFormat.parse(value1).doubleValue();
double doubleValue2 = numberFormat.parse(value2).doubleValue();                    
return check(doubleValue1, compareType, doubleValue2);
}
....
    }
 

    protected static final boolean check (double value1, CompareType compareType, double value2)
                                   
    {
        double compare = value1 - value2;
        boolean isValid = false;
 
        if (compareType.getId() == CompareType.NUMBER_LESS_THAN.longValue())
        {
            isValid = compare < 0;
        } else 
        ....
 
        return isValid;
    }
 

}

[David Weitzman]

I would recommend replacing code that looks like this:

if (!isInitialized) {
   init();        
}

with code that looks like this:

init();

And modify the init() method to check if it's been called before and do nothing if it has. There are two major benefits. First, less code duplication. Second, you can then synchronize the init() method and you shouldn't have anything else to worry about.

If all of the public methods call init(), there's an even better option: Put the initialization code into a static initializer block. The virtual machine will load the class into memory and run the initialization code in a thread-safe way the first time a method in the class is called.

[Jes Sie]

Thanks David,

Your suggestions are wise.

The occasional exceptions that we've got is a NumberFormat exception. I was wondering whether is this due to java.text.NumberFormat? http://java.sun.com/j2se/1.4.2/docs/api/java/text/NumberFormat.html

Number formats are generally not synchronized. It is recommended to create separate format instances for each thread. If multiple threads access a format concurrently, it must be synchronized externally.

As it is, I think the init method (as your rightly pointed out) is bad....and as a result NumberFormat is stuffed. What do you think?

[David Harkness]

Originally posted by Jes Sie:
The occasional exceptions that we've got is a NumberFormat exception.

Exactly. The number and date formatters are not thread-safe: they maintain internal state while parsing and formatting. This means that if two threads are using the same formatter simultaneously, they will interfere with each other, occassionally leading to an NFE.

The solutions are those specified: synchronize access to the formatter if contention is small, create them on the fly (somewhat expensive, but maybe not nearly so with modern VMs), create one per thread (use ThreadLocal -- this might be more expensive than creating on the fly), or create a pool (probably overkill).

[David Weitzman]

Ah, good point. I had assumed NumberFormat would probably be safe, but I see that's not the case. I would probably start by extracting calls to NumberFormat.parse() (and any other NumberFormat methods you use) into static synchronized methods in your class.

For example,

double doubleValue1 = numberFormat.parse(value1).doubleValue();

becomes

double doubleValue1 = parseDouble();
 
// .. Later on ...
private static synchronized double parseDouble(String value) {
  return numberFormat.parse(value).doubleValue();
}

If you're using an IDE with automated refactoring it might actually be easier to create a subclass of NumberFormat called SynchronizedNumberFormat and then run a "Replace Inheritence with Delegation" refactoring. You IDE will be able to create method stubs for all the NumberFormatMethods that just call the corrisponding methods on the delegate object. If you just mark all the SynchronizedNumberFormat methods as synchronized then you can just take the exact code you have an replace

numberFormat = NumberFormat.getNumberInstance(...);

with

numberFormat = new SynchronizedNumberFormat(NumberFormat.getNumberInstance(...));

wherever it occurs. If the numberFormat gets used in a lot of places I might want to go with this option just to prevent missing any uses or forgetting to synchronize when you add new code that uses the numberFormat.

Another theoretical option is to give each thread its own NumberFormat instance using ThreadLocal objects to avoid contention, but I doubt that has any benefits here since all the synchronized blocks will return pretty quickly. A simpler version of the same thing would be to create (or clone) a new NumberFormat every time you need one, which can give the garbage collector a workout under high loads but usually isn't a problem. Anyway, I recommend just synchronizing access to the methods you use for now.

[David Harkness]

Looking at the code for SimpleDateFormat gives me another idea for the curious, though I haven't run any benchmarks. The constructor and initialization methods for SDF do quite a bit of work. Some of the results are cached, but quite a few temporary small objects are created. The implementation of clone(), however, is quite simple.

So the idea is to create and initialize a master formatter and then clone it before each use. My theory is that cloning it will be far quicker than initializing a new one programmatically. If you try this, please let us know.

[Warren Dew]

Clone() isn't thread safe by default either, is it? Seems to me you'd have the same synchronization issues....

[David Harkness]

Originally posted by Warren Dew:
Clone() isn't thread safe by default either, is it? Seems to me you'd have the same synchronization issues....

No, clone() by itself is not thread-safe. If another thread is modifying the object being cloned, problems will arise. In this case, however, the "prototype" object created at initialization time is never modified by any thread -- it is created once and cloned over and over.

As long as clone() does not modify the object being cloned, this is safe. If it does, it is violating the contract for clone().

Here's a trimmed-down example:

public class FormatHelper {
    private static final DateFormat prototype = new SimpleDateFormat(...);
 
    protected static DateFormat getDateFormatter()
    {
        return (DateFormat) prototype.clone();
    }
 
    public static String format (Date date)
    {
        return getDateFormatter().format(date);
    }
}

To avoid massive proliferation of temporary formatters, you could make the helper class non-static and have it grab a formatter clone during construction and reuse it for subsequent operations. As long as you don't share helper objects among threads, you'll achieve single-threaded access to the formatters themselves.

Here's the same example modified as such:

public class FormatHelper {
    private static final DateFormat prototype = new SimpleDateFormat(...);
 
    protected static DateFormat getDateFormatter()
    {
        return (DateFormat) prototype.clone();
    }
 
    private DateFormat formatter;
 
    public FormatHelper()
    {
        formatter = getDateFormatter();
    }
 
    public String format (Date date)
    {
        return formatter.format(date);
    }
}

[ August 24, 2004: Message edited by: David Harkness ]