Clojure

Instant literals do not round trip correctly

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Declined
  • Affects Version/s: Release 1.4
  • Fix Version/s: None
  • Component/s: None
  • Patch:
    Code and Test
  • Approval:
    Screened

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.

Activity

Hide
Cosmin Stejerean added a comment -

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

Show
Cosmin Stejerean added a comment - Not quite sure how to create a repeatable test for this since the issue depends on the local timezone.
Hide
Steve Miner added a comment -

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

Show
Steve Miner added a comment - java.util.TimeZone/setDefault could be used for testing in various timezones.
Hide
Steve Miner added a comment -

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.

Show
Steve Miner added a comment - 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.
Hide
Fogus added a comment -

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

Show
Fogus added a comment - Excellent analysis. I'll keep track of this case and vet any patches that come along. Please do attach a regression test if possible.
Hide
Cosmin Stejerean added a comment -

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

Show
Cosmin Stejerean added a comment - I attached a new patch using a SimpleDateFormat per thread using a thread local. I'll try to add some tests next.
Hide
Cosmin Stejerean added a comment -

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.

Show
Cosmin Stejerean added a comment - 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.
Hide
Steve Miner added a comment -

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.

Show
Steve Miner added a comment - 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.
Hide
Fogus added a comment -

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

Show
Fogus added a comment - Thanks for the regression test also. I'll vett this patch ASAP.
Hide
Fogus added a comment -

Seems reasonable to me.

Show
Fogus added a comment - Seems reasonable to me.
Hide
Stuart Sierra added a comment -

Vetted. Will apply later.

Show
Stuart Sierra added a comment - Vetted. Will apply later.
Hide
Stuart Sierra added a comment -

Not Vetted. Test. That thing.

Show
Stuart Sierra added a comment - Not Vetted. Test. That thing.
Hide
Stuart Sierra added a comment -

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

Show
Stuart Sierra added a comment - No, it's "Screened," not "Test." Somebody save me.
Hide
Stuart Sierra added a comment -

Superseded by CLJ-928.

Show
Stuart Sierra added a comment - Superseded by CLJ-928.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: