<< 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: 0
Labels: instant, reader, tagged-literals

Attachments: Text File CLJ-926-round-trip-date-instants-with-tests.patch    
Patch: Code and Test
Approval: Screened
Waiting On: Stuart Sierra


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")
user> (.getHours d)
user> (.getTimezoneOffset d)

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)
clojure.instant> (format "%1$tT" d)

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.

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.

Comment by Fogus [ 07/Feb/12 3:11 PM ]

Thanks for the regression test also. I'll vett this patch ASAP.

Comment by Fogus [ 17/Feb/12 1:24 PM ]

Seems reasonable to me.

Comment by Stuart Sierra [ 17/Feb/12 1:46 PM ]

Vetted. Will apply later.

Comment by Stuart Sierra [ 17/Feb/12 1:56 PM ]

Not Vetted. Test. That thing.

Comment by Stuart Sierra [ 17/Feb/12 2:10 PM ]

No, it's "Screened," not "Test." Somebody save me.

Comment by Stuart Sierra [ 17/Feb/12 3:55 PM ]

Superseded by CLJ-928.

Generated at Thu Jan 18 18:23:16 CST 2018 using JIRA 4.4#649-r158309.