5

I got the following piece of code from a programmers test

private String formatDate(Date date) { String result = ""; //…. SimpleDateFormat sdf = new SimpleDateFormat("yyyy-MM-dd"); result = sdf.format(date); //… return result; }

with the additional information that several threads are using the method at once. Are there any problems with this?

My answer is that no, it should be fine (assuming that nothing else is going on in the //... parts).

My motivation is that no global or class data structures are used. The date is passed from each tread as a parameter and inside the method only local variables and local objects are being used. Thus, each thread will get and use it's own object instance of the SimpleDateFormat class.

However, this was not the "correct" answer in the test. The "correct" answer is that the class SimpleDateFormat isn't thread safe and that the access to that object therefore needs to be synchronized.

So, am I or the solution correct?

Improve this question
5
  • 4
    The "correct" answer is incorrect. The passed-in Date, however, is also unsafe and, since it is received from the outside, datarace issues could ensue. Commented Nov 16, 2012 at 9:56
  • SimpleDateFormat is a real hog to use since it is a) thread-unsafe; b) slow to initialize. One must cache a thread-local instance, the most terrible option. Commented Nov 16, 2012 at 9:57
  • @MarkoTopolnik IMHO "Slow" is a quite overused term in this context. As long as you don't run into performance issues I'd always prefer the simplicity of the method local object to the usage of ThreadLocal, which still is a closed book to many developers Commented Nov 16, 2012 at 10:02
  • @Korgen The consequences of SimpleDateFormats design on performance constrain its use cases. That doesn't imply that each and every usage of this class will have performance issues. Commented Nov 16, 2012 at 10:05
  • I still don't see the reason they don't use ThreadLocal internally within SimpleDateFormat so that we don't have to do it. That would comply with the performance constrain and the class would be thread safe. Commented Jul 11, 2019 at 22:37

2 Answers 2

Reset to default
8

Your answer is correct. SimpleDateFormat isn't thread-safe that's true but each method call will create an own instance so this is ok. If the SimpleDateFormat were an instance variable this wouldn't be thread-safe (as you mentioned).

Improve this answer
Sign up to request clarification or add additional context in comments.

1 Comment

If we declare SimpleDateFormat as an instance variable then we can use synchronized block to make it thread-safe, Right? But sonar shows warning squid-AS2885 . Is there any way to resolve sonar issue?
3

SimpleDateFormatter is not a problem - this is a local variable and it can not be accessed from multiple threads because it is not exposed to the outside. The real problem is Date parameter (as @Marko Topolnik already said). This object can be passed to the method and some thread, that can modify it in the middle of your formatDate method execution. You can use long as parameter type to prevent datarace. To convert Date to long use Date.getTime() method and to create Date from long you can use new Date(long) constructor.

Improve this answer

5 Comments

If the same Date object is shared between multiple threads that modify it and its access is not synchronised then the use of Date.getTime() won't be sufficient to eliminate all problems. It will reduce the window of opportunity but won't eliminate it completely because the value of the Date object could still be changed by another thread before the Date.getTime() get called. If there is some concern about sharing the same Date object between threads than the access to this object should be synchronised and using Date.getTime() instead won't be a solution to this problem
Using long type will save the method's contract in multithreaded environment. If Date type will be used instead then the one part of the method can be executed with the one Date instance and the other part of the method can be executed with the other Date instance. This situation will break the method's contract. Moreover, we does not know how asker will use Date instances and can not propose how to synchronize access properly. According to the answer he accepted he does not need any synchronization on the Date instance at all.
There is no need to respect any contract because the value of the date is stored in the private calendar object: "calendar.setTime(date);"; therefore, all parts of the method "sdf.format(date)" are executed on the same value of the date even if this date object is changed by another thread in the mean time. In the context of a multithread race condition, the wrong value might be returned but this value won't be corrupted (if this makes sense saying that).
I am not shure about //… part. This part may affect something valueable and not local, for example. But if there is nothing valuable, then you are correct from my point of view.
For the sake of completness, I should have mentionned that I was referring to the "protected Calendar calendar" object that is part of the DateFormat and SimpleDateFormat classes; the very same object that is responsible for their non-thread safety. See grepcode.com/file_/repository.grepcode.com/java/root/jdk/…

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.