Is this the correct way to do java.text.DateFormat threadSafe?
I am working on a project where the date format is stored as a Utility static field as shown below.
public static final SimpleDateFormat MM_DD_YYYY = new SimpleDateFormat(DATE_FORMAT_DEFAULT, Locale.US);
Fortunately, FindBugs have started warning that DateFormats are inherently unsafe for multithreading use.
To remove these warnings, one of the developers on the team changed the access to the private field and provided a public access function such as
private static final SimpleDateFormat MM_DD_YYYY = new SimpleDateFormat(DATE_FORMAT_DEFAULT, Locale.US);
public static SimpleDateFormat getMmDdYyyy() {
return MM_DD_YYYY;
}
FindBugs magic warnings are gone! My question is, aren't both code sampling the same semantically?
If so, why doesn't FindBugs display warnings in the second case?
source to share
Well, the best part is that having public fields is almost always a bad idea, especially for mutable objects.
However, it is definitely not thread safe. Two threads could call the method completely and end up using the formatter at the same time.
You can create your own wrapper object (possibly still extending DateFormat
; I'm not sure about carelessly) that serialized requests and possibly had a "pool" of base formats. Either you can use Joda Time or java.time
, of which date and time APIs are best, and there are thread safe formats.
Another alternative could only be two static methods: parseMonthDayYear
and formatMonthDayYear
, and let them take care of thread safety.
source to share
the best and quickest way to use DateFormat in a safe context is to use it in ThreadLocal. it will provide you with one instance per thread.
like this:
private ThreadLocal<DateFormat> df = new ThreadLocal<DateFormat> () {
@Override
public DateFormat get() {
return super.get();
}
@Override
protected DateFormat initialValue() {
return new SimpleDateFormat("yyyy MM dd");
}
@Override
public void remove() {
super.remove();
}
@Override
public void set(DateFormat value) {
super.set(value);
}
};
public Date convertStringToDate(String dateString) throws ParseException {
return df.get().parse(dateString);
}
have a look at this link for more details and benchmarks: http://www.javacodegeeks.com/2010/07/java-best-practices-dateformat-in.html
source to share
SimpleDateFormat
is not thread safe because it uses internally mutable state when processing. This final static
will not help, because the operations will be performed in this single instance, in which threads will interact through these states.
Solution: return a defensive copy:
private static final DateFormat MM_DD_YYYY = new SimpleDateFormat(DATE_FORMAT_DEFAULT, Locale.US);
public static DateFormat getMmDdYyyy() {
return (DateFormat) MM_DD_YYYY.clone();
}
source to share
This is just an experiment, but I'm wondering if an additional solution could create a subclass SimpleDateFormat
that overrides all methods set*()
with a no-op implementation and uses that? It seems to me that the thread safety concerns SimpleDateFormat
come from format mutability. (I can't imagine format()
there is any delay in the state on valid calls ). So if you are using SimpleDateFormat
that you know will never change, maybe use this subclass instead? You can even pass an instance SimpleDateFormat
in the constructor. This will be a kind of immutable decorator that you make with methods java.util.Collections.unmodifiable*
.
Plus: you wouldn't need to have a per thread instance or have the synchronization overhead. The downside (I think) is that this may not satisfy FindBugs (I don't have one, so I can't try it).
Update - why it won't work
It turns out that it won't work. As @biziclop pointed out in the comments below, there is actually a mutable internal state used in the methods format()
- a shared instance Calendar
. Here is a @biziclop link showing what's going on at the source SimpleDateFormat
. I will not speculate about why the developers did this, but I can safely say that it format()
is even workable, and therefore my plan will not work.
source to share