public class MyService // could be a web service or a session beanThis is particularly nasty because the code may well run without producing any exceptions but could easily parse the date incorrectly. If you don't believe how likely this is, here's some code to prove the point:
{
public void someBusinessMethod(String datestring, ...) {
Date date = GlobalConst.DATE_FMT.parse(datestring);
// rest of code omitted
}
}
public class ProveNotSafe {
static SimpleDateFormat df
= new SimpleDateFormat( "dd-MMM-yyyy" );
static String testdata[] = {
"01-Jan-1999", "14-Feb-2001", "31-Dec-2007"
};
public static void main(String[] args) {
Runnable r[] = new Runnable[ testdata.length ];
for (int i = 0; i < r.length; i++) {
final int i2 = i;
r[i] = new Runnable() {
public void run() {
try {
for (int j=0; j<1000; j++) {
String str = testdata[i2];
String str2 = null;
/*synchronized(df)*/ {
Date d = df.parse(str);
str2 = df.format( d );
}
if (!str.equals(str2)) {
throw new RuntimeException(
"date conversion failed after "
+ j + " iterations. Expected "
+ str + " but got " + str2 );
}
}
} catch (ParseException e) {
throw new RuntimeException( "parse failed" );
}
}
};
new Thread(r[i]).start();
}
}
}
When I run this code, I get the following output:
java.lang.RuntimeException: date conversion failed after 3 iterations.Note that "01-Dec-2007" isn't even one of the strings in the test data. It is actually a combination of the dates being processed by the other two threads!
Expected 14-Feb-2001 but got 01-Dec-2007
If I uncomment the synchronized keyword then of course it runs without exception.
If I want to quickly remove these issues from a code base I usually create a simple replacement class like this that wraps SimpleDateFormat.
public class ThreadSafeSimpleDateFormat {
private DateFormat df;
public ThreadSafeSimpleDateFormat(String format) {
this.df = new SimpleDateFormat(format);
}
public synchronized String format(Date date) {
return df.format(date);
}
public synchronized Date parse(String string) throws ParseException {
return df.parse(string);
}
}
Labels: java
12 Comments:
Another alternative is use commons-lang FastDateFormat
http://commons.apache.org/lang/api-release/org/apache/commons/lang/time/FastDateFormat.html
One of the annoying thing about this class is that it's NOT extends from DateFormat!
What a nasty surprise - I could think of a couple of places where this might be an issue in the apps I'm currently maintaining.
Though my personal favority way around date formatting is to use String.format().
An issue with just synchronizing the calls to parse and format is the thread blocking that will occur. Date parsing and formatting should not be a bottleneck.
Instead, each thread should create its own instance of SimpleDateFormat (which is recommended by the class's Javadoc) or use another solution altogether, like the aforementioned FastDateFormat.
wow that is nasty I cannot believe it is necessary to store *instance* variables just to parse a date! Terrible design once again in the Java libraries - not a shocker really.
I'm going to agree with an earlier comment synchronizing surely id a heavy handed solution - if there is a copy constructor think a better solution would be to make a throwaway copy - modern jvms don't have any problem allocating short lived objects - on the other hand synchronization will wreak havoc on execution pipeline.
You might be surprised to learn that the ThreadSafeDateFormat class in the post is actually only 0.6% slower than SimpleDateFormat when used in a single thread based on 100,000 iterations calling parse() and format(). Obviously it is better to re-architect code to have separate instances of the date format class for each thread but as a quick fix a global search and replace changing "SimpleDateFormat" to "ThreadSafeDateFormat" will fix some nasty thread safety issues at very little cost.
This one caught us out, too, although in fairness the lack of thread-safety is buried at the very end of the javadoc for the class, which seems a bit sub-optimal.
I chose a different solution - the use of thread local:
public class FormatTimestampDate {
/**
* Threadsafe date/time formatter
* Each thread gets it's own instance of the formatter, which is maintained for the lifetime of the thread.
*/
protected static ThreadLocal<
SimpleDateFormat> formatter = new ThreadLocal<
SimpleDateFormat>() {
/*
* use inner class to initalise instance per thread
*/
protected SimpleDateFormat initialValue() {
SimpleDateFormat sdf = new SimpleDateFormat("yyyyMMdd HH:mm:ss");
return sdf;
}
};
/**
* Accessor for the formatter
* @return
*/
public static final SimpleDateFormat get() {
return formatter.get();
}
}
@Andy,
A single threaded program may not exhibit much slowdown, I would agree - but a multi-threaded one will, especially a multi-core machine.
You should try your test again with a copy constructor version. I bet the slow down will be next to nothing.
Hi Andy,
Your example says:
public class ProveNotSafe { static ThreadSafeSimpleDateFormat df = new ThreadSafeSimpleDateFormat( "dd-MMM-yyyy" );
should it be using SimpleDateFormat as your ThreadSafeSimpleDateFormat class should work?
Or did I miss something? By the way, this post has given me an uneasy feeling about a previous project.
Hi Tim,
Well spotted! I obviously copy-and-pasted the code after testing it with my replacement class. I've updated the post now :-)
Cheers,
Andy.
I had this issue in my recent project and wasted couple of valuable working days by not knowing where is the problem.
Finally I got this culprit and fixed the issue.
I think it's best to make code block as synchronized than a method.. in ThreadSafeSimpleDateFormat class?
what's your say ?
I made a class that can make the work easier.
Please check at:
http://li-ma.blogspot.com/2007/10/thread-safe-date-formatparser.html
Sorry, the link got cut in my previous post. Please check
here.
Or it is the first post in my blog at:
http://li-ma.blogspot.com
Post a Comment
<< Home