ThreadLocal and SimpleDateFormat array

Question

Using a pattern very similar to that described in a recent question, for a multithreaded application, I am getting weird date values (e.g., years like 2025 or 2035, when clearly no such value exists in the source data). It seems that a concurrency issue is occuring.

The source code looks something like

// Various Java DateFormat patterns, e.g. "yyyy-MM-dd".
private static final String[] DATE_PATTERNS = new String[] {...};

private static SimpleDateFormat[] getFormats(final String[] patterns)
{
    ThreadLocal<SimpleDateFormat[]> LOCAL_FORMATS = new ThreadLocal<SimpleDateFormat[]>()
    {
        @Override
        protected SimpleDateFormat[] initialValue()
        {
            List<SimpleDateFormat> formatList = new ArrayList<SimpleDateFormat>();

            for (String pattern:patterns)
            {
                formatList.add(new SimpleDateFormat(pattern));
            }

            return formatList.toArray(new SimpleDateFormat[formatList.size()]);
        }
    };

    return LOCAL_FORMATS.get(); // Create a thread-local copy
}

private static final SimpleDateFormat[] DATE_FORMATS = getFormats(DATE_PATTERNS);

After its static initialization, the DATE_FORMATS array is accessed by numerous classes, which in turn use the SimpleDateFormat objects of the array for parsing or formatting several date strings.

Can there be any concurrency issue in such a usage scenario, especially given the use of ThreadLocal?

JB Nizet · Accepted Answer · 2012-07-07T16:08:57.083

Yes, there can be concurrency issues. Your thread local variable doesn't serve any purpose. It's only used when the class is initialized, to temporarily store an array of date formats that is immediately retrieved and stored in a static constant.

All the threads, after, always use the same instances of date formats concurrently, without getting them from any thread local variable.

The code should rather be:

private static final String[] DATE_PATTERNS = new String[] {...};
private static final ThreadLocal<SimpleDateFormat[]> DATE_FORMATS = 
    new ThreadLocal<SimpleDateFormat[]>() {
        @Override
        protected SimpleDateFormat[] initialValue() {
            List<SimpleDateFormat> formatList = new ArrayList<SimpleDateFormat>();

            for (String pattern : DATE_PATTERNS)
            {
                formatList.add(new SimpleDateFormat(pattern));
            }

            return formatList.toArray(new SimpleDateFormat[formatList.size()]);
        }
    };

public static SimpleDateFormat[] getDateFormats() {
    return DATE_FORMATS.get();
}

I would also use an unmodifiable List<SimpleDateFormat> rather than an array, to be safer.

I was actually asking whether the solution you suggest would be the correct one while you were editing your answer. Thanks for the details! So, each method calling getDateFormats() will always get the same SimpleDateFormat[] array for the same thread, while for each new thread that array will only be initialized once, correct? — PNS, Jul 07 '12 at 16:12
Instead of getDateFormats() one could use DATE_FORMATS.get(), of course. If the SimpleDateFormat[] array never gets written, it should be thread-safe. Thanks again for the answers! — PNS, Jul 07 '12 at 16:23
Even if written, it will be thread-safe, since each thread has its own copy. But if you want to prevent an accidental reordering or nullification of the array members, you'd better make the array an unmodifiable list. — JB Nizet, Jul 07 '12 at 16:33

score 2 · Answer 2 · answered Jul 07 '12 at 15:56

2

// Various Java DateFormat patterns, e.g. "yyyy-mm-dd".

The format 'yyyy-mm-dd' is likely to give you weird results because 'mm' is minutes and not months. From the javadoc:

M   Month in year   Month   July; Jul; 07
...
m   Minute in hour  Number  30

answered Jul 07 '12 at 15:56

matt freake

4,877
4
27
56

Not actually used, just a typo in the comment above. Amended, thanks for spotting it! :-) – PNS Jul 07 '12 at 15:58

ThreadLocal and SimpleDateFormat array

2 Answers2

Linked