Clojure JIRA

<< Back to previous view

[CLJ-926] Instant literals do not round trip correctly Created: 05/Feb/12 Updated: 17/Feb/12 Resolved: 17/Feb/12
Status:	Closed
Project:	Clojure
Component/s:	None
Affects Version/s:	Release 1.4
Fix Version/s:	None

Type:

Defect

Priority:

Major

Reporter:

Cosmin Stejerean

Assignee:

Unassigned

Resolution:

Declined

Votes:

Labels:

instant, reader, tagged-literals

Attachments:

CLJ-926-round-trip-date-instants-with-tests.patch

Patch:

Code and Test

Approval:

Screened

Waiting On:

Stuart Sierra

Description

When using java.util.Date for instant literals (which is the default) instants do not round-trip properly during daylight savings. Here is an example:

(read-string "#inst \"2010-11-12T13:14:15.666-06:00\"")
#inst "2010-11-13T06:14:15.666+10:00"

I'm currently in Melbourne, which is normally GMT+10. However, on November 12th daylight savings is in effect, so the proper GMT offset is +11. The date above is actually using the correct local time (6:14:15) but with the wrong offset. The problem is more obvious when you attempt to round-trip the instant that was read.

user> #inst "2010-11-13T06:14:15.666+10:00"
#inst "2010-11-13T07:14:15.666+10:00"

Notice that we read 6:14am but the output was 7:14 with the same offset. Upon digging deeper the real culprit seems to be the use of String.format (through clojure.core/format) when outputting java.util.Date. Notice the following inside caldate->rfc3339

(format "#inst \"%1$tFT%1$tT.%1$tL%1$tz\"" d))

Let's compare the timezone offset in the underlying date with what is printed by %1$tz

user> (def d #inst "2010-11-13T06:14:15.666+10:00")
#'clojure.instant/d                                                                                                                                                                                         
user> (.getHours d)
7                                                                                                                                                                                                           
user> (.getTimezoneOffset d)
-660

For reference, the definition of getTimezoneOffset is

-(Calendar.get(Calendar.ZONE_OFFSET) + Calendar.get(Calendar.DST_OFFSET)) / (60 * 1000)

So far it looks good. 6am in GMT+10 becomes 7am in GMT+11. Let's see how String.format handles it though.

                                                                                               
clojure.instant> (format "%1$tz" d)
"+1000"                                                                                                                                                                                                     
clojure.instant> (format "%1$tT" d)
"07:14:15"

String.format prints the correct hour, but with the wrong offset. The recommended way for formatting dates is to use a DateFormatter.

The String.format approach appears to work properly for Calendar, but not for Date. Therefore the attached patch keeps the current
implementation for java.util.Calendar but uses SimpleDateFormat to handle java.util.Date correctly. This fixes the roundtrip problem.

Comments

Comment by Cosmin Stejerean [ 05/Feb/12 9:29 PM ]

Not quite sure how to create a repeatable test for this since the issue depends on the local timezone.

Comment by Steve Miner [ 06/Feb/12 8:33 AM ]

java.util.TimeZone/setDefault could be used for testing in various timezones.

Comment by Steve Miner [ 06/Feb/12 8:37 AM ]

Regarding the patch: SimpleDateFormat is a relatively heavy-weight object, so it seems bad to allocate one every time you print a Date. Unfortunately, SimpleDateFormat is not thread-safe, so you can't just share one. The Java world seems to agree that you should use JodaTime instead, but if you're stuck with the JDK, you need to have a ThreadLocal SimpleDateFormat. I'm working on my own patch along those lines.

Comment by Fogus [ 06/Feb/12 7:38 PM ]

Excellent analysis. I'll keep track of this case and vet any patches that come along. Please do attach a regression test if possible.

Comment by Cosmin Stejerean [ 06/Feb/12 8:39 PM ]

I attached a new patch using a SimpleDateFormat per thread using a thread local. I'll try to add some tests next.

Comment by Cosmin Stejerean [ 06/Feb/12 10:42 PM ]

A combined patch that uses a thread local SimpleDateFormat, tests round-tripping at a known daylight savings point (by overriding the default timezone) and checks round tripping at multiple points throughout the year (every hour of the first 4 weeks of every month of the year).

Steve, thanks for the suggestions on using a thread local and TimeZone/setDefault.

Comment by Steve Miner [ 07/Feb/12 1:32 PM ]

I filed ~~CLJ-928~~ with my patch for fixing the printing of Date and Timestamp using UTC. I copied the tests from the ~~CLJ-926~~ patch to make sure we're compatible.

[CLJ-926] Instant literals do not round trip correctly Created: 05/Feb/12 Updated: 17/Feb/12 Resolved: 17/Feb/12