Lees de Javadoc
By: Jan-Kees van Andel, 27 June 2008
Om de een of andere reden zie je nog steeds vaak dat mensen hun SimpleDateFormat instanties in members verpakken.
DOE DIT NIET!
De Javadoc van SimpleDateFormat zegt heel duidelijk dat de klasse niet thread safe is en dus ook niet op zo’n manier gebruikt mag worden.
http://java.sun.com/j2se/1.4.2/docs/api/java/text/SimpleDateFormat.html#synchronization
Date formats are not synchronized. It is recommended to create separate format instances for each thread. If multiple threads access a format concurrently, it must be synchronized externally.
Dit is niet alleen een theoretisch probleem, maar kan tot hele vreemde bugs leiden, zoals:
- OutOfBoundsExceptions op positie 13 (in een datum :S),
- java.lang.NumberFormatException: For input string: “”,
- java.lang.NumberFormatException: multiple points,
- java.lang.NumberFormatException: empty String,
- Of gewoon verkeerde output (invoer 11-11-1911, output 11-01-0001).
Dit waren gewoon een paar voorbeeldjes uit wat test runs. Er zullen vast nog wel vreemdere fouten kunnen optreden. Bovendien komen dergelijke bugs vaak alleen in productie (onder hoge load) aan het licht, waardoor het nog vervelender wordt.
Dus maak nooit meer private static final SimpleDateFormat SDF = new SimpleDateFormat(“dd-MM-yyyy”); constructies, tenzij je alle aanroepen naar dat object synchronized maakt.
Maar dit laatste is ook geen optie, om twee redenen.
- Creatie van SimpleDateFormat objecten is heel goedkoop, dus er is eigenlijk geen reden om ze te hergebruiken/poolen. Bovendien, als je ze op stack niveau gebruikt, kan de compiler en de Java runtime het veel efficiënter verwerken.
- Synchronized blokken zijn op zichzelf niet meer duur, zeker niet vanaf Java SE 6. Het grote nadeel van synchronized blijft echter nog steeds staan, namelijk dat meerdere threads het code block niet parallel uit kunnen voeren, waardoor je een bottleneck introduceert.
Dus zorg dat je je SimpleDateFormat klassen op stack niveau houdt.
Eventueel, als je ECHT veel instanties aan moet maken, kun je naar pool maken. Zorg wel dat die pool stack confined is, zoals een ThreadLocal. Voorbeeld:
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 | import java.text.ParseException; import java.text.SimpleDateFormat; public class MultiThread { public static final FormatterPool POOL = new FormatterPool(); public static void main(String[] args) { for (int i = 0; i < 50; i++) { new Thread(new Worker()).start(); } } } class Worker implements Runnable { public void run() { try { SimpleDateFormat sdf = MultiThread.POOL.get(); System.out.println(sdf.format(sdf.parse("11-11-1911"))); } catch (ParseException e) { e.printStackTrace(); } catch (Throwable t) { t.printStackTrace(); } } } import java.text.SimpleDateFormat; public class FormatterPool extends ThreadLocal<SimpleDateFormat> { private static final SimpleDateFormat SDF = new SimpleDateFormat("dd-MM-yyyy"); @Override protected SimpleDateFormat initialValue() { return new SimpleDateFormat("dd-MM-yyyy"); } } |
Let op bij het gebruik van ThreadLocal. De initialValue bevat in sommige Java versies nog bugs.
27 June 2008 om 12:08 pm
Goed punt. Het aantal keren dat ik dit in code heb gezien is inmiddels ontelbaar. Reageren auto review tools als FindBugs hier trouwens niet op?
27 June 2008 om 1:37 pm
Toevallig net opgezocht. Findbugs heeft hiervoor bug patterns. Ook voor andere static mutable fields trouwens, zoals arrays. Specifiek voor SimpleDateFormat, krijg je een melding als je format of parse op een static referentie aanroept.
Ik wilde het doorgeven als bug pattern request, maar er is er al eentje…
http://findbugs.sourceforge.net/bugDescriptions.html#STCAL_INVOKE_ON_STATIC_DATE_FORMAT_INSTANCE
27 June 2008 om 1:40 pm
In het geval waar ik tegen kwam was het gewrapped in een andere klasse (en die was static).
Helaas, moet je er dan gewoon tegen aan lopen dat iemand daar nog niet van op de hoogte was. Gelukkig bleek uit de eerste performance test met JMeter al dat het vrij snel naar voren komt.
28 June 2008 om 9:57 am
SDF als attribuut opnemen in een multithreaded context is inderdaad een veel gemaakte fout. Heb deze constructie al regelmatig in code moeten verwijderen.
Hoewel het niet goed te praten is – ontwikkelaars moet altijd de api docs lezen – is het wel begrijpelijk dat SDF op deze wijze wordt gebruikt. Een formatter die je meerdere keren toepast is normaal gesproken een prima kandiaat om als constante te declareren.
Hopelijk komt de nieuwe Date/Time API (JSR-310 gebaseerd op JodaTime) snel beschikbaar, want wat nu in de JDK zit is waardeloos.
28 June 2008 om 4:01 pm
Idd, maar aan de andere kant kun je de makers van de API ook de schuld geven van dat iedereen het bijna fout doet. SimpleDateFormat klinkt namelijk alsof het een immutable object is dat alleen een datumformaat voorstelt.
SimpleDateFormatTER had een betere naam geweest, dan kun je aan de naam zien dat het een klasse is die iets doet.
30 June 2008 om 8:26 am
Hmm, FindBugs ziet deze bugs trouwens alleen als je objecten exact van het type DateFormat gebruikt, dus private static final DateFormat… Ik heb maar ff een bug ingeschoten bij FindBugs, want op deze manier is het niet echt nuttig.
http://sourceforge.net/tracker/index.php?func=detail&aid=2006102&group_id=96405&atid=614696
30 June 2008 om 12:08 pm
Tja, beetje beginners fouten volgens mij. Hoeveel mensen nemen ook niet instance variabellen in een Servlet (of Spring Bean) op. Zelfde soort beginnersfouten…