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.
Cosmin Stejerean made changes -
Field Original Value New Value
Attachment CLJ-926-fixed-printing-instants.patch [ 10892 ]
Cosmin Stejerean made changes -
Patch Code [ 10001 ]
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.
Cosmin Stejerean made changes -
Attachment CLJ-926-thread-local-date-format.patch [ 10894 ]
Cosmin Stejerean made changes -
Attachment CLJ-926-fixed-printing-instants.patch [ 10892 ]
Cosmin Stejerean made changes -
Attachment CLJ-926-thread-local-date-format.patch [ 10894 ]
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.
Cosmin Stejerean made changes -
Cosmin Stejerean made changes -
Patch Code [ 10001 ] Code and Test [ 10002 ]
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.
Fogus made changes -
Waiting On stuart.sierra
Approval Screened [ 10004 ]
Labels instant reader tagged-literals
Hide
Stuart Sierra added a comment -

Vetted. Will apply later.

Show
Stuart Sierra added a comment - Vetted. Will apply later.
Stuart Sierra made changes -
Approval Screened [ 10004 ] Vetted [ 10003 ]
Hide
Stuart Sierra added a comment -

Not Vetted. Test. That thing.

Show
Stuart Sierra added a comment - Not Vetted. Test. That thing.
Stuart Sierra made changes -
Approval Vetted [ 10003 ] Test [ 10013 ]
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.
Stuart Sierra made changes -
Approval Test [ 10013 ] Screened [ 10004 ]
Hide
Stuart Sierra added a comment -

Superseded by CLJ-928.

Show
Stuart Sierra added a comment - Superseded by CLJ-928.
Stuart Sierra made changes -
Resolution Declined [ 2 ]
Status Open [ 1 ] Closed [ 6 ]

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: