Thursday, August 28, 2008

java.util.Calendar confusion - is it safe or not?

Currently I'm reading couple of technical and agile books (see the nice gadget on the right panel) and one of the books is "Effective Java - Second Edition" by Josh Bloch.

Josh's item number 5 - "avoid creating unnecessary objects" tells me that I should not create (unnecessary) instances of classes like java.util.Date and java.util.Calendar:

// DON'T DO THIS
public void someMethod() {
// unnecessary allocation of expensive object
Calendar cal = Calendar.getInstance();
...
}

He suggests using "heavy" objects only once, during the static initialization of the class:

public class Person {
...
static {
Calendar cal = Calendar.getInstance();
// do some initialization using Calendar instance
...
}
...
}

I can agree with this point but on the other hand consider a following snippet:

// DON'T DO THIS
public class Confusion {
private static final Calendar CALENDAR = Calendar.getInstance();

public void doSomething(Date date) {
CALENDAR.setTime(date);
...
}
}

This code does not perform initialization using Calendar but uses this class to perform some on-the-fly computations - this is the place where this example differs from the previous code snippets (I use "heavy" object after the class initialization and cannot do it in a different way). My code snippet is potentially dangerous: FindBugs tells me:

[STCAL] Call to static Calendar [STCAL_INVOKE_ON_STATIC_CALENDAR_INSTANCE]

Even though the JavaDoc does not contain a hint about it, Calendars are inherently unsafe for multihtreaded use. The detector has found a call to an instance of Calendar that has been obtained via a static field. This looks suspicous.

For more information on this see Sun Bug #6231579 and Sun Bug #6178997.

This way my code should be refactored to this shape:

public class Confusion {
public void doSomething(Date date) {
Calendar cal = Calendar.getInstance()
cal.setTime(date);
...
}
}

But hey! Josh Bloch tells me that creating java.util.Calendar is expensive and he's the Guru - he must be right. Maybe I missed the point of "creating unnecessary objects" and in my example if I want to operate on dates using java.util.Calendar object this is really necessary?

Anyway I'm a bit confused - how should I use java.lang.Calendar objects? Using them as a final static constants is dangerous but on the other hand creating them every time I need it can deteriorate the performance or memory footprint.

What do you think?

11 comments:

Anonymous said...

The Findbugs warning is shown because you are calling the static getInstance() method of Calendar, which is as of FindBugs not thread safe.
I just had the same problem and was able to get rid of the FindBugs warning by using the 'synchronized' keyword at my method.

Przemyslaw Bielicki said...

Actually you are partially right because not only getInstance() method is not thread-safe but the whole Calendar class is so.

If you add 'synchronized' keyword you will definitely get rid of FindBugs warning but you will not get rid of the problem. By adding 'synchronized' keyword you are applying "excessive synchronization" anti-pattern which can result in significant performance issues (or even deadlock). This is not very fortunate solution unless you really need your method that operates on the Calendar object be 'synchronized'.

Cheers!

Yardena said...
This comment has been removed by the author.
JodaStephen said...

The solution is to use Joda-Time - http://joda-time.sf.net - as it is built from immutable classes and is safe for threads and statics, and high performance.

evernat said...

I've just read item number 5 : http://www.informit.com/articles/article.aspx?p=1216151&seqNum=5.
And I think you don't understand it.

Josh says to avoid creating unnecessary objects for example with static fields.
But he says "the objects in question could be reused because they were not modified after initialization".
And you are modifying the static calendar in doSomething method.

Findbugs is right to warn you and it is also right that adding synchronized is probably an excessive synchronisation.

It is necessary to create a new Calendar objet in this case.

Steve said...

You could use a thread local.

Anonymous said...

Don't use Calendar at all. It's the worst thing ever invented by mankind. Use Joda-time instead (which will be in Java 7 BTW).

Przemysław Bielicki said...

Thanks for your comments - one conclusion is to use Joda-time which may be included in Java 7 (I'm stil;l not sure what will be included in Java 7 and whether it will be EVER released :)

@evernat
It's funny how people use sentences out of the context. Yes, Josh writes: "In the previous examples in this item, it was obvious that the objects in question could be reused because they were not modified after initialization." but his next sentence is: "There are other situations where it is less obvious."

Cheers!

evernat said...

@Przemyslaw

I don't understand what is funny and what is out of context.
I initially thought my response was adapted to the context that you provided.
But you are certainly right.

Przemysław Bielicki said...

It was not funny indeed, it was interesting :) Anyway I just wanted to show that such a small thing like java.util.Calendar can confuse a lot of developers.

But let's keep the conclusion that Java developers wanting to operate on dates should use Joda-time library.

Cheers!

kgilpin said...

I also suggest you use a ThreadLocal. This works for DateFormat too.