Friday, January 23, 2009

Formatter and new machines

Why is this code broken?

public class Stringifier {
    private final DateFormatter formatter = new SimpleDateFormatter("YYYMMDD");

    public String toYYYMMDD(final Date date) {
        return formatter.format(date);
    }
}

Here's the fixed code which makes the answer obvious:

public class Stringifier {
    private final DateFormatter formatter = new SimpleDateFormatter("YYYMMDD");

    public String toYYYMMDD(final Date date) {
        synchronized(formatter) {
            return formatter.format(date);
        }
    }
}

I checked recently and the lowest-powered servers my company buys have 8 cores. There is no such thing as single-threaded code. See the bug now?

5 comments:

Unknown said...

Yes, format is not thread safe. A less contentious fix than a synchronized block is to put the SimpleDateFormat in a ThreadLocal

Darren

noah said...

SimpleDateFormatter("YYYMMDD").format(date);

might be better. Having threads wait just because another thread is formatting a date doesn't seem like a good solution to me.

Unknown said...

Hi,

as long as only formatting is required, FastDateFormat from the Apache Commons Lang project can be used as thread-safe drop-in replacement for SimpleDateFormat.

If it comes to parsing, I would suggest holding the pattern as String constant and create a new SimpleDateFormat instance from this each time.

Greets,

Gunnar

Darren Hobbs said...

It's only broken if it's called from more than one thread.

new Stringifier().toYYYMMDD() is always safe.

If the formatter field was static it would be a problem.

CARFIELD said...

Or may be using threadlocal, we do that and so far so good, which look like the fastest choice.